Skip to content

Conversation

@Not-Nik
Copy link
Collaborator

@Not-Nik Not-Nik commented Nov 20, 2025

Encrypt SQLite database whose connections are created in mozStorageService. Cloned and ATTACHed connections are also affected. The mechanism is behind a new security.storage.keystore.enabled. The keys for the connections are stored, encrypted by the SDR, in a new bikeshed/keystore.enc.

}

nsresult GetKeyForFile(nsIFile* aFile, nsCString& keyString) {
nsAutoCString telemetryFilename;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird naming here.

@gcp
Copy link
Contributor

gcp commented Nov 21, 2025

I think this wants tests as well. For Foxfooding we can deal with some flakyness but causing all SQLite databases to get corrupted will probably get old soon for the poor testers 😄

@Not-Nik Not-Nik marked this pull request as draft December 4, 2025 16:39
// PK11SDR_Encrypt will allocate the needed buffer in SECItem
encryptedKey.reset(new SECItem());

stat = PK11SDR_Encrypt(&keyid, key.get(), encryptedKey.get(), nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses 3DES. You want PK11SDR_EncryptWithMechanism.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason to have this in its own directory? These kinds of files tend to belong in security/manager (or at least a subdirectory therein).

Comment on lines +153 to +154
mozilla::UniqueSECItem key = mozilla::UniqueSECItem(new SECItem());
SECStatus stat = SECITEM_MakeItem(nullptr, key.get(),
(unsigned char*)encryptedKey.Data(),
encryptedKey.Length());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking, you probably don't need to do the whole new SECItem() ... SECITEM_MakeItem(...) - you can do something like SECItem key = { siBuffer, (unsigned char*)encryptedKey.Data(), encryptedKey.Length() }; (assuming the NSS functions you call either copy the data or only borrow it for the length of the call, which is most of them).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That particular example needs a "proper" SECItem (on the heap), because it is later stored in KEY_MAP, which is static and thus outlives encryptedKey. I already tried to reduce buffer allocation, and I'll move to a stack SECItem in the path == SYSTEM_KEY_NAME path, but if I missed anything else do let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of this code seems very similar to NSSKeyStore - is there a reason to not re-use that? (currently it's only used as a fallback on Linux, but you could use it unconditionally here)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For one the custom solution allows us some flexibility right away, only requiring the token to be unlocked once for loading all the keys. Later database open operations can then rely on the master key to do their work (create new key, decrypt existing key). Additionally, there is a plan to turn the keystore into the lockstore module which Benjamin has been working on.

@Not-Nik Not-Nik requested a review from gcp January 15, 2026 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants