From e5bd512660be5deb5ed5e256a0661bf740b62e37 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Mon, 9 May 2022 19:08:53 +0530 Subject: [PATCH 1/4] 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); + }); From 7bf6abe1792a8d4ed3aad629ba685de7d5e37268 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Tue, 17 May 2022 13:45:25 +0530 Subject: [PATCH 2/4] fix: replace BUILDFLAG(IS_LINUX) with defined(OS_LINUX) Signed-off-by: Darshan Sen --- shell/browser/api/electron_api_safe_storage.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/browser/api/electron_api_safe_storage.cc b/shell/browser/api/electron_api_safe_storage.cc index d0e3a73bcdb0d..7db2bb7718376 100644 --- a/shell/browser/api/electron_api_safe_storage.cc +++ b/shell/browser/api/electron_api_safe_storage.cc @@ -31,7 +31,7 @@ void SetElectronCryptoReady(bool ready) { #endif bool IsEncryptionAvailable() { -#if BUILDFLAG(IS_LINUX) +#if defined(OS_LINUX) // Calling IsEncryptionAvailable() before the app is ready results in a crash // on Linux. // Refs: https://github.com/electron/electron/issues/32206. From bc72f7a3e08383c083095174d1c60bbbaa38d342 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Wed, 18 May 2022 11:31:34 +0530 Subject: [PATCH 3/4] Linux: Send OSCrypt raw encryption key to the Network Service This backports https://github.com/electron/electron/pull/32419/commits/0e09738b182e71789396fd862d3dd28b64febfef. Signed-off-by: Darshan Sen --- shell/browser/electron_browser_main_parts.cc | 23 +++++++++++++ .../net/system_network_context_manager.cc | 34 ------------------- 2 files changed, 23 insertions(+), 34 deletions(-) diff --git a/shell/browser/electron_browser_main_parts.cc b/shell/browser/electron_browser_main_parts.cc index 42ad53af25174..eca1573c0a6ba 100644 --- a/shell/browser/electron_browser_main_parts.cc +++ b/shell/browser/electron_browser_main_parts.cc @@ -17,6 +17,9 @@ #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" #include "chrome/browser/icon_manager.h" +#include "chrome/common/chrome_paths.h" +#include "chrome/common/chrome_switches.h" +#include "components/os_crypt/key_storage_config_linux.h" #include "components/os_crypt/os_crypt.h" #include "content/browser/browser_main_loop.h" // nogncheck #include "content/public/browser/browser_thread.h" @@ -483,6 +486,26 @@ void ElectronBrowserMainParts::PostCreateMainMessageLoop() { #endif #if defined(OS_LINUX) bluez::DBusBluezManagerWrapperLinux::Initialize(); + + // Set up crypt config. This needs to be done before anything starts the + // network service, as the raw encryption key needs to be shared with the + // network service for encrypted cookie storage. + std::string app_name = electron::Browser::Get()->GetName(); + const base::CommandLine& command_line = + *base::CommandLine::ForCurrentProcess(); + std::unique_ptr config = + std::make_unique(); + // Forward to os_crypt the flag to use a specific password store. + config->store = command_line.GetSwitchValueASCII(::switches::kPasswordStore); + config->product_name = app_name; + config->application_name = app_name; + config->main_thread_runner = base::ThreadTaskRunnerHandle::Get(); + // c.f. + // https://source.chromium.org/chromium/chromium/src/+/master:chrome/common/chrome_switches.cc;l=689;drc=9d82515060b9b75fa941986f5db7390299669ef1 + config->should_use_preference = + command_line.HasSwitch(::switches::kEnableEncryptionSelection); + base::PathService::Get(chrome::DIR_USER_DATA, &config->user_data_path); + OSCrypt::SetConfig(std::move(config)); #endif #if defined(OS_POSIX) // Exit in response to SIGINT, SIGTERM, etc. diff --git a/shell/browser/net/system_network_context_manager.cc b/shell/browser/net/system_network_context_manager.cc index c2162e0ab5ba8..52c7184f56389 100644 --- a/shell/browser/net/system_network_context_manager.cc +++ b/shell/browser/net/system_network_context_manager.cc @@ -293,48 +293,14 @@ void SystemNetworkContextManager::OnNetworkServiceCreated( KeychainPassword::GetServiceName() = app_name + " Safe Storage"; KeychainPassword::GetAccountName() = app_name; #endif -#if defined(OS_LINUX) - // c.f. - // https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/net/system_network_context_manager.cc;l=515;drc=9d82515060b9b75fa941986f5db7390299669ef1;bpv=1;bpt=1 - const base::CommandLine& command_line = - *base::CommandLine::ForCurrentProcess(); - - auto config = std::make_unique(); - config->store = command_line.GetSwitchValueASCII(::switches::kPasswordStore); - config->product_name = app_name; - config->application_name = app_name; - config->main_thread_runner = base::ThreadTaskRunnerHandle::Get(); - // c.f. - // https://source.chromium.org/chromium/chromium/src/+/master:chrome/common/chrome_switches.cc;l=689;drc=9d82515060b9b75fa941986f5db7390299669ef1 - config->should_use_preference = - command_line.HasSwitch(::switches::kEnableEncryptionSelection); - base::PathService::Get(chrome::DIR_USER_DATA, &config->user_data_path); -#endif // The OSCrypt keys are process bound, so if network service is out of // process, send it the required key. if (content::IsOutOfProcessNetworkService() && electron::fuses::IsCookieEncryptionEnabled()) { -#if defined(OS_LINUX) - network::mojom::CryptConfigPtr network_crypt_config = - network::mojom::CryptConfig::New(); - network_crypt_config->application_name = config->application_name; - network_crypt_config->product_name = config->product_name; - network_crypt_config->store = config->store; - network_crypt_config->should_use_preference = config->should_use_preference; - network_crypt_config->user_data_path = config->user_data_path; - - network_service->SetCryptConfig(std::move(network_crypt_config)); - -#else network_service->SetEncryptionKey(OSCrypt::GetRawEncryptionKey()); -#endif } -#if defined(OS_LINUX) - OSCrypt::SetConfig(std::move(config)); -#endif - #if DCHECK_IS_ON() electron::safestorage::SetElectronCryptoReady(true); #endif From ae4ea0b4c067fab2f84b4ad6ca683133af709011 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Wed, 18 May 2022 12:11:01 +0530 Subject: [PATCH 4/4] fix: add ifdef guard around NetworkService::SetEncryptionKey() network::mojom::NetworkService::SetEncryptionKey() is only available on Windows and macOS. Signed-off-by: Darshan Sen --- shell/browser/net/system_network_context_manager.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/shell/browser/net/system_network_context_manager.cc b/shell/browser/net/system_network_context_manager.cc index 52c7184f56389..aefdbf01d755d 100644 --- a/shell/browser/net/system_network_context_manager.cc +++ b/shell/browser/net/system_network_context_manager.cc @@ -294,12 +294,14 @@ void SystemNetworkContextManager::OnNetworkServiceCreated( KeychainPassword::GetAccountName() = app_name; #endif +#if defined(OS_WIN) || defined(OS_MAC) // The OSCrypt keys are process bound, so if network service is out of // process, send it the required key. if (content::IsOutOfProcessNetworkService() && electron::fuses::IsCookieEncryptionEnabled()) { network_service->SetEncryptionKey(OSCrypt::GetRawEncryptionKey()); } +#endif #if DCHECK_IS_ON() electron::safestorage::SetElectronCryptoReady(true);