From 836e23c08219f53850b451168c1a77c6e9e864b4 Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Thu, 22 Feb 2024 13:29:16 -0800 Subject: [PATCH] Desktop: Security: Disallow UNC file links (#9979) --- packages/app-desktop/app.ts | 6 ++- packages/app-desktop/bridge.ts | 48 ++++++++++++++++--- .../gui/MainScreen/commands/openItem.ts | 6 +-- .../app-desktop/gui/NoteRevisionViewer.tsx | 7 +-- packages/app-desktop/gui/ResourceScreen.tsx | 4 +- .../lib/services/ExternalEditWatcher/utils.ts | 2 +- packages/utils/path.test.ts | 20 +++++++- packages/utils/path.ts | 7 +++ 8 files changed, 80 insertions(+), 20 deletions(-) diff --git a/packages/app-desktop/app.ts b/packages/app-desktop/app.ts index 2c2c621b2..23d7635b4 100644 --- a/packages/app-desktop/app.ts +++ b/packages/app-desktop/app.ts @@ -586,7 +586,11 @@ class Application extends BaseApplication { ExternalEditWatcher.instance().setLogger(reg.logger()); ExternalEditWatcher.instance().initialize(bridge, this.store().dispatch); - ResourceEditWatcher.instance().initialize(reg.logger(), (action: any) => { this.store().dispatch(action); }, (path: string) => bridge().openItem(path)); + ResourceEditWatcher.instance().initialize( + reg.logger(), + (action: any) => { this.store().dispatch(action); }, + (path: string) => bridge().openItem(path), + ); // Forwards the local event to the global event manager, so that it can // be picked up by the plugin manager. diff --git a/packages/app-desktop/bridge.ts b/packages/app-desktop/bridge.ts index da33b2938..f31bc970e 100644 --- a/packages/app-desktop/bridge.ts +++ b/packages/app-desktop/bridge.ts @@ -1,12 +1,15 @@ import ElectronAppWrapper from './ElectronAppWrapper'; import shim from '@joplin/lib/shim'; import { _, setLocale } from '@joplin/lib/locale'; -import { BrowserWindow, nativeTheme, nativeImage } from 'electron'; +import { BrowserWindow, nativeTheme, nativeImage, shell } from 'electron'; +import { dirname, isUncPath, toSystemSlashes } from '@joplin/lib/path-utils'; +import { fileUriToPath } from '@joplin/utils/url'; +import { urlDecode } from '@joplin/lib/string-utils'; import * as Sentry from '@sentry/electron/main'; -import { writeFileSync } from 'fs'; import { homedir } from 'os'; import { msleep } from '@joplin/utils/time'; -const { dirname, toSystemSlashes } = require('@joplin/lib/path-utils'); +import { pathExists, writeFileSync } from 'fs-extra'; +import { normalize } from 'path'; interface LastSelectedPath { file: string; @@ -81,6 +84,11 @@ export class Bridge { return this.rootProfileDir_; } + private logWarning(...message: string[]) { + // eslint-disable-next-line no-console + console.warn('bridge:', ...message); + } + public electronApp() { return this.electronWrapper_; } @@ -89,6 +97,14 @@ export class Bridge { return !this.electronApp().electronApp().isPackaged; } + public get autoUploadCrashDumps() { + return this.autoUploadCrashDumps_; + } + + public set autoUploadCrashDumps(v: boolean) { + this.autoUploadCrashDumps_ = v; + } + public async captureException(error: any) { Sentry.captureException(error); // We wait to give the "beforeSend" event handler time to process the crash dump and write @@ -300,12 +316,32 @@ export class Bridge { return require('electron').MenuItem; } - public openExternal(url: string) { - return require('electron').shell.openExternal(url); + public async openExternal(url: string) { + const protocol = new URL(url).protocol; + + if (protocol === 'file:') { + await this.openItem(url); + } else { + return shell.openExternal(url); + } } public async openItem(fullPath: string) { - return require('electron').shell.openPath(toSystemSlashes(fullPath)); + if (fullPath.startsWith('file:/')) { + fullPath = fileUriToPath(urlDecode(fullPath), shim.platformName()); + } + fullPath = normalize(fullPath); + // On Windows, \\example.com\... links can map to network drives. Opening files on these + // drives can lead to arbitrary remote code execution. + const isUntrustedUncPath = isUncPath(fullPath); + if (isUntrustedUncPath) { + this.logWarning(`Not opening external file link: ${fullPath} -- it starts with two \\s, so could be to a network drive.`); + return 'Refusing to open file on a network drive.'; + } else if (await pathExists(fullPath)) { + return shell.openPath(fullPath); + } else { + return 'Path does not exist.'; + } } public screen() { diff --git a/packages/app-desktop/gui/MainScreen/commands/openItem.ts b/packages/app-desktop/gui/MainScreen/commands/openItem.ts index 1ac94a2be..a9411c8bc 100644 --- a/packages/app-desktop/gui/MainScreen/commands/openItem.ts +++ b/packages/app-desktop/gui/MainScreen/commands/openItem.ts @@ -22,7 +22,7 @@ export const runtime = (): CommandRuntime => { const { itemId, hash } = parsedUrl; await openItemById(itemId, context.dispatch, hash); } else { - void require('electron').shell.openExternal(link); + void bridge().openExternal(link); } } else if (urlProtocol(link)) { if (link.indexOf('file://') === 0) { @@ -33,9 +33,9 @@ export const runtime = (): CommandRuntime => { // but doesn't on macOS, so we need to convert it to a path // before passing it to openPath. const decodedPath = fileUriToPath(urlDecode(link), shim.platformName()); - void require('electron').shell.openPath(decodedPath); + void bridge().openItem(decodedPath); } else { - void require('electron').shell.openExternal(link); + void bridge().openExternal(link); } } else { bridge().showErrorMessageBox(_('Unsupported link or message: %s', link)); diff --git a/packages/app-desktop/gui/NoteRevisionViewer.tsx b/packages/app-desktop/gui/NoteRevisionViewer.tsx index 8eef140ab..47e5b7f21 100644 --- a/packages/app-desktop/gui/NoteRevisionViewer.tsx +++ b/packages/app-desktop/gui/NoteRevisionViewer.tsx @@ -15,7 +15,6 @@ import { NoteEntity, RevisionEntity } from '@joplin/lib/services/database/types' import { AppState } from '../app.reducer'; const urlUtils = require('@joplin/lib/urlUtils'); const ReactTooltip = require('react-tooltip'); -const { urlDecode } = require('@joplin/lib/string-utils'); const { connect } = require('react-redux'); import shared from '@joplin/lib/components/shared/note-screen-shared'; @@ -169,11 +168,7 @@ class NoteRevisionViewerComponent extends React.PureComponent { if (msg.indexOf('joplin://') === 0) { throw new Error(_('Unsupported link or message: %s', msg)); } else if (urlUtils.urlProtocol(msg)) { - if (msg.indexOf('file://') === 0) { - void require('electron').shell.openExternal(urlDecode(msg)); - } else { - void require('electron').shell.openExternal(msg); - } + await bridge().openExternal(msg); } else if (msg.indexOf('#') === 0) { // This is an internal anchor, which is handled by the WebView so skip this case } else { diff --git a/packages/app-desktop/gui/ResourceScreen.tsx b/packages/app-desktop/gui/ResourceScreen.tsx index 35a8db9e3..27cdcb33d 100644 --- a/packages/app-desktop/gui/ResourceScreen.tsx +++ b/packages/app-desktop/gui/ResourceScreen.tsx @@ -4,7 +4,7 @@ import { _ } from '@joplin/lib/locale'; const { connect } = require('react-redux'); const { themeStyle } = require('@joplin/lib/theme'); -const bridge = require('@electron/remote').require('./bridge').default; +import bridge from '../services/bridge'; const prettyBytes = require('pretty-bytes'); import Resource from '@joplin/lib/models/Resource'; @@ -187,7 +187,7 @@ class ResourceScreenComponent extends React.Component { public openResource(resource: InnerResource) { const resourcePath = Resource.fullPath(resource); - const ok = bridge().openExternal(`file://${resourcePath}`); + const ok = bridge().openItem(resourcePath); if (!ok) { bridge().showErrorMessageBox(`This file could not be opened: ${resourcePath}`); } diff --git a/packages/lib/services/ExternalEditWatcher/utils.ts b/packages/lib/services/ExternalEditWatcher/utils.ts index a0ecb4141..283ca60dd 100644 --- a/packages/lib/services/ExternalEditWatcher/utils.ts +++ b/packages/lib/services/ExternalEditWatcher/utils.ts @@ -74,7 +74,7 @@ const textEditorCommand = () => { export const openFileWithExternalEditor = async (filePath: string, bridge: any) => { const cmd = textEditorCommand(); if (!cmd) { - bridge.openExternal(`file://${filePath}`); + bridge.openItem(filePath); } else { cmd.args.push(filePath); await spawnCommand(cmd.path, cmd.args, { detached: true }); diff --git a/packages/utils/path.test.ts b/packages/utils/path.test.ts index b58a3bb48..eeaf831a3 100644 --- a/packages/utils/path.test.ts +++ b/packages/utils/path.test.ts @@ -1,4 +1,4 @@ -import { extractExecutablePath, quotePath, toFileProtocolPath, unquotePath } from './path'; +import { extractExecutablePath, isUncPath, quotePath, toFileProtocolPath, unquotePath } from './path'; describe('path', () => { it('should quote and unquote paths', (async () => { @@ -55,4 +55,22 @@ describe('path', () => { expect(toFileProtocolPath(t[0], 'linux')).toBe(t[1]); } })); + + test.each([ + ['./a.txt', 'win32', false], + ['./b.txt', 'win32', false], + ['/home/foo/bar/baz', 'win32', false], + ['./a.txt', 'posix', false], + ['./b.txt', 'posix', false], + ['/home/foo/bar/baz', 'posix', false], + + ['//LOCALHOST/', 'win32', true], + [' //LOCALHOST/', 'win32', true], + [' //example.com/a/b/c', 'win32', true], + ['//LOCALHOST/', 'posix', false], + [' //example.com/a/b/c', 'posix', false], + ['\\\\LOCALHOST/', 'win32', true], + ])('should correctly detect UNC paths', (path, os, expected) => { + expect(isUncPath(path, os)).toBe(expected); + }); }); diff --git a/packages/utils/path.ts b/packages/utils/path.ts index d6eccd579..fc10aeb22 100644 --- a/packages/utils/path.ts +++ b/packages/utils/path.ts @@ -94,6 +94,13 @@ export function trimSlashes(path: string): string { return ltrimSlashes(rtrimSlashes(path)); } +// UNC paths can point to network drives and thus can be dangerous to open +// on some Windows devices. +// See https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-dtyp/62e862f4-2a51-452e-8eeb-dc4ff5ee33cc +export const isUncPath = (path: string, os: string|null = null) => { + return toSystemSlashes(path.trim(), os).startsWith('\\\\'); +}; + export function quotePath(path: string) { if (!path) return ''; if (path.indexOf('"') < 0 && path.indexOf(' ') < 0) return path;