From b361f86a9165b20771c4533a7b5c97103a21eeab Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Tue, 9 Jun 2026 19:07:15 +0200 Subject: [PATCH 1/7] fix: close remaining no-click NTLM image-load sinks (GHSA-hr87-735w-hfq3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A sweep for auto-loading subresource sinks fed attacker-controllable (synced) content found two more no-click NTLM vectors beyond the markdown/attachment images already gated: - READ_LOCAL_IMAGE_AS_DATA_URL (electron/local-file-sync.ts): a synced theme backgroundImage 'file://host/share/x.png' reaches this main-process handler, where fileURLToPath -> fs.stat/readFile on the resulting UNC path opens an SMB connection and leaks the NTLM hash — no click, just the theme being active. This is the most reliable variant (main-process fs, not a Chromium subresource). Guard with isPathSafeToOpen before any fs access. - note.imgUrl (note.component): a synced note image URL is bound directly to /[enlargeImg] and auto-loads on render. Gate at the setter so only a safe URL reaches the bindings. Both reuse the existing isPathSafeToOpen helper; local file://, http(s), data: and blob: are unaffected. New electron test proves stat/readFile are never reached for UNC / remote file:// input. (cherry picked from commit 80a3c20b3f5a3f535f5d04d7fd215ba569493ef8) --- electron/local-file-sync-ntlm.test.cjs | 116 ++++++++++++++++++ electron/local-file-sync.ts | 10 ++ .../features/note/note/note.component.html | 4 +- src/app/features/note/note/note.component.ts | 7 ++ 4 files changed, 135 insertions(+), 2 deletions(-) create mode 100644 electron/local-file-sync-ntlm.test.cjs diff --git a/electron/local-file-sync-ntlm.test.cjs b/electron/local-file-sync-ntlm.test.cjs new file mode 100644 index 0000000000..61631d8ffd --- /dev/null +++ b/electron/local-file-sync-ntlm.test.cjs @@ -0,0 +1,116 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const Module = require('node:module'); + +require('ts-node/register/transpile-only'); + +const originalModuleLoad = Module._load; +const modulePath = path.resolve(__dirname, 'local-file-sync.ts'); + +let handlers; +let statCalls; +let readFileCalls; + +const resetModule = () => { + delete require.cache[modulePath]; +}; + +// Mock everything EXCEPT the real `is-external-url-allowed` guard and node `url`, +// so the test exercises the actual UNC / remote-file:// gate end-to-end. +const installMocks = () => { + Module._load = function patchedLoad(request, parent, isMain) { + if (request === 'electron') { + return { + ipcMain: { handle: (name, handler) => handlers.set(name, handler) }, + dialog: {}, + app: { getPath: () => '/nonexistent-userdata' }, + }; + } + if (request === 'electron-log/main') { + return { error: () => {}, log: () => {} }; + } + if (request === './main-window') { + return { getWin: () => ({}) }; + } + if (/[/\\]file-path-guard$/.test(request)) { + // Stub the userData-containment guard: this suite verifies only the + // UNC / remote-file:// gate. Containment is covered by local-file-sync.test.cjs. + return { assertPathOutside: () => {} }; + } + if (request === './shared-with-frontend/ipc-events.const') { + // Any IPC.X resolves to the string 'X'. + return { IPC: new Proxy({}, { get: (_t, prop) => prop }) }; + } + if (request === 'fs') { + const noop = () => undefined; + return { + readdirSync: noop, + readFileSync: noop, + renameSync: noop, + statSync: noop, + writeFileSync: noop, + unlinkSync: noop, + promises: { + stat: async (p) => { + statCalls.push(p); + return { size: 4 }; + }, + readFile: async (p) => { + readFileCalls.push(p); + return Buffer.from('test'); + }, + }, + }; + } + return originalModuleLoad.apply(this, [request, parent, isMain]); + }; +}; + +const loadHandler = () => { + handlers = new Map(); + statCalls = []; + readFileCalls = []; + resetModule(); + installMocks(); + // Mocks stay active through the handler call: the handler resolves `fs` lazily + // via `await import('fs')`, so it must hit the mock at invocation time too. + // afterEach restores the original loader. + require(modulePath).initLocalFileSyncAdapter(); + return handlers.get('READ_LOCAL_IMAGE_AS_DATA_URL'); +}; + +test.afterEach(() => { + Module._load = originalModuleLoad; +}); + +test('READ_LOCAL_IMAGE_AS_DATA_URL blocks UNC / remote file:// before any fs access (GHSA-hr87-735w-hfq3)', async () => { + const handler = loadHandler(); + const blocked = [ + '\\\\host\\share\\x.png', + '//host/share/x.png', + 'file://host/share/x.png', + 'file://192.168.1.100/share/x.png', + 'file:////host/share/x.png', + ]; + for (const input of blocked) { + const result = await handler(null, input); + assert.equal(result, null, `expected null for ${input}`); + } + // The security property: the filesystem was never touched for any blocked path. + assert.equal(statCalls.length, 0, 'fs.promises.stat must not be called'); + assert.equal(readFileCalls.length, 0, 'fs.promises.readFile must not be called'); +}); + +test('READ_LOCAL_IMAGE_AS_DATA_URL still reads local paths and local file:// URLs', async () => { + const handler = loadHandler(); + const okPng = await handler(null, '/home/user/img.png'); + assert.ok(okPng.startsWith('data:image/png;base64,'), 'local path should be read'); + assert.equal(statCalls.length, 1); + + const okFileUrl = await handler(null, 'file:///home/user/img.png'); + assert.ok( + okFileUrl.startsWith('data:image/png;base64,'), + 'local file:// should be read', + ); +}); diff --git a/electron/local-file-sync.ts b/electron/local-file-sync.ts index 3ecdaff41c..6a89c93543 100644 --- a/electron/local-file-sync.ts +++ b/electron/local-file-sync.ts @@ -12,6 +12,7 @@ import { app, dialog, ipcMain } from 'electron'; import { getWin } from './main-window'; import { fileURLToPath, pathToFileURL } from 'url'; import { assertPathOutside } from './file-path-guard'; +import { isPathSafeToOpen } from './shared-with-frontend/is-external-url-allowed'; // SECURITY: file-sync must never read/write/list inside the app's private dir, // which holds settings/grants/db — touching it is a privilege-escalation @@ -25,6 +26,15 @@ export const initLocalFileSyncAdapter = (): void => { IPC.READ_LOCAL_IMAGE_AS_DATA_URL, async (_, filePathOrUrl: string): Promise => { try { + // A theme background-image path is synced and thus attacker-controllable. + // Reject UNC / remote file:// before touching the filesystem: fs.stat / + // readFile on `\\host\share` (or `file://host/share`) opens an SMB + // connection and leaks the user's NTLM hash — with no click, just by the + // theme being active. See GHSA-hr87-735w-hfq3. + if (!isPathSafeToOpen(filePathOrUrl)) { + return null; + } + const normalized = filePathOrUrl.startsWith('file://') ? fileURLToPath(filePathOrUrl) : filePathOrUrl; diff --git a/src/app/features/note/note/note.component.html b/src/app/features/note/note/note.component.html index 91afb00de6..e481780f36 100644 --- a/src/app/features/note/note/note.component.html +++ b/src/app/features/note/note/note.component.html @@ -6,8 +6,8 @@ > @if (note.imgUrl) { diff --git a/src/app/features/note/note/note.component.ts b/src/app/features/note/note/note.component.ts index 2af3f2fb4b..211db76b9d 100644 --- a/src/app/features/note/note/note.component.ts +++ b/src/app/features/note/note/note.component.ts @@ -38,6 +38,7 @@ import { DEFAULT_PROJECT_COLOR } from '../../work-context/work-context.const'; import { DEFAULT_PROJECT_ICON } from '../../project/project.const'; import { ClipboardImageService } from '../../../core/clipboard-image/clipboard-image.service'; import { RenderLinksPipe } from '../../../ui/pipes/render-links.pipe'; +import { isPathSafeToOpen } from '../../../../../electron/shared-with-frontend/is-external-url-allowed'; @Component({ selector: 'note', @@ -69,10 +70,16 @@ export class NoteComponent implements OnChanges { note!: Note; + // The src auto-loads on render (no click), so a synced remote file:// / + // UNC imgUrl would silently leak the user's NTLM hash. Only a safe URL reaches + // the [src]/[enlargeImg] bindings. See GHSA-hr87-735w-hfq3. + safeImgUrl?: string; + // TODO: Skipped for migration because: // Accessor inputs cannot be migrated as they are too complex. @Input('note') set noteSet(v: Note) { this.note = v; + this.safeImgUrl = isPathSafeToOpen(v?.imgUrl) ? v.imgUrl : undefined; this._note$.next(v); this._updateNoteTxt(); } From 7e6b2df1ade22bbfc7224eb77aa5603f24a85ef2 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Tue, 9 Jun 2026 19:07:15 +0200 Subject: [PATCH 2/7] fix(plugins): load plugin iframe via srcdoc to keep opaque-origin isolation on desktop Dropping allow-same-origin (so the plugin UI iframe runs on an opaque origin and cannot reach window.parent.ea) blanks the UI on packaged file:// builds: a blob: URL inherits the host origin and an opaque-origin iframe cannot fetch it. srcdoc is parsed inline (no cross-origin fetch), so the iframe renders under its opaque origin everywhere. - buildPluginIframeHtml() returns the document string (was createPluginIframeUrl returning a blob: URL); component binds it via [srcdoc] with bypassSecurityTrustHtml (srcdoc is a SecurityContext.HTML sink, raw strings would be sanitized). - remove now-unused blob URL tracking/cleanup. - sandbox stays a static attribute (Angular NG0910). (cherry picked from commit 5efab5d5ac2da5e6e1babf25e7e37d0d965b7559) --- src/app/plugins/plugin-cleanup.service.ts | 4 -- .../plugin-index/plugin-index.component.html | 9 ++-- .../ui/plugin-index/plugin-index.component.ts | 47 ++++++------------- .../plugins/util/plugin-iframe.util.spec.ts | 29 ++++++++++++ src/app/plugins/util/plugin-iframe.util.ts | 44 +++++------------ 5 files changed, 60 insertions(+), 73 deletions(-) diff --git a/src/app/plugins/plugin-cleanup.service.ts b/src/app/plugins/plugin-cleanup.service.ts index a7fcd01aad..d2133c1c02 100644 --- a/src/app/plugins/plugin-cleanup.service.ts +++ b/src/app/plugins/plugin-cleanup.service.ts @@ -1,5 +1,4 @@ import { Injectable } from '@angular/core'; -import { cleanupAllPluginIframeUrls } from './util/plugin-iframe.util'; /** * Simplified cleanup service following KISS principles. @@ -34,8 +33,5 @@ export class PluginCleanupService { cleanupAll(): void { // Just clear all references - let Angular manage iframe DOM lifecycle this._pluginIframes.clear(); - - // Clean up all blob URLs to prevent memory leaks - cleanupAllPluginIframeUrls(); } } diff --git a/src/app/plugins/ui/plugin-index/plugin-index.component.html b/src/app/plugins/ui/plugin-index/plugin-index.component.html index 190111d4df..5e59fa2ea9 100644 --- a/src/app/plugins/ui/plugin-index/plugin-index.component.html +++ b/src/app/plugins/ui/plugin-index/plugin-index.component.html @@ -59,16 +59,19 @@ } - @if (iframeSrc() && !error()) { + @if (iframeSrcdoc() && !error()) {
+ Angular forbids binding security-sensitive iframe attributes (throws NG0910). + No allow-same-origin: the iframe runs on an opaque origin so it cannot reach + window.parent.ea. Loaded via srcdoc (not a blob: URL) so it still renders on + packaged file:// builds, where an opaque-origin blob: fetch would be blocked. -->