Conversation
PK11_ListFixedKeysInSlot reports a token that is not authenticated the same way as an empty key store: softoken searches the key database only while the token counts as logged in (sftk_searchTokenList in softoken/pkcs11.c), so the search succeeds with no results and the wrapper turned the resulting null into Ok(None). get_or_create_aes256_key answered that by generating and persisting a fresh key, which either failed as "Encryption key is missing" or left key4.db with two as-logins-key objects and logins that no longer decrypt. Logouts are not serialized by GLOBAL_TOKEN_LOCK - on Firefox Desktop they run PK11_Logout on a background thread - so one landing in NSSKeyManager::get_key between the token check and the key lookup was enough. get_aes256_key now confirms the token is authenticated both before and after the search, so Ok(None) only means the token answered and holds no such key. The search itself cannot be interleaved with a logout, since pk11_FindObjectsByTemplate holds the slot monitor that PK11_Logout also takes. A token that could not be searched reaches the application as NSSAuthenticationError rather than MissingKey. ManagedEncryptorDecryptor mapped every get_key() failure to MissingKey and now passes on the ones that say nothing about the key, a cancelled primary password prompt among them.
… creation get_or_create_aes256_key took the lock twice, once inside get_aes256_key for the lookup and once inside import_and_persist_sym_key for the persist, so the decision that no key exists and the creation acting on it were not one operation. Two callers could both look up the key, both find nothing, and both create one, leaving key4.db with two objects under the same nickname and logins that decrypt with only one of them. A logout landing in the same gap reaches the persist rather than the lookup, where it fails the import instead of being reported as a token that could not be searched. get_or_create_aes256_key now takes the lock once and holds it across the lookup, the key generation and the persist. The lookup moves into lookup_aes256_key, which expects the caller to hold the lock; get_aes256_key keeps taking it for callers that only read. import_and_persist_sym_key can no longer take the lock itself without deadlocking against that hold, so it checks that the token is authenticated instead, which is the condition the lock was there to preserve.
jo
approved these changes
Sep 25, 2026
jo
left a comment
jo
left a comment
Contributor
There was a problem hiding this comment.
looks good! My comment shall not block the merge.
| mod tests_keydb { | ||
| use super::*; | ||
| use nss_as::ensure_initialized_with_profile_dir; | ||
| use std::path::PathBuf; |
Contributor
There was a problem hiding this comment.
if you'd add a TEST_LOCK similar to the one in sym_key that would solve these flaky tests, too, I think:
---- encryption::tests_keydb::test_nss_key_manager stdout ----
thread 'encryption::tests_keydb::test_nss_key_manager' (34385) panicked at components/support/db-crypto/src/encryption.rs:516:9:
assertion left == right failed
left: [123, 34, 107, 116, 121, 34, 58, 34, 111, 99, 116, 34, 44, 34, 107, 34, 58, 34, 100, 97, 79, 115, 88, 102, 51, 77, 104, 66, 90, 89, 104, 49, 97, 90, 116, 112, 106, 88, 101, 71, 105, 71, 73, 84, 88, 81, 101, 76, 110, 117, 71, 52, 68, 101, 85, 56, 117, 68, 55, 111, 85, 34, 125]
right: [123, 34, 107, 116, 121, 34, 58, 34, 111, 99, 116, 34, 44, 34, 107, 34, 58, 34, 66, 74, 104, 84, 108, 103, 51, 118, 56, 49, 65, 66, 51, 118, 87, 50, 71, 122, 54, 104, 69, 54, 84, 116, 75, 83, 112, 85, 102, 84, 86, 75, 73, 83, 99, 74, 45, 77, 78, 83, 67, 117, 99, 34, 125]
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request checklist
[ci full]to the PR title.