fix: fix a crash in `safeStorage` on Linux #33913

RaisinTen · 2022-04-25T12:14:46Z

Description of Change

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()
expects the config to be set but the function responsible for setting the
config, SetConfig(),
is called only after the app is ready inside PostCreateMainMessageLoop().
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

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed a crash in safeStorage on Linux.

nornagon

I don't think this is worth a patch. Can we throw a JS exception instead if the function is called before the app is ready?

RaisinTen · 2022-04-26T08:08:00Z

@nornagon done, now isEncryptionAvailable() returns false if the app is not ready on Linux and the other APIs of safeStorage like encryptString() and decryptString() throws exceptions instead of crashing if isEncryptionAvailable() returns false.

ckerr

LGTM. What's happening is OSCrypt has a CHECK() to confirm that the config has been set, and Electron does that in ElectronBrowserMainParts::PostCreateMainMessageLoop.

An alternative would be to set the config sooner in Electron's life cycle, but from a quick read I can't tell if that would have ripple effects.

Cross-platform app devs are already going to be waiting for ready on Windows, so documenting the same for Linux + fixing the crash is reasonable.

RaisinTen · 2022-05-02T10:38:27Z

@nornagon could you PTAL?

spec-main/api-safe-storage-spec.ts

shell/browser/api/electron_api_safe_storage.cc

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>

shell/browser/api/electron_api_safe_storage.cc

release-clerk · 2022-05-09T13:38:57Z

Release Notes Persisted

Fixed a crash in safeStorage on Linux.

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>

trop · 2022-05-17T07:14:28Z

@RaisinTen has manually backported this PR to "17-x-y", please check out #34261

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>

trop · 2022-05-17T07:15:55Z

@RaisinTen has manually backported this PR to "16-x-y", please check out #34262

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>

trop · 2022-05-17T07:16:58Z

@RaisinTen has manually backported this PR to "15-x-y", please check out #34263

* 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: #32206 Signed-off-by: Darshan Sen <raisinten@gmail.com> * fix: replace BUILDFLAG(IS_LINUX) with defined(OS_LINUX) Signed-off-by: Darshan Sen <raisinten@gmail.com> * Linux: Send OSCrypt raw encryption key to the Network Service This backports 0e09738. Signed-off-by: Darshan Sen <raisinten@gmail.com> * fix: add ifdef guard around NetworkService::SetEncryptionKey() network::mojom::NetworkService::SetEncryptionKey() is only available on Windows and macOS. Signed-off-by: Darshan Sen <raisinten@gmail.com>

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>

RaisinTen requested review from a team as code owners April 25, 2022 12:14

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 25, 2022

RaisinTen mentioned this pull request Apr 25, 2022

[Bug]: electron.safeStorage.isEncryptionAvailable() segfaults on Linux when called before creation of the first BrowserWindow #32206

Closed

3 tasks

RaisinTen force-pushed the fix/return-boolean-from-safeStorage.isEncryptionAvailable-instead-of-crashing branch from 3dcf54a to 656f235 Compare April 25, 2022 12:23

nornagon requested changes Apr 25, 2022

View reviewed changes

RaisinTen force-pushed the fix/return-boolean-from-safeStorage.isEncryptionAvailable-instead-of-crashing branch 2 times, most recently from 73832a7 to bfb392d Compare April 26, 2022 08:02

RaisinTen changed the title ~~fix: return boolean from safeStorage.isEncryptionAvailable() instead of crashing~~ fix: fix a crash in safeStorage on Linux Apr 26, 2022

RaisinTen force-pushed the fix/return-boolean-from-safeStorage.isEncryptionAvailable-instead-of-crashing branch from bfb392d to 5f4350a Compare April 26, 2022 08:05

RaisinTen requested a review from nornagon April 26, 2022 11:26

ckerr approved these changes Apr 26, 2022

View reviewed changes

ckerr added the semver/patch backwards-compatible bug fixes label Apr 27, 2022

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Apr 27, 2022

ckerr added target/15-x-y labels Apr 27, 2022

nornagon requested changes May 2, 2022

View reviewed changes

spec-main/api-safe-storage-spec.ts Outdated Show resolved Hide resolved

shell/browser/api/electron_api_safe_storage.cc Show resolved Hide resolved

RaisinTen force-pushed the fix/return-boolean-from-safeStorage.isEncryptionAvailable-instead-of-crashing branch from 17c80bb to 4197bc3 Compare May 5, 2022 08:24

RaisinTen requested a review from nornagon May 5, 2022 10:22

nornagon reviewed May 5, 2022

View reviewed changes

shell/browser/api/electron_api_safe_storage.cc Show resolved Hide resolved

nornagon approved these changes May 5, 2022

View reviewed changes

jkleinsc approved these changes May 9, 2022

View reviewed changes

jkleinsc merged commit 03e68e2 into electron:main May 9, 2022

trop bot added merged/18-x-y and removed target/18-x-y labels May 9, 2022

RaisinTen mentioned this pull request May 13, 2022

docs: note safeStorage.isEncryptionAvailable() needs ready event #33724

Merged

4 tasks

RaisinTen mentioned this pull request May 17, 2022

fix: crash in safeStorage on Linux #34261

Merged

trop bot added in-flight/17-x-y and removed needs-manual-bp/17-x-y labels May 17, 2022

RaisinTen mentioned this pull request May 17, 2022

fix: crash in safeStorage on Linux #34262

Merged

trop bot added in-flight/16-x-y and removed needs-manual-bp/16-x-y labels May 17, 2022

RaisinTen mentioned this pull request May 17, 2022

fix: crash in safeStorage on Linux #34263

Merged

trop bot added in-flight/15-x-y and removed needs-manual-bp/15-x-y labels May 17, 2022

trop bot added merged/16-x-y and removed in-flight/16-x-y labels May 19, 2022

commit-lint bot mentioned this pull request Jul 13, 2022

[pull] main from electron:main Blaquez-home/electron#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix a crash in `safeStorage` on Linux #33913

fix: fix a crash in `safeStorage` on Linux #33913

RaisinTen commented Apr 25, 2022 •

edited

nornagon left a comment

RaisinTen commented Apr 26, 2022

ckerr left a comment

RaisinTen commented May 2, 2022

release-clerk bot commented May 9, 2022

trop bot commented May 17, 2022

trop bot commented May 17, 2022

trop bot commented May 17, 2022

fix: fix a crash in safeStorage on Linux #33913

fix: fix a crash in safeStorage on Linux #33913

Conversation

RaisinTen commented Apr 25, 2022 • edited

Description of Change

Checklist

Release Notes

nornagon left a comment

Choose a reason for hiding this comment

RaisinTen commented Apr 26, 2022

ckerr left a comment

Choose a reason for hiding this comment

RaisinTen commented May 2, 2022

release-clerk bot commented May 9, 2022

trop bot commented May 17, 2022

trop bot commented May 17, 2022

trop bot commented May 17, 2022

fix: fix a crash in `safeStorage` on Linux #33913

fix: fix a crash in `safeStorage` on Linux #33913

RaisinTen commented Apr 25, 2022 •

edited