From c853f682871db1951c9674448e398bba01fa2c91 Mon Sep 17 00:00:00 2001 From: nig Date: Tue, 14 Dec 2021 14:34:50 +0100 Subject: [PATCH] [desktop] use electron safeStorage to store device keys * renames PathUtils.swapFilename to replaceLastComponent * provides a new impl for SecretStorage * uses electron.safeStorage to encrypt the device key that then gets stored in a file in app.getPath('userData')/safe_storage/ * from now on, per-user data is encrypted with a per-user key, even for per-machine installs. #3733 close #3676 --- .../error/DeviceStorageUnavailableError.js | 2 +- src/desktop/DesktopMain.js | 5 +- src/desktop/DesktopUtils.js | 8 +- src/desktop/PathUtils.js | 8 +- src/desktop/sse/SecretStorage.js | 113 +++++++++++++++++- test/client/desktop/PathUtilsTest.js | 26 ++-- 6 files changed, 135 insertions(+), 27 deletions(-) diff --git a/src/api/common/error/DeviceStorageUnavailableError.js b/src/api/common/error/DeviceStorageUnavailableError.js index 39098bcd1de5..a59cd29be958 100644 --- a/src/api/common/error/DeviceStorageUnavailableError.js +++ b/src/api/common/error/DeviceStorageUnavailableError.js @@ -4,7 +4,7 @@ import {TutanotaError} from "./TutanotaError" export class DeviceStorageUnavailableError extends TutanotaError { - constructor(msg: string, error: Error) { + constructor(msg: string, error: ?Error) { super("DeviceStorageUnavailableError", error ? (msg + "> " + (error.stack ? error.stack : error.message)) : msg) } } diff --git a/src/desktop/DesktopMain.js b/src/desktop/DesktopMain.js index a220245597a0..675cee8ea742 100644 --- a/src/desktop/DesktopMain.js +++ b/src/desktop/DesktopMain.js @@ -23,8 +23,9 @@ import {DesktopTray} from "./tray/DesktopTray" import {log} from "./DesktopLog"; import {UpdaterWrapperImpl} from "./UpdaterWrapper" import {ElectronNotificationFactory} from "./NotificatonFactory" -import {KeytarSecretStorage} from "./sse/SecretStorage" +import {SafeStorageSecretStorage} from "./sse/SecretStorage" import fs from "fs" +import path from "path" import {DesktopIntegrator, getDesktopIntegratorForPlatform} from "./integration/DesktopIntegrator" import net from "net" import child_process from "child_process" @@ -96,7 +97,7 @@ if (opts.registerAsMailHandler && opts.unregisterAsMailHandler) { async function createComponents(): Promise { lang.init(en) - const secretStorage = new KeytarSecretStorage() + const secretStorage = new SafeStorageSecretStorage(electron, fs, path) const deviceKeyProvider = new DeviceKeyProviderImpl(secretStorage, desktopCrypto) const configMigrator = new DesktopConfigMigrator(desktopCrypto, deviceKeyProvider, electron) const conf = new DesktopConfig(app, configMigrator, deviceKeyProvider, desktopCrypto) diff --git a/src/desktop/DesktopUtils.js b/src/desktop/DesktopUtils.js index df9bd5e111f1..6cba62d8cdae 100644 --- a/src/desktop/DesktopUtils.js +++ b/src/desktop/DesktopUtils.js @@ -7,7 +7,7 @@ import {app} from 'electron' import {defer, delay, noOp, uint8ArrayToHex} from '@tutao/tutanota-utils' import {log} from "./DesktopLog" import {DesktopCryptoFacade} from "./DesktopCryptoFacade" -import {fileExists, swapFilename} from "./PathUtils" +import {fileExists, replaceLastComponent} from "./PathUtils" import url from "url" import {registerKeys, unregisterKeys} from "./reg-templater" @@ -29,7 +29,7 @@ export class DesktopUtils { } checkIsPerUserInstall(): Promise { - const markerPath = swapFilename(process.execPath, "per_user") + const markerPath = replaceLastComponent(process.execPath, "per_user") return fileExists(markerPath) } @@ -217,7 +217,7 @@ export class DesktopUtils { */ _writeToDisk(contents: string): string { const filename = uint8ArrayToHex(this._desktopCrypto.randomBytes(12)) - const filePath = swapFilename(process.execPath, filename) + const filePath = replaceLastComponent(process.execPath, filename) this._fs.writeFileSync(filePath, contents, {encoding: 'utf-8', mode: 0o400}) return filePath } @@ -234,7 +234,7 @@ export class DesktopUtils { // * where to put tmp files (also user-dependent) // all these must be set in the registry const execPath = process.execPath - const dllPath = swapFilename(execPath, 'mapirs.dll') + const dllPath = replaceLastComponent(execPath, 'mapirs.dll') // we may be a per-machine installation that's used by multiple users, so the dll will replace %USERPROFILE% // with the value of the USERPROFILE env var. const appData = path.join("%USERPROFILE%", 'AppData') diff --git a/src/desktop/PathUtils.js b/src/desktop/PathUtils.js index fe71006304ef..02bdd6a85a91 100644 --- a/src/desktop/PathUtils.js +++ b/src/desktop/PathUtils.js @@ -88,12 +88,12 @@ export function urlIsPrefix(prefix: URL, url: URL): boolean { } /** - * replace the last component in a file path with another + * replace the last component in a path with another * @param p path to a file/folder - * @param file the file name to put in the last path component + * @param newLast the name to put in the last path component * @param pathModule path module to use for cross platform testing */ -export function swapFilename(p: string, file: string, pathModule: $Exports<"path"> = path): string { +export function replaceLastComponent(p: string, newLast: string, pathModule: $Exports<"path"> = path): string { const dir = pathModule.dirname(p) - return pathModule.join(dir, file) + return pathModule.join(dir, newLast) } \ No newline at end of file diff --git a/src/desktop/sse/SecretStorage.js b/src/desktop/sse/SecretStorage.js index 107a18909b04..2dcfe293af6f 100644 --- a/src/desktop/sse/SecretStorage.js +++ b/src/desktop/sse/SecretStorage.js @@ -1,6 +1,9 @@ //@flow -import {getPassword, setPassword} from "keytar" +import {deletePassword, getPassword, setPassword} from "keytar" +import type {App, SafeStorage} from "electron" +import {log} from "../DesktopLog" +import {DeviceStorageUnavailableError} from "../../api/common/error/DeviceStorageUnavailableError" export interface SecretStorage { getPassword(service: string, account: string): Promise; @@ -8,12 +11,116 @@ export interface SecretStorage { setPassword(service: string, account: string, password: string): Promise; } -export class KeytarSecretStorage implements SecretStorage { +/** + * @deprecated: will be replaced by SafeStorageSecretStorage + */ +class KeytarSecretStorage implements SecretStorage { + getPassword(service: string, account: string): Promise { return getPassword(service, account) } setPassword(service: string, account: string, password: string): Promise { - return setPassword(service, account, password); + return setPassword(service, account, password) + } + + deletePassword(service: string, account: string): Promise { + return deletePassword(service, account) + } +} + +/** + * Secret Storage impl using the electron 15+ SafeStorage API + * + * Note: the main thread will be blocked while the keychain is being unlocked, + * potentially for as long as the user takes to enter a password. + * We're asking for access before any windows are created, which should prevent + * any weirdness arising from that. + */ +export class SafeStorageSecretStorage implements SecretStorage { + _safeStorage: SafeStorage + _app: App + _fs: $Exports<"fs"> + _path: $Exports<"path"> + _keytarSecretStorage: KeytarSecretStorage + + constructor({safeStorage, app}: $Exports<"electron">, fs: $Exports<"fs">, path: $Exports<"path">) { + this._safeStorage = safeStorage + this._app = app + this._fs = fs + this._path = path + this._keytarSecretStorage = new KeytarSecretStorage() + } + + async getPassword(service: string, account: string): Promise { + await this._assertAvailable() + await this._migrateKeytarPassword(service, account) + const keyPath = this._getKeyPath(service, account) + try { + const encPwBuffer = await this._fs.promises.readFile(keyPath) + const plainPw = this._safeStorage.decryptString(encPwBuffer) + return Promise.resolve(plainPw) + } catch (e) { + if (e.code === "ENOENT") return null + throw e + } + } + + async setPassword(service: string, account: string, password: string): Promise { + await this._assertAvailable() + const keyPath = this._getKeyPath(service, account) + const cypherBuffer = this._safeStorage.encryptString(password) + return this._fs.promises.writeFile(keyPath, cypherBuffer) + } + + _getKeyPath(service: string, account: string): string { + const fname = service.concat("-", account) + const safeStoragePath = this._getSafeStoragePath() + return this._path.join(safeStoragePath, fname) + } + + /** + * this should always be a path inside the user's home directory (or equivalent) + * @private + */ + _getSafeStoragePath(): string { + return this._path.join(this._app.getPath('userData'), "safe_storage") + } + + /** + * ensures that the safe_storage directory exists and that we can use the + * safeStorage API + * @private + */ + async _assertAvailable(): Promise { + await this._fs.promises.mkdir(this._getSafeStoragePath(), {recursive: true}) + // see https://github.com/electron/electron/issues/32206 + // the rest of the safeStorage API should be throwing errors + // we can catch until this works. + if (process.platform === 'linux') return + if (this._safeStorage.isEncryptionAvailable()) return + throw new DeviceStorageUnavailableError("safeStorage API is not available", null) + } + + /** + * most devices will have stored a deviceKey with keytar, which we can move + * to the safeStorage impl. + * + * @private + */ + async _migrateKeytarPassword(service: string, account: string): Promise { + let keytarPw = null + try { + keytarPw = await this._keytarSecretStorage.getPassword(service, account) + } catch (e) { + log.debug("keytar failed, assuming there's no pw stored") + } + if (keytarPw) { + await this.setPassword(service, account, keytarPw) + // do not do this until later. there may be multiple installs using + // the deviceKey if for some reason keytar used a system keychain + // to store it. + // await this._keytarSecretStorage.deletePassword(service, account) + } } } \ No newline at end of file diff --git a/test/client/desktop/PathUtilsTest.js b/test/client/desktop/PathUtilsTest.js index fd5f5f102fbe..c9ce3a56bb0f 100644 --- a/test/client/desktop/PathUtilsTest.js +++ b/test/client/desktop/PathUtilsTest.js @@ -1,6 +1,6 @@ //@flow import o from "ospec" -import {nonClobberingFilename, swapFilename} from "../../../src/desktop/PathUtils" +import {nonClobberingFilename, replaceLastComponent} from "../../../src/desktop/PathUtils" import path from "path" o.spec("PathUtils", function () { @@ -206,21 +206,21 @@ o.spec("PathUtils", function () { o.spec("swapFileName Test", function () { o("replace file with file, posix", function () { - o(swapFilename("/a/b/c.txt", "d.txt")).equals("/a/b/d.txt") - o(swapFilename("a/b/c.txt", "d.txt")).equals("a/b/d.txt") - o(swapFilename("/a/b/c.txt", "d")).equals("/a/b/d") - o(swapFilename("/a/b/c", "d.txt")).equals("/a/b/d.txt") - o(swapFilename("/a/b/c", "d.txt")).equals("/a/b/d.txt") - o(swapFilename("/", "bla.txt")).equals("/bla.txt") + o(replaceLastComponent("/a/b/c.txt", "d.txt")).equals("/a/b/d.txt") + o(replaceLastComponent("a/b/c.txt", "d.txt")).equals("a/b/d.txt") + o(replaceLastComponent("/a/b/c.txt", "d")).equals("/a/b/d") + o(replaceLastComponent("/a/b/c", "d.txt")).equals("/a/b/d.txt") + o(replaceLastComponent("/a/b/c", "d.txt")).equals("/a/b/d.txt") + o(replaceLastComponent("/", "bla.txt")).equals("/bla.txt") }) o("replace file with file, windows", function () { - o(swapFilename("C:\\tmp\\file.html", "text.txt", path.win32)).equals("C:\\tmp\\text.txt") - o(swapFilename("C:\\tmp\\file.html", "text", path.win32)).equals("C:\\tmp\\text") - o(swapFilename("C:\\tmp\\folder\\", "text", path.win32)).equals("C:\\tmp\\text") - o(swapFilename("tmp\\file.html", "text.txt", path.win32)).equals("tmp\\text.txt") - o(swapFilename("tmp", "text.txt", path.win32)).equals("text.txt") - o(swapFilename("C:\\", "text.txt", path.win32)).equals("C:\\text.txt") + o(replaceLastComponent("C:\\tmp\\file.html", "text.txt", path.win32)).equals("C:\\tmp\\text.txt") + o(replaceLastComponent("C:\\tmp\\file.html", "text", path.win32)).equals("C:\\tmp\\text") + o(replaceLastComponent("C:\\tmp\\folder\\", "text", path.win32)).equals("C:\\tmp\\text") + o(replaceLastComponent("tmp\\file.html", "text.txt", path.win32)).equals("tmp\\text.txt") + o(replaceLastComponent("tmp", "text.txt", path.win32)).equals("text.txt") + o(replaceLastComponent("C:\\", "text.txt", path.win32)).equals("C:\\text.txt") }) }) }) \ No newline at end of file