Repository: electron/electron (language: cpp)

Issue:
[Bug]: safeStorage use is invalid prior use of a BrowserWindow
### Preflight Checklist

- [X] I have read the [Contributing Guidelines](https://github.com/electron/electron/blob/main/CONTRIBUTING.md) for this project.
- [X] I agree to follow the [Code of Conduct](https://github.com/electron/electron/blob/main/CODE_OF_CONDUCT.md) that this project adheres to.
- [X] I have searched the [issue tracker](https://www.github.com/electron/electron/issues) for a bug report that matches the one I want to file, without success.

### Electron Version

19.0.0

### What operating system are you using?

macOS

### Operating System Version

macOS 12.4

### What arch are you using?

arm64 (including Apple Silicon)

### Last Known Working Electron version

_No response_

### Expected Behavior

* ✓ Wait for `app.whenReady()` to resolve
* ✓ Validate `safeStorage.isEncryptionAvailable()` is true
* ✓ Make calls to `safeStorage.encryptString()` and `safeStorage.decryptString()`
* ✓ Instantiate a `BrowserWindow()`
* x Make successful calls to `safeStorage.encryptString()` and `safeStorage.decryptString()` with arguments from first usage.
* x Have only one keychain password entry for the application using the correct product name and not "Chromium Safe Storage".

### Actual Behavior

1. The keychain service name used for the safeStorage's encryption key is altered after a `BrowserWindow` is created leading to encryption and decryption errors.
2. The keychain service name used before a `BrowserWindow` is created is generically titled "Chromium Safe Storage" which will conflict with any other electron apps on the system if used in this manor.

### Testcase Gist URL

_No response_

### Additional Information

I believe this is Mac Keychain specific as I didn't notice any specific mentions of application name for the other os_crypt backends in chromium and it pertains to packed and unpacked builds.

---

Here is where chromium sets the default keychain service name used for OSCrypt:

https://github.com/chromium/chromium/blob/641e2d024dc1cf180b16181ef23f287a80628573/components/os_crypt/keychain_password_mac.mm#L30-L36

Only after a `BrowserWindow` is created does this code run, which changes the keychains service name to the appropriate app name value.

https://github.com/electron/electron/blob/20538c4f34a471677d40ef304e76ae46a3a3c3f1/shell/browser/net/system_network_context_manager.cc#L292-L295

---

If a `BrowserWindow` really must be opened first, I'd expect `isEncryptionAvailable` to be false until then.  In other words my expectation was that any use of `safeStorage` after `isEncryptionAvailable() === true` should work consistently.  Ideally though, the keychain service name could be set before `app.whenReady()` resolves, so users can use safeStorage before having opened any BrowserWindows.  This is exactly how I use node-keytar presently.


Discussion / hints:
> x Make successful calls to safeStorage.encryptString() and safeStorage.decryptString() with arguments from first usage.

Do you mean that the last assertion in this test fails for you when you run it with `npx electron test.js`?

**`test.js`**
```js
const { app, safeStorage, BrowserWindow } = require('electron');
const { strictEqual } = require('assert');

(async () => {
  // ✓ Wait for app.whenReady() to resolve
  await app.whenReady();

  // ✓ Validate safeStorage.isEncryptionAvailable() is true
  strictEqual(safeStorage.isEncryptionAvailable(), true);

  const plainText = 'Hello, world!';

  // ✓ Make calls to safeStorage.encryptString() and safeStorage.decryptString()
  const cipherText = safeStorage.encryptString(plainText);
  strictEqual(safeStorage.decryptString(cipherText), plainText);

  // ✓ Instantiate a BrowserWindow()
  new BrowserWindow();

  // x Make successful calls to safeStorage.encryptString() and safeStorage.decryptString() with arguments from first usage.
  strictEqual(safeStorage.decryptString(cipherText), plainText);
})()
  .then(app.quit)
  .catch((err) => {
    console.error(err);
    app.exit(1);
  });
```

> x Have only one keychain password entry for the application using the correct product name and not "Chromium Safe Storage".

In the script above, it only creates the `"Chromium Safe Storage"` keychain password. However, if I create the `BrowserWindow` right after `await app.whenReady();`, it creates the `"Electron Safe Storage"` keychain password instead.
Hi @RaisinTen

Unfortunately that test will "pass" because the mac impl of OSCrypt in chromium will cache the encryption key in the first encryptString call.

Ref: https://github.com/chromium/chromium/blob/641e2d024dc1cf180b16181ef23f287a80628573/components/os_crypt/os_crypt_mac.mm#L92-L99

So while your final `strictEqual` call will succeed, any materials encrypted and stored for future invocations after the BrowserWindow call will actually be using the randomly generated encryption key from the "Chromium Safe Storage" keychain instead of the "**My Product** Safe Storage" keychain.

If and only if your application ALWAYS made an initial encrypt/decrypt call before the first BrowserWindow is created would everything appear to work properly, because of the poorly defined key caching in the mac OSCrypt impl.   Any use cases where the encrypt/decrypt calls sometimes run before a BrowserWindow is created and at other times after BrowserWindow, will lead to errors as I did in my exact use case.

Hopefully that makes sense.

I'm not sure how you would construct a simple test case for this, as the only way I see to reset the cached encryption key is to start a fresh chromium/network-service process.  But it would look something like this (which does fail on my mac test mule)..

```javascript
const { app, safeStorage, BrowserWindow } = require('electron');
const { strictEqual } = require('assert');
const fs = require('node:fs');

const firstRun = !process.argv.includes('--second-proc-run');
const tmpFile = 'encryption_ready_test_mac_impl_cipher';

(async () => {
  await app.whenReady();
  const plainText = 'Hello, world!';
  if (firstRun) {
    strictEqual(safeStorage.isEncryptionAvailable(), true);
    const cipherText = safeStorage.encryptString(plainText);
    strictEqual(safeStorage.decryptString(cipherText), plainText);
    new BrowserWindow();
    strictEqual(safeStorage.decryptString(cipherText), plainText);
    fs.writeFileSync(tmpFile, cipherText);
  } else {
    const cipherText = fs.readFileSync(tmpFile);
    new BrowserWindow();
    strictEqual(safeStorage.isEncryptionAvailable(), true);
    strictEqual(safeStorage.decryptString(cipherText), plainText);
  }
})()
  .then(app.quit)
  .catch((err) => {
    console.error(err);
    app.exit(1);
  });
```

Then you'd have to run it twice like this..
```shell
$ ./node_modules/.bin/electron . && ./node_modules/.bin/electron . --second-proc-run
```

Fix patch:
diff --git a/.circleci/config/base.yml b/.circleci/config/base.yml
index 04b3daec34de3..91efc6bb13547 100644
--- a/.circleci/config/base.yml
+++ b/.circleci/config/base.yml
@@ -216,6 +216,7 @@ step-maybe-cleanup-arm64-mac: &step-maybe-cleanup-arm64-mac
         rm -rf ~/Library/Application\ Support/electron*
         security delete-generic-password -l "Chromium Safe Storage" || echo "✓ Keychain does not contain password from tests"
         security delete-generic-password -l "Electron Test Main Safe Storage" || echo "✓ Keychain does not contain password from tests"
+        security delete-generic-password -a "electron-test-safe-storage" || echo "✓ Keychain does not contain password from tests"
       elif [ "$TARGET_ARCH" == "arm" ] || [ "$TARGET_ARCH" == "arm64" ]; then
         XVFB=/usr/bin/Xvfb
         /sbin/start-stop-daemon --stop --exec $XVFB || echo "Xvfb not running"
diff --git a/shell/browser/electron_browser_main_parts.cc b/shell/browser/electron_browser_main_parts.cc
index 698e957421818..1bf7fdc55fe4a 100644
--- a/shell/browser/electron_browser_main_parts.cc
+++ b/shell/browser/electron_browser_main_parts.cc
@@ -90,6 +90,7 @@
 #endif
 
 #if BUILDFLAG(IS_MAC)
+#include "components/os_crypt/keychain_password_mac.h"
 #include "services/device/public/cpp/geolocation/geolocation_manager.h"
 #include "shell/browser/ui/cocoa/views_delegate_mac.h"
 #else
@@ -481,6 +482,9 @@ void ElectronBrowserMainParts::WillRunMainMessageLoop(
 }
 
 void ElectronBrowserMainParts::PostCreateMainMessageLoop() {
+#if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_MAC)
+  std::string app_name = electron::Browser::Get()->GetName();
+#endif
 #if BUILDFLAG(IS_LINUX)
   auto shutdown_cb =
       base::BindOnce(base::RunLoop::QuitCurrentWhenIdleClosureDeprecated());
@@ -491,7 +495,6 @@ void ElectronBrowserMainParts::PostCreateMainMessageLoop() {
   // 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 =
@@ -508,6 +511,10 @@ void ElectronBrowserMainParts::PostCreateMainMessageLoop() {
   base::PathService::Get(DIR_SESSION_DATA, &config->user_data_path);
   OSCrypt::SetConfig(std::move(config));
 #endif
+#if BUILDFLAG(IS_MAC)
+  KeychainPassword::GetServiceName() = app_name + " Safe Storage";
+  KeychainPassword::GetAccountName() = app_name;
+#endif
 #if BUILDFLAG(IS_POSIX)
   // Exit in response to SIGINT, SIGTERM, etc.
   InstallShutdownSignalHandlers(
diff --git a/shell/browser/net/system_network_context_manager.cc b/shell/browser/net/system_network_context_manager.cc
index d1f474d59ead7..2b8f431425fad 100644
--- a/shell/browser/net/system_network_context_manager.cc
+++ b/shell/browser/net/system_network_context_manager.cc
@@ -42,10 +42,6 @@
 #include "shell/common/options_switches.h"
 #include "url/gurl.h"
 
-#if BUILDFLAG(IS_MAC)
-#include "components/os_crypt/keychain_password_mac.h"
-#endif
-
 #if BUILDFLAG(IS_LINUX)
 #include "components/os_crypt/key_storage_config_linux.h"
 #endif
@@ -288,12 +284,6 @@ void SystemNetworkContextManager::OnNetworkServiceCreated(
       base::FeatureList::IsEnabled(features::kAsyncDns),
       default_secure_dns_mode, doh_config, additional_dns_query_types_enabled);
 
-  std::string app_name = electron::Browser::Get()->GetName();
-#if BUILDFLAG(IS_MAC)
-  KeychainPassword::GetServiceName() = app_name + " Safe Storage";
-  KeychainPassword::GetAccountName() = app_name;
-#endif
-
   // The OSCrypt keys are process bound, so if network service is out of
   // process, send it the required key.
   if (content::IsOutOfProcessNetworkService() &&
diff --git a/spec/api-safe-storage-spec.ts b/spec/api-safe-storage-spec.ts
index 098095e2fde06..e7f4dcf18547c 100644
--- a/spec/api-safe-storage-spec.ts
+++ b/spec/api-safe-storage-spec.ts
@@ -4,7 +4,7 @@ import { safeStorage } from 'electron/main';
 import { expect } from 'chai';
 import { emittedOnce } from './events-helpers';
 import { ifdescribe } from './spec-helpers';
-import * as fs from 'fs';
+import * as fs from 'fs-extra';
 
 /* isEncryptionAvailable returns false in Linux when running CI due to a mocked dbus. This stops
 * Chrome from reaching the system's keyring or libsecret. When running the tests with config.store
@@ -36,8 +36,8 @@ describe('safeStorage module', () => {
 ifdescribe(process.platform !== 'linux')('safeStorage module', () => {
   after(async () => {
     const pathToEncryptedString = path.resolve(__dirname, 'fixtures', 'api', 'safe-storage', 'encrypted.txt');
-    if (fs.existsSync(pathToEncryptedString)) {
-      await fs.unlinkSync(pathToEncryptedString);
+    if (await fs.pathExists(pathToEncryptedString)) {
+      await fs.remove(pathToEncryptedString);
     }
   });
 


Test patch: