Desktop: Security: Disallow UNC file links (#9979)

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.

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) {
+	public async openExternal(url: string) {
-		return require('electron').shell.openExternal(url);
+		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() {

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));

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<Props, State> {
 			if (msg.indexOf('joplin://') === 0) {
 				throw new Error(_('Unsupported link or message: %s', msg));
 			} else if (urlUtils.urlProtocol(msg)) {
-				if (msg.indexOf('file://') === 0) {
+				await bridge().openExternal(msg);
-					void require('electron').shell.openExternal(urlDecode(msg));
-				} else {
-					void require('electron').shell.openExternal(msg);
-				}
 			} else if (msg.indexOf('#') === 0) {
 				// This is an internal anchor, which is handled by the WebView so skip this case
 			} else {

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<Props, State> {
 
 	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}`);
 		}

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 });

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);
+	});
 });

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;