fix: crash in `safeStorage` on Linux (#34261)

* 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 <[email protected]>

* fix: replace BUILDFLAG(IS_LINUX) with defined(OS_LINUX)

Signed-off-by: Darshan Sen <[email protected]>

* 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 <[email protected]>

* fix: add ifdef guard around NetworkService::SetEncryptionKey()

network::mojom::NetworkService::SetEncryptionKey() is only available on
Windows and macOS.

Signed-off-by: Darshan Sen <[email protected]>

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

shell/browser/api/electron_api_safe_storage.cc

@@ -31,12 +31,24 @@ void SetElectronCryptoReady(bool ready) {
 #endif
 
 bool IsEncryptionAvailable() {
+#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.
+  if (!Browser::Get()->is_ready())
+    return false;
+#endif
   return OSCrypt::IsEncryptionAvailable();
 }
 
 v8::Local<v8::Value> 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<v8::Value>();
+    }
     gin_helper::ErrorThrower(isolate).ThrowError(
         "Error while decrypting the ciphertext provided to "
         "safeStorage.decryptString. "
@@ -59,7 +71,12 @@ v8::Local<v8::Value> EncryptString(v8::Isolate* isolate,
 }
 
 std::string DecryptString(v8::Isolate* isolate, v8::Local<v8::Value> 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. "

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"
@@ -466,6 +469,26 @@ void ElectronBrowserMainParts::PostCreateMainMessageLoop() {
   ui::OzonePlatform::GetInstance()->PostCreateMainMessageLoop(
       std::move(shutdown_cb));
   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<os_crypt::Config> config =
+      std::make_unique<os_crypt::Config>();
+  // 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.

shell/browser/net/system_network_context_manager.cc

@@ -314,46 +314,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<os_crypt::Config>();
-  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
 
+#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()) {
-#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()

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

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