diff --git a/electron/ipc-handlers/system.ts b/electron/ipc-handlers/system.ts index 9984f73e30..a10b529d45 100644 --- a/electron/ipc-handlers/system.ts +++ b/electron/ipc-handlers/system.ts @@ -1,10 +1,30 @@ import { app, dialog, ipcMain, shell } from 'electron'; import { IPC } from '../shared-with-frontend/ipc-events.const'; +import { + isExternalUrlSchemeAllowed, + isPathSafeToOpen, +} from '../shared-with-frontend/is-external-url-allowed'; import { getWin } from '../main-window'; export const initSystemIpc = (): void => { - ipcMain.on(IPC.OPEN_PATH, (ev, path: string) => shell.openPath(path)); - ipcMain.on(IPC.OPEN_EXTERNAL, (ev, url: string) => shell.openExternal(url)); + ipcMain.on(IPC.OPEN_PATH, (ev, path: string) => { + // Block UNC / network paths and remote file:// URLs: shell.openPath would + // resolve \\host\share (and file://host/share) to an SMB connection and leak + // the user's NTLM hash. FILE-type task-attachment paths are synced and thus + // attacker-controllable. See GHSA-hr87-735w-hfq3. + if (!isPathSafeToOpen(path)) { + return; + } + shell.openPath(path); + }); + ipcMain.on(IPC.OPEN_EXTERNAL, (ev, url: string) => { + // Defense in depth: never hand an unsafe scheme to the OS handler, even if + // the renderer-side guard is bypassed. See GHSA-hr87-735w-hfq3. + if (!isExternalUrlSchemeAllowed(url)) { + return; + } + shell.openExternal(url); + }); ipcMain.on( IPC.SHOW_EMOJI_PANEL, diff --git a/electron/main-window.ts b/electron/main-window.ts index 42a09cb1ba..8543d7da65 100644 --- a/electron/main-window.ts +++ b/electron/main-window.ts @@ -13,6 +13,7 @@ import { errorHandlerWithFrontendInform } from './error-handler-with-frontend-in import * as path from 'path'; import { join, normalize } from 'path'; import { IPC } from './shared-with-frontend/ipc-events.const'; +import { isExternalUrlSchemeAllowed } from './shared-with-frontend/is-external-url-allowed'; import { readFileSync, stat, writeFileSync } from 'fs'; import { error, log } from 'electron-log/main'; import { IS_MAC, IS_GNOME_DESKTOP } from './common.const'; @@ -414,6 +415,14 @@ export const setWasMaximizedBeforeHide = (value: boolean): void => { // eslint-disable-next-line prefer-arrow/prefer-arrow-functions function initWinEventListeners(app: Electron.App): void { const openUrlInBrowser = (url: string): void => { + // Defense in depth: never hand an unsafe scheme to the OS handler, even if + // a renderer-side guard is bypassed (e.g. a link click that falls through + // to navigation rather than the explicit openExternalUrl IPC). The blocked + // schemes are OS protocol handlers / UNC paths. See GHSA-hr87-735w-hfq3. + if (!isExternalUrlSchemeAllowed(url)) { + error('Refused to open URL with disallowed scheme via openExternal'); + return; + } // needed for mac; especially for jira urls we might have a host like this www.host.de// const urlObj = new URL(url); urlObj.pathname = urlObj.pathname.replace('//', '/'); diff --git a/electron/shared-with-frontend/is-external-url-allowed.ts b/electron/shared-with-frontend/is-external-url-allowed.ts new file mode 100644 index 0000000000..0f071792af --- /dev/null +++ b/electron/shared-with-frontend/is-external-url-allowed.ts @@ -0,0 +1,96 @@ +/** + * URL schemes that are permitted to reach the OS handler via shell.openExternal. + * + * Task notes render Markdown links whose href would otherwise be passed verbatim + * to the OS handler on click. Without this gate, anyone who can populate note + * content (multi-device sync, shared/imported backups, issue-provider content) + * could make a single click silently invoke any OS-registered protocol — + * `ms-msdt:`, `search-ms:`, etc. + * See GHSA-hr87-735w-hfq3. + * + * Shared between the Angular renderer (link rendering) and the Electron main + * process (shell.openExternal call sites) so both layers enforce one policy. + */ +export const ALLOWED_EXTERNAL_URL_SCHEMES = ['http:', 'https:', 'mailto:', 'file:']; + +const _isSlash = (char: string): boolean => char === '/' || char === '\\'; + +/** + * True for a UNC / network path such as `\\host\share` or `//host/share` — i.e. + * any path whose first two characters are slashes (in either direction). + * + * Opening such a path (`shell.openPath` / `shell.openExternal('file://host/…')`) + * makes the OS reach out to a remote SMB host, leaking the user's NTLM hash. + * Local absolute paths (`/home/x`, `C:\x`) and POSIX/Windows roots are NOT UNC. + */ +export const isUncPath = (path: unknown): boolean => { + if (typeof path !== 'string') { + return false; + } + const trimmed = path.trim(); + return trimmed.length >= 2 && _isSlash(trimmed[0]) && _isSlash(trimmed[1]); +}; + +/** + * Returns true only for URLs whose scheme is in ALLOWED_EXTERNAL_URL_SCHEMES. + * Schemeless/relative input and any string that fails URL parsing are rejected. + * `file:` is restricted to LOCAL files — a `file:` URL with a remote authority + * (`file://host/share`, or a path-based UNC like `file:////host/share`) is the + * same NTLM-hash-leak vector as a raw `\\host\share` path and is rejected. + */ +export const isExternalUrlSchemeAllowed = (url: unknown): boolean => { + if (typeof url !== 'string') { + return false; + } + const trimmed = url.trim(); + if (!trimmed) { + return false; + } + let parsed: URL; + try { + // Raw UNC paths (`\\host\share`, `//host`) and schemeless input throw here. + parsed = new URL(trimmed); + } catch { + return false; + } + // `protocol` is always lower-cased by the WHATWG URL parser. + if (!ALLOWED_EXTERNAL_URL_SCHEMES.includes(parsed.protocol)) { + return false; + } + if (parsed.protocol === 'file:') { + // Allow ONLY the canonical local form `file:///`. Anything with an + // authority (`file://host/…`) or a path-based UNC (`file:////host`) is a + // remote SMB reference and leaks the user's NTLM hash. Gate on the RAW + // string: Chromium (renderer) and Node (main) parse file: hosts/paths + // differently, so `parsed.host` / `parsed.pathname` are not portable. + const lower = trimmed.toLowerCase(); + return lower.startsWith('file:///') && lower[8] !== '/' && lower[8] !== '\\'; + } + return true; +}; + +/** + * Returns true if a value is safe to hand to `shell.openPath` (which opens a + * filesystem path, not a URL). Rejects UNC paths AND `file:`-scheme values with + * a remote authority — `shell.openPath('file://host/share')` resolves to + * `\\host\share` on Windows, the same NTLM-leak `isUncPath` blocks for the raw + * form. Plain local paths (`/home/x`, `C:\x`, `./rel`) are allowed. + * See GHSA-hr87-735w-hfq3. + */ +export const isPathSafeToOpen = (path: unknown): boolean => { + if (typeof path !== 'string') { + return false; + } + const trimmed = path.trim(); + if (!trimmed) { + return false; + } + if (isUncPath(trimmed)) { + return false; + } + // A file:-scheme value must be a LOCAL file URL, not a remote authority. + if (trimmed.toLowerCase().startsWith('file:')) { + return isExternalUrlSchemeAllowed(trimmed); + } + return true; +}; diff --git a/src/app/features/tasks/task-attachment/task-attachment-list/task-attachment-list.component.spec.ts b/src/app/features/tasks/task-attachment/task-attachment-list/task-attachment-list.component.spec.ts index 825078023b..8dcfea509e 100644 --- a/src/app/features/tasks/task-attachment/task-attachment-list/task-attachment-list.component.spec.ts +++ b/src/app/features/tasks/task-attachment/task-attachment-list/task-attachment-list.component.spec.ts @@ -40,6 +40,38 @@ describe('TaskAttachmentListComponent', () => { snackService = TestBed.inject(SnackService) as jasmine.SpyObj; }); + describe('resolvedAttachments img src safety (GHSA-hr87-735w-hfq3)', () => { + const imgAttachment = (path: string): TaskAttachment => ({ + id: 'a', + type: 'IMG', + title: 'x', + path, + }); + + it('drops remote file:// / UNC src so the cannot auto-load and leak NTLM', () => { + [ + 'file://192.168.1.100/share/pixel.png', + 'file:////host/share/pixel.png', + '\\\\host\\share\\pixel.png', + '//host/share/pixel.png', + ].forEach((path) => { + fixture.componentRef.setInput('attachments', [imgAttachment(path)]); + expect(component.resolvedAttachments()[0].resolvedOriginalPath).toBeUndefined(); + }); + }); + + it('keeps safe srcs (local file://, http(s), data:)', () => { + [ + 'file:///home/user/img.png', + 'https://example.com/img.png', + 'data:image/png;base64,iVBORw0KGgo=', + ].forEach((path) => { + fixture.componentRef.setInput('attachments', [imgAttachment(path)]); + expect(component.resolvedAttachments()[0].resolvedOriginalPath).toBe(path); + }); + }); + }); + describe('copy', () => { let attachment: TaskAttachment; let originalClipboardDescriptor: PropertyDescriptor | undefined; diff --git a/src/app/features/tasks/task-attachment/task-attachment-list/task-attachment-list.component.ts b/src/app/features/tasks/task-attachment/task-attachment-list/task-attachment-list.component.ts index 9ed09128ea..0507d0361e 100644 --- a/src/app/features/tasks/task-attachment/task-attachment-list/task-attachment-list.component.ts +++ b/src/app/features/tasks/task-attachment/task-attachment-list/task-attachment-list.component.ts @@ -19,6 +19,7 @@ import { MatIcon } from '@angular/material/icon'; import { EnlargeImgDirective } from '../../../../ui/enlarge-img/enlarge-img.directive'; import { MatAnchor, MatButton } from '@angular/material/button'; import { ClipboardImageService } from '../../../../core/clipboard-image/clipboard-image.service'; +import { isPathSafeToOpen } from '../../../../../../electron/shared-with-frontend/is-external-url-allowed'; interface ResolvedAttachment extends TaskAttachment { resolvedPath?: string; @@ -66,9 +67,15 @@ export class TaskAttachmentListComponent { : att.path; const imgPath = att.originalImgPath || att.path; - const resolvedOriginalPath = imgPath?.startsWith('indexeddb://clipboard-images/') + const rawOriginalPath = imgPath?.startsWith('indexeddb://clipboard-images/') ? urlMap.get(imgPath) || imgPath : imgPath; + // The src auto-loads on render (no click), so a synced remote + // file://host / UNC path would silently leak the user's NTLM hash. Drop + // such srcs so they never reach the binding. See GHSA-hr87-735w-hfq3. + const resolvedOriginalPath = isPathSafeToOpen(rawOriginalPath) + ? rawOriginalPath + : undefined; const isLoading = att.path?.startsWith('indexeddb://clipboard-images/') && diff --git a/src/app/ui/is-external-url-allowed.spec.ts b/src/app/ui/is-external-url-allowed.spec.ts new file mode 100644 index 0000000000..7e80b31852 --- /dev/null +++ b/src/app/ui/is-external-url-allowed.spec.ts @@ -0,0 +1,125 @@ +import { + ALLOWED_EXTERNAL_URL_SCHEMES, + isExternalUrlSchemeAllowed, + isPathSafeToOpen, + isUncPath, +} from '../../../electron/shared-with-frontend/is-external-url-allowed'; + +describe('isExternalUrlSchemeAllowed', () => { + describe('allowed schemes', () => { + const allowed = [ + 'http://example.com', + 'https://example.com/path?q=1#frag', + 'HTTPS://EXAMPLE.COM', // scheme is case-insensitive + 'mailto:someone@example.com', + 'file:///home/user/notes.txt', + ' https://example.com ', // surrounding whitespace tolerated + ]; + allowed.forEach((url) => { + it(`allows "${url}"`, () => { + expect(isExternalUrlSchemeAllowed(url)).toBe(true); + }); + }); + + it('keeps the allowlist in sync with expectations', () => { + expect(ALLOWED_EXTERNAL_URL_SCHEMES).toEqual([ + 'http:', + 'https:', + 'mailto:', + 'file:', + ]); + }); + }); + + describe('blocked schemes (GHSA-hr87-735w-hfq3)', () => { + const blocked = [ + 'ms-calculator:', + 'ms-msdt:/id PCWDiagnostic', // Follina (CVE-2022-30190) + 'search-ms:query=foo', + 'ms-officecmd:{}', + 'javascript:alert(1)', + 'data:text/html,', + 'vbscript:msgbox(1)', + 'ftp://example.com', + 'ssh://example.com', + 'tel:+123456789', + '\\\\192.168.1.100\\share', // UNC / SMB — NTLM hash capture + '/\\192.168.1.100\\share', + // file: with a remote authority is the same SMB / NTLM-leak vector and + // must be blocked even though `file:` is allow-listed for local files. + 'file://192.168.1.100/share/x', + 'file:////host/share', // path-based UNC, empty host + 'file://///host/share', + 'file:\\\\host\\share', + 'FILE://HOST/share', // case-insensitive + ]; + blocked.forEach((url) => { + it(`blocks "${url}"`, () => { + expect(isExternalUrlSchemeAllowed(url)).toBe(false); + }); + }); + + it('still allows LOCAL file: URLs (Windows drive + POSIX paths)', () => { + expect(isExternalUrlSchemeAllowed('file:///home/user/notes.txt')).toBe(true); + expect(isExternalUrlSchemeAllowed('file:///C:/Users/me/doc.pdf')).toBe(true); + }); + }); + + describe('isUncPath', () => { + it('flags UNC / network paths', () => { + ['\\\\host\\share', '//host/share', '/\\host', '\\/host', ' \\\\host\\s'].forEach( + (p) => expect(isUncPath(p)).toBe(true), + ); + }); + + it('does not flag local absolute paths or non-strings', () => { + ['/home/user/x', 'C:\\Users\\me', './rel', '', '/', 'x', undefined, null].forEach( + (p) => expect(isUncPath(p as unknown as string)).toBe(false), + ); + }); + }); + + describe('isPathSafeToOpen (shell.openPath gate)', () => { + it('allows local filesystem paths and local file:// URLs', () => { + [ + '/home/user/doc.pdf', + 'C:\\Users\\me\\doc.pdf', + './rel/x', + 'file:///home/x', + ].forEach((p) => expect(isPathSafeToOpen(p)).toBe(true)); + }); + + it('blocks UNC paths AND remote file:// URLs (NTLM leak via shell.openPath)', () => { + [ + '\\\\host\\share', + '//host/share', + 'file://host/share', + 'file://192.168.1.100/share/x', + 'file:////host/share', + '', + undefined, + null, + ].forEach((p) => expect(isPathSafeToOpen(p as unknown as string)).toBe(false)); + }); + }); + + describe('malformed / non-string input', () => { + it('blocks empty and whitespace-only strings', () => { + expect(isExternalUrlSchemeAllowed('')).toBe(false); + expect(isExternalUrlSchemeAllowed(' ')).toBe(false); + }); + + it('blocks schemeless / relative input', () => { + expect(isExternalUrlSchemeAllowed('example.com')).toBe(false); + expect(isExternalUrlSchemeAllowed('//example.com')).toBe(false); + expect(isExternalUrlSchemeAllowed('./relative/path')).toBe(false); + }); + + it('blocks non-string input', () => { + expect(isExternalUrlSchemeAllowed(undefined)).toBe(false); + expect(isExternalUrlSchemeAllowed(null)).toBe(false); + expect(isExternalUrlSchemeAllowed(42)).toBe(false); + expect(isExternalUrlSchemeAllowed({ href: 'https://example.com' })).toBe(false); + }); + }); +}); diff --git a/src/app/ui/marked-options-factory.spec.ts b/src/app/ui/marked-options-factory.spec.ts index c7d3a9d679..0b9d5eff70 100644 --- a/src/app/ui/marked-options-factory.spec.ts +++ b/src/app/ui/marked-options-factory.spec.ts @@ -352,6 +352,57 @@ describe('markedOptionsFactory', () => { expect(result).toContain('href="http://x" onmouseover="alert(1)"'); expect(result).toContain('title="" onfocus="alert(2)"'); }); + + // GHSA-hr87-735w-hfq3: the rendered href is passed verbatim to + // shell.openExternal on click, so unsafe schemes must never become anchors. + describe('unsafe URL schemes (GHSA-hr87-735w-hfq3)', () => { + const mockParser = { + parseInline: (tokens: any[]) => + tokens.map((t: any) => t.raw || t.text || '').join(''), + }; + + ['ms-calculator:', 'javascript:alert(1)', 'ssh://h', '\\\\host\\share'].forEach( + (href) => { + it(`renders "${href}" as inert text, not an anchor`, () => { + const linkRenderer = options.renderer!.link.bind({ parser: mockParser }); + const result = linkRenderer({ + href, + title: 'x', + tokens: [{ type: 'text', raw: 'Click here', text: 'Click here' }], + } as any); + expect(result).not.toContain(' { + const linkRenderer = options.renderer!.link.bind({ parser: mockParser }); + ['mailto:a@b.com', 'file:///tmp/x', 'https://example.com'].forEach((href) => { + const result = linkRenderer({ + href, + title: '', + tokens: [{ type: 'text', raw: 'L', text: 'L' }], + } as any); + expect(result).toContain(`href="${href}"`); + }); + }); + + it('escapes a quote-injection href so it cannot break out of the attribute', () => { + const linkRenderer = options.renderer!.link.bind({ parser: mockParser }); + const result = linkRenderer({ + href: 'https://example.com/" onmouseover="alert(1)', + title: 'a"b', + tokens: [{ type: 'text', raw: 'L', text: 'L' }], + } as any); + expect(result).not.toContain('onmouseover="alert(1)"'); + expect(result).toContain('"'); + expect(result).toContain( + 'href="https://example.com/" onmouseover="alert(1)"', + ); + }); + }); }); describe('image renderer', () => { @@ -472,6 +523,56 @@ describe('markedOptionsFactory', () => { ); }); }); + + // GHSA-hr87-735w-hfq3: an image src auto-loads on render (no click), so a + // remote file:// / UNC src would silently leak the user's NTLM hash. Such + // srcs must never reach the `src` attribute. + describe('unsafe image src (GHSA-hr87-735w-hfq3)', () => { + [ + 'file://192.168.1.100/share/pixel.png', + 'file:////host/share/pixel.png', + '\\\\host\\share\\pixel.png', + '//host/share/pixel.png', + ].forEach((href) => { + it(`blocks remote/UNC src "${href}" (renders no )`, () => { + const result = options.renderer!.image({ + href, + title: null, + text: 'Alt text', + } as any); + expect(result).not.toContain(' { + [ + 'file:///home/user/img.png', + 'http://example.com/img.png', + 'https://example.com/img.png', + 'data:image/png;base64,iVBORw0KGgo=', + 'blob:https://example.com/abc', + ].forEach((href) => { + const result = options.renderer!.image({ + href, + title: null, + text: 'L', + } as any); + expect(result).toContain(`src="${href}"`); + }); + }); + + it('escapes the blocked-image alt text so it cannot inject markup', () => { + const result = options.renderer!.image({ + href: 'file://host/share/x.png', + title: null, + text: '', + } as any); + expect(result).not.toContain(' { diff --git a/src/app/ui/marked-options-factory.ts b/src/app/ui/marked-options-factory.ts index 21c95d26c4..7bd318b270 100644 --- a/src/app/ui/marked-options-factory.ts +++ b/src/app/ui/marked-options-factory.ts @@ -1,14 +1,15 @@ import { MarkedOptions, MarkedRenderer } from 'ngx-markdown'; import { Hooks, Token } from 'marked'; +import { + isExternalUrlSchemeAllowed, + isPathSafeToOpen, +} from '../../../electron/shared-with-frontend/is-external-url-allowed'; /** * Escape a string for safe interpolation into a double-quoted HTML attribute. * - * Defense-in-depth: the note render surfaces run this renderer's output through - * Angular's SecurityContext.HTML sanitizer (they no longer set disableSanitizer), - * which already strips event-handler attributes. Escaping here additionally keeps - * the raw renderer output non-injectable on its own, so a future sanitizer bypass - * can't turn an attacker-controlled image href/title/alt into a new attribute. + * Defense-in-depth: note renders are sanitized as HTML before display, but the + * raw renderer output still must not be attribute-injectable on its own. * `&` must be replaced first. */ export const escapeHtmlAttr = (value: string): string => @@ -117,10 +118,12 @@ export const markedOptionsFactory = (): MarkedOptions => { tokens: Token[]; }) { const text = tokens ? this.parser.parseInline(tokens) : ''; - // Escape href/title for parity with renderer.image — the raw renderer output - // must not be attribute-injectable on its own (text is already-parsed inline - // HTML, so it is not re-escaped). rel="noopener noreferrer" matches the - // hardening in render-links.pipe.ts and avoids reverse-tabnabbing. + // Block unsafe URL schemes from rendering as clickable links. On click the + // href is passed verbatim to shell.openExternal (Electron), which would let + // note content silently invoke OS protocol handlers. See GHSA-hr87-735w-hfq3. + if (!isExternalUrlSchemeAllowed(href)) { + return `${text}`; + } return `${text}`; }; @@ -136,6 +139,17 @@ export const markedOptionsFactory = (): MarkedOptions => { title: string | null; text: string; }) => { + // Unlike links, an image src auto-loads on render (no click). A remote + // `file://host/share` or UNC src would silently make the OS open an SMB + // connection and leak the user's NTLM hash just by viewing the note, so + // such srcs must never reach the `src` attribute. Remote web images + // (http/https/data/blob) are unaffected. See GHSA-hr87-735w-hfq3. + if (!isPathSafeToOpen(href)) { + return `${escapeHtmlAttr( + text || '', + )}`; + } + const { width, height } = parseImageDimensionsFromTitle(title); // Build width and height attributes (not style, as Angular sanitizer strips inline styles) diff --git a/src/styles/components/markdown.scss b/src/styles/components/markdown.scss index 3501f76efb..a12f314c1f 100644 --- a/src/styles/components/markdown.scss +++ b/src/styles/components/markdown.scss @@ -136,4 +136,16 @@ .checkbox-label { cursor: pointer; } + + // A markdown link with a disallowed URL scheme, or an image with an unsafe + // (remote file:// / UNC) src, renders as inert text instead of an anchor/img + // (security: GHSA-hr87-735w-hfq3, see marked-options-factory.ts). Style it as + // muted, non-interactive text with a help cursor so blocked content reads as + // intentionally disabled, not body copy (the title attribute explains why on + // hover). + .markdown-blocked-link { + color: var(--text-color-muted); + text-decoration: underline dotted; + cursor: help; + } }