From a87e4e4c29cf0a25687ecae93982d6df7dfae137 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Mon, 9 May 2022 19:08:53 +0530 Subject: [PATCH] fix: fix a crash in `safeStorage` on Linux (#33913) On Linux, `isEncryptionAvailable()` was crashing instead of returning a boolean before the 'ready' event was emitted by the app. The reason of the crash is that [`CreateKeyStorage()`](https://source.chromium.org/chromium/chromium/src/+/main:components/os_crypt/os_crypt_linux.cc;l=74;drc=35be6215ec8f09e50176f36753c68f26c63d1885;bpv=1;bpt=0) expects the config to be set but the function responsible for setting the config, [`SetConfig()`](https://source.chromium.org/chromium/chromium/src/+/main:components/os_crypt/os_crypt_linux.cc;l=237;drc=35be6215ec8f09e50176f36753c68f26c63d1885;bpv=1;bpt=0), is called only after the app is ready inside [`PostCreateMainMessageLoop()`](https://github.com/electron/electron/blob/main/shell/browser/electron_browser_main_parts.cc#L499). So this changes `IsEncryptionAvailable()` to return `false` when the app is not ready on Linux and uses that instead of the raw API in other places like `EncryptString()` and `DecryptString()`. Fixes: https://github.com/electron/electron/issues/32206 Signed-off-by: Darshan Sen --- docs/api/safe-storage.md | 4 +- .../browser/api/electron_api_safe_storage.cc | 21 +++++++++- spec-main/api-safe-storage-spec.ts | 19 +++++++++ .../crash-cases/safe-storage/index.js | 39 +++++++++++++++++++ 4 files changed, 79 insertions(+), 4 deletions(-) create mode 100644 spec-main/fixtures/crash-cases/safe-storage/index.js diff --git a/docs/api/safe-storage.md b/docs/api/safe-storage.md index 564419be98446..590c3496c1ae6 100644 --- a/docs/api/safe-storage.md +++ b/docs/api/safe-storage.md @@ -18,8 +18,8 @@ The `safeStorage` module has the following methods: Returns `Boolean` - Whether encryption is available. -On Linux, returns true if the secret key is -available. On MacOS, returns true if Keychain is available. +On Linux, returns true if the app has emitted the `ready` event and the secret key is available. +On MacOS, returns true if Keychain is available. On Windows, returns true once the app has emitted the `ready` event. ### `safeStorage.encryptString(plainText)` diff --git a/shell/browser/api/electron_api_safe_storage.cc b/shell/browser/api/electron_api_safe_storage.cc index ba1396cf28332..d0e3a73bcdb0d 100644 --- a/shell/browser/api/electron_api_safe_storage.cc +++ b/shell/browser/api/electron_api_safe_storage.cc @@ -31,12 +31,24 @@ void SetElectronCryptoReady(bool ready) { #endif bool IsEncryptionAvailable() { +#if BUILDFLAG(IS_LINUX) + // Calling IsEncryptionAvailable() before the app is ready results in a crash + // on Linux. + // Refs: https://github.com/electron/electron/issues/32206. + if (!Browser::Get()->is_ready()) + return false; +#endif return OSCrypt::IsEncryptionAvailable(); } v8::Local EncryptString(v8::Isolate* isolate, const std::string& plaintext) { - if (!OSCrypt::IsEncryptionAvailable()) { + if (!IsEncryptionAvailable()) { + if (!Browser::Get()->is_ready()) { + gin_helper::ErrorThrower(isolate).ThrowError( + "safeStorage cannot be used before app is ready"); + return v8::Local(); + } gin_helper::ErrorThrower(isolate).ThrowError( "Error while decrypting the ciphertext provided to " "safeStorage.decryptString. " @@ -59,7 +71,12 @@ v8::Local EncryptString(v8::Isolate* isolate, } std::string DecryptString(v8::Isolate* isolate, v8::Local buffer) { - if (!OSCrypt::IsEncryptionAvailable()) { + if (!IsEncryptionAvailable()) { + if (!Browser::Get()->is_ready()) { + gin_helper::ErrorThrower(isolate).ThrowError( + "safeStorage cannot be used before app is ready"); + return ""; + } gin_helper::ErrorThrower(isolate).ThrowError( "Error while decrypting the ciphertext provided to " "safeStorage.decryptString. " diff --git a/spec-main/api-safe-storage-spec.ts b/spec-main/api-safe-storage-spec.ts index b89f909513bd1..098095e2fde06 100644 --- a/spec-main/api-safe-storage-spec.ts +++ b/spec-main/api-safe-storage-spec.ts @@ -12,8 +12,27 @@ import * as fs from 'fs'; * * Because all encryption methods are gated by isEncryptionAvailable, the methods will never return the correct values * when run on CI and linux. +* Refs: https://github.com/electron/electron/issues/30424. */ +describe('safeStorage module', () => { + it('safeStorage before and after app is ready', async () => { + const appPath = path.join(__dirname, 'fixtures', 'crash-cases', 'safe-storage'); + const appProcess = cp.spawn(process.execPath, [appPath]); + + let output = ''; + appProcess.stdout.on('data', data => { output += data; }); + appProcess.stderr.on('data', data => { output += data; }); + + const code = (await emittedOnce(appProcess, 'exit'))[0] ?? 1; + + if (code !== 0 && output) { + console.log(output); + } + expect(code).to.equal(0); + }); +}); + ifdescribe(process.platform !== 'linux')('safeStorage module', () => { after(async () => { const pathToEncryptedString = path.resolve(__dirname, 'fixtures', 'api', 'safe-storage', 'encrypted.txt'); diff --git a/spec-main/fixtures/crash-cases/safe-storage/index.js b/spec-main/fixtures/crash-cases/safe-storage/index.js new file mode 100644 index 0000000000000..151751820a8d5 --- /dev/null +++ b/spec-main/fixtures/crash-cases/safe-storage/index.js @@ -0,0 +1,39 @@ +const { app, safeStorage } = require('electron'); +const { expect } = require('chai'); + +(async () => { + if (!app.isReady()) { + // isEncryptionAvailable() returns false before the app is ready on + // Linux: https://github.com/electron/electron/issues/32206 + // and + // Windows: https://github.com/electron/electron/issues/33640. + expect(safeStorage.isEncryptionAvailable()).to.equal(process.platform === 'darwin'); + if (safeStorage.isEncryptionAvailable()) { + const plaintext = 'plaintext'; + const ciphertext = safeStorage.encryptString(plaintext); + expect(Buffer.isBuffer(ciphertext)).to.equal(true); + expect(safeStorage.decryptString(ciphertext)).to.equal(plaintext); + } else { + expect(() => safeStorage.encryptString('plaintext')).to.throw(/safeStorage cannot be used before app is ready/); + expect(() => safeStorage.decryptString(Buffer.from(''))).to.throw(/safeStorage cannot be used before app is ready/); + } + } + await app.whenReady(); + // isEncryptionAvailable() will always return false on CI due to a mocked + // dbus as mentioned above. + expect(safeStorage.isEncryptionAvailable()).to.equal(process.platform !== 'linux'); + if (safeStorage.isEncryptionAvailable()) { + const plaintext = 'plaintext'; + const ciphertext = safeStorage.encryptString(plaintext); + expect(Buffer.isBuffer(ciphertext)).to.equal(true); + expect(safeStorage.decryptString(ciphertext)).to.equal(plaintext); + } else { + expect(() => safeStorage.encryptString('plaintext')).to.throw(/Encryption is not available/); + expect(() => safeStorage.decryptString(Buffer.from(''))).to.throw(/Decryption is not available/); + } +})() + .then(app.quit) + .catch((err) => { + console.error(err); + app.exit(1); + });