Skip to content

Commit

Permalink
fix: fix a crash in safeStorage on Linux
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: #32206
Signed-off-by: Darshan Sen <raisinten@gmail.com>
  • Loading branch information
RaisinTen committed Apr 26, 2022
1 parent f3e0517 commit bfb392d
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 5 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
11 changes: 9 additions & 2 deletions shell/browser/api/electron_api_safe_storage.cc
Expand Up @@ -31,12 +31,19 @@ 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()) {
gin_helper::ErrorThrower(isolate).ThrowError(
"Error while decrypting the ciphertext provided to "
"safeStorage.decryptString. "
Expand All @@ -59,7 +66,7 @@ v8::Local<v8::Value> EncryptString(v8::Isolate* isolate,
}

std::string DecryptString(v8::Isolate* isolate, v8::Local<v8::Value> buffer) {
if (!OSCrypt::IsEncryptionAvailable()) {
if (!IsEncryptionAvailable()) {
gin_helper::ErrorThrower(isolate).ThrowError(
"Error while decrypting the ciphertext provided to "
"safeStorage.decryptString. "
Expand Down
37 changes: 36 additions & 1 deletion spec-main/api-safe-storage-spec.ts
@@ -1,6 +1,6 @@
import * as cp from 'child_process';
import * as path from 'path';
import { safeStorage } from 'electron/main';
import { app, safeStorage } from 'electron/main';
import { expect } from 'chai';
import { emittedOnce } from './events-helpers';
import { ifdescribe } from './spec-helpers';
Expand All @@ -12,8 +12,43 @@ 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.isEncryptionAvailable() before and after app is ready', () => {
it('isEncryptionAvailable() returns false before app is ready', 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(encrypted)).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/);
}
}
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(encrypted)).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/);
}
});
});

ifdescribe(process.platform !== 'linux')('safeStorage module', () => {
after(async () => {
const pathToEncryptedString = path.resolve(__dirname, 'fixtures', 'api', 'safe-storage', 'encrypted.txt');
Expand Down

0 comments on commit bfb392d

Please sign in to comment.