Skip to content

Commit

Permalink
fix: fix a crash in safeStorage on Linux (electron#33913)
Browse files Browse the repository at this point in the history
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: electron#32206
Signed-off-by: Darshan Sen <raisinten@gmail.com>
  • Loading branch information
RaisinTen committed May 17, 2022
1 parent 52c019c commit 6588df6
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 4 deletions.
4 changes: 2 additions & 2 deletions docs/api/safe-storage.md
Expand Up @@ -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)`
Expand Down
21 changes: 19 additions & 2 deletions shell/browser/api/electron_api_safe_storage.cc
Expand Up @@ -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<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. "
Expand All @@ -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. "
Expand Down
19 changes: 19 additions & 0 deletions spec-main/api-safe-storage-spec.ts
Expand Up @@ -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');
Expand Down
39 changes: 39 additions & 0 deletions 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);
});

0 comments on commit 6588df6

Please sign in to comment.