Protect database encryption key with Electron `safeStorage` API

[pl4nty]

First time contributor checklist:

• [X] I have read the README and Contributor Guidelines
• [X] I have signed the Contributor Licence Agreement

Contributor checklist:

• [X] My contribution is not related to translations.
• [X] My commits are in nice logical chunks with good commit messages
• [X] My changes are rebased on the latest main branch
• [X] A yarn ready run passes successfully (more about tests here)
• [ ] My changes are ready to be shipped to users

Description

Protecting local user data has been discussed extensively in #5703 and elsewhere. I recently watched Claudia speak at CrikeyCon about offline recovery of Signal messages, relying on the plaintext encryption key rather than noisy live attacks like keylogging.

As a simple mitigation, I've implemented Electron's safeStorage API to opportunistically encrypt the key with platform APIs like DPAPI on Windows and Keychain on macOS. I'd love some feedback on this approach, particularly

• Any additional features to accept this PR eg migration of existing plaintext keys
• Whether the mitigated threats (eg offline analysis and low-privilege malware) are worth the user impact (password prompts) on Linux and macOS

CI has passed on my fork and installers can be downloaded here. I've manually tested startup, messaging, and restarts on

• Windows 11 Education 22631.3374 (DPAPI)
• Debian Bullseye in a devcontainer (basic_text)

[pl4nty]

I've also implemented AppX packaging on a separate branch, which provides filesystem isolation on Windows and mitigates attachment theft

[AkechiShiro]

Any way forward with this PR what is on wait ?

[DoxrGitHub]

This is sigma skibidi, as a Signal alpha I do approve of this encryption rizz.

[orazioedoardo]

Would love to be corrected but AFAIK on Linux this is only useful in case the user doesn’t have any home or full disk encryption, as the keystore does not bind secrets to the app that saved them in the first place.

In other words, even if Signal Desktop saves the encryption key in the keystore, any other software that can read the keystore can access the encryption key without any involvement from the user, as long as the user is logged in.

[pl4nty]

@orazioedoardo this PR is intended to mitigate offline attacks like database exfiltration, where access to the running system and keystore is unavailable.

That said, keystore access usually requires non-standard permissions so the changes may provide some protection. I guess it could depend on which of the supported keystores are used.

App-bound encryption seems to be implemented in Chromium's OSCrypt for Windows and macOS (with caveats). Maybe it's worth opening an issue upstream for Linux, if one of the keystores supports app binding?

[driuba]

Would love to be corrected but AFAIK on Linux this is only useful in case the user doesn’t have any home or full disk encryption …

@orazioedoardo Full disk encryption does nothing once the system is booted and working. It protects the data in storage and prevents reading it of the disk without encryption keys, but once the storage is mounted there is no difference between encrypted and non-encrypted disks form the OS perspective.

The problem here is that any user side program can access the encryption keys and, therefore, full chat history.

I haven't looked into key binding to apps on Linux, so can't comment on that.

[orazioedoardo]

Full disk encryption does nothing once the system is booted and working.

OP clarified that threat model for this PR is offline extraction, for which FDE also protects. Even with this PR, once the keystore is unlocked (== user logged in), we are back to the FDE booted model. This is not to say the PR isn't useful, but it's an incremental step.

[Zk2u]

The problem here is that any user side program can access the encryption keys and, therefore, full chat history.

On macOS, safeStorage is backed by the system's keychain which mitigates from apps accessing each other's secrets. On other platforms, like Windows with DPAPI, it doesn't seem to make much difference as they only limit access by user rather than app.

[karlhorky]

The security weakness addressed by this PR is getting publicity over on Twitter/X:

• https://x.com/mysk_co/status/1809287118235070662

[nbtvmedia]

A couple of questions:

It seems that Signal is using the MacOS keychain for something, but it's not to store the decryption key of the local desktop msg database. Does anyone know what it's being used for?

Also, does anyone know the reason why Signal doesn't currently store the decryption key in the keychain, like whatsapp does? Is it just a matter of bandwidth/priorities or is there something else?

[Zk2u]

@nbtvmedia This PR should be closed without changes as it's being tracked properly in https://github.com/signalapp/Signal-Desktop/pull/6933. I see a "Signal Safe Storage" with what looks like a base64 encoded key.

I'm not sure what it's currently being used for, but in the new PR it uses the key in safeStorage (the macos keychain) to decrypt the encryption key in config.json, which is then used to decrypt/encrypt the database itself. I'm not quite sure what it's being used for currently however...

All that I can see is in app/main.ts on line 1777 there is app.commandLine.appendSwitch('password-store', 'basic');

This matches up with safeStorage's command line option for --password-store="basic" which forces basic_text/plaintext secret storage on linux... I think.

This is just a guess though, this isn't my code. Also, if "NBTV" is Naomi Brockwell TV, I'm a big fan :) guessing we'll see a video on this soon lol

[AkechiShiro]

@scottnonnenberg-signal any reviews on the mentioned PRs or issue at hand ?

[jamiebuilds-signal]

We have implemented support for the Electron safeStorage API to start using the system keystore in a separate commit here: https://github.com/signalapp/Signal-Desktop/commit/e449702a3ad4d07a603b1779914810dc77d7efde

In addition to migrating to encrypted/keystore-backed local database encryption keys on supported platforms, our implementation also includes some additional troubleshooting steps and a temporary fallback option that will allow users to recover their message database using their legacy database encryption key if something goes wrong. This should help minimize data loss if any edge cases or other keystore-related bugs are discovered during the migration process and production rollout. The temporary fallback and legacy key will both be removed after everything has been tested and deployed on a wide variety of devices across various operating systems and OS versions.

This is a big change that will require a lot of testing. It will start rolling out soon in an upcoming beta release and hit production shortly after that assuming everything goes well.

We'd like to thank @pl4nty and @AaronDewes for their assistance — and thanks in advance to everyone who helps us test this new feature too. You can learn more about the beta process and find out how to join the Signal Desktop beta here. We appreciate your support!

[pm4rcin]

Do better next time. Don't wait for a twitter drama to implement things (especially if people send you PR) and be more responsive. I hope the lesson was learnt and similar problem doesn't happen in the future. Good luck!

[nbtvmedia]

Thanks for the update! And thanks for your awesome work, and providing such valuable privacy tools!

[MrHamel]

https://github.com/signalapp/Signal-Desktop/blob/e449702a3ad4d07a603b1779914810dc77d7efde/app/main.ts#L1631-L1639

@jamiebuilds-signal @AaronDewes @pl4nty

What is the point of setting the encrypted key on line 1634, if the plain text version will still be set immediately after on line 1635? Shouldn't the plain text (legacy) key be removed from userConfig (userConfig.set('key', undefined);) if the key was successfully encrypted and stored (migrated)?

[pl4nty]

@MrHamel from Jamie's comment:

temporary fallback option that will allow users to recover their message database using their legacy database encryption key if something goes wrong. This should help minimize data loss if any edge cases or other keystore-related bugs are discovered during the migration process and production rollout. The temporary fallback and legacy key will both be removed after everything has been tested and deployed on a wide variety of devices across various operating systems and OS versions.

[MrHamel]

@pl4nty How would something go wrong if the function can already read and write to that file for the existing key? If there was a keystore bug, Electron would not have pushed the safeStorage API into production almost three years ago. Many applications are using it now.

[ericlaw1979]

How would something go wrong

See, for example, https://textslashplain.com/2020/09/28/local-data-encryption-in-chromium/#:~:text=OS%20DPAPI%20key%20was%20corrupted

[ghost]

What I also wonder it's why do they not take advantage of the modern sandbox OS like Mac ? and why not using windows hello / touchid to "unlock" the database.

If you add this to their complete refusal to publish app on the store make me really wonder if they really have our security at heart

[ygoe]

Security aspects noted, but please don't kill the import/export "feature" that allows users to transfer their entire chat history from one computer to another. I don't want to lose that when I move to a new PC. Until now, I could copy the files from the old to the new PC and pair the app with the phone again to have a seamless transfer. It's not documented, but it's the best and only solution, and it works very well. If this isn't working after this change, a separate and official feature is needed.

[marcolaux]

but please don't kill the import/export "feature"

~~Can you elaborate where this feature is hidden? On Windows and macOS I can't see such a feature in the settings nor in the chat options.~~

sorry, just read the rest 😅

Yeah I also did it this way, but this usually breaks file downloads of previous files (while images still can be viewed within a chat), stickers and group images.

[ygoe]

I haven't noticed any issues with the old "imported" history yet, but maybe I just didn't happen to try that. Anyway, I'd still prefer this issue fixed and a proper (and somehow secure, if needed) export/import feature accessible right from the UI.