leave the wallet file encryption unchanged in a queued update where the storage key has changed since it was queued
What changed, and why it matters
This commit fixes a bug in Sparrow Wallet where a background save of a wallet file could use the wrong encryption password. If a user added or removed a wallet password while another save was still waiting in a queue, the queued save could later encrypt or decrypt the file with the old setting, potentially leaving the wallet file unencrypted when it should be encrypted, or encrypted with a password the user no longer expects. The fix makes the queued save check the current encryption key before deciding whether to change the file's encryption.
Treat this as a security bugfix and include it in the next release. Users who changed wallet passwords while Sparrow was performing background saves should verify their wallet files are encrypted as expected. No immediate external action (such as rotating keys) is required, but wallet file encryption state should be confirmed after password changes.
Security signals we found
Race condition between asynchronous wallet persistence and password change
Potential unintended decryption of wallet file after password is added
Potential unintended encryption with stale password after password is removed
Encryption key visibility fixed by marking field volatile
Defensive check added to queued update to detect stale encryption key
Unit tests added to cover password-change concurrency scenarios
Evidence from the diff
The patch addresses a race condition between queued asynchronous wallet updates and password changes. In DbPersistence.updateWallet, an update is submitted to updateExecutor with the encryptionPubKey captured at queueing time. If the user subsequently changed the wallet password (via setPassword / saveWallet), the queued update could apply the stale key: for example, encrypting with a removed password or decrypting with an added one. The fix reads storage.getEncryptionPubKey() inside the executor task and compares it to the queued key; if they differ, it writes the update using the current datasource password without changing encryption. Storage.encryptionPubKey is also made volatile to ensure visibility across threads. New unit tests verify the three cases: queued no-key update after password added leaves file encrypted; queued keyed update after password removed leaves file unencrypted; queued update with new password correctly encrypts the file.
Changed components
com.sparrowwallet.sparrow.io.Storagecom.sparrowwallet.sparrow.io.db.DbPersistenceDbPersistenceTestInspect captured patch +71 / −5
### src/main/java/com/sparrowwallet/sparrow/io/Storage.java
@@ -49,7 +49,7 @@ public class Storage {
private Persistence persistence;
private File walletFile;
- private ECKey encryptionPubKey;
+ private volatile ECKey encryptionPubKey; //set where the wallet is saved, and read by the queued updates that follow on another thread
public Storage(File walletFile) {
this(!walletFile.exists() || walletFile.getName().endsWith("." + PersistenceType.DB.getExtension()) ? PersistenceType.DB : PersistenceType.JSON, walletFile);
### src/main/java/com/sparrowwallet/sparrow/io/db/DbPersistence.java
@@ -195,12 +195,18 @@ public void updateWallet(Storage storage, Wallet wallet) throws StorageException
@Override
public void updateWallet(Storage storage, Wallet wallet, ECKey encryptionPubKey) throws StorageException {
- String newPassword = getFilePassword(encryptionPubKey);
- String currentPassword = getDatasourcePassword();
-
updateExecutor.execute(() -> {
try {
- if(dataSource != null && currentPassword != null && newPassword == null) {
+ //An update can wait behind others while the wallet is saved with a password added or removed. The key it was queued with is then
+ //no longer the key of the storage, and it leaves the encryption of the file as it finds it
+ ECKey storagePubKey = storage.getEncryptionPubKey();
+ boolean queuedWithStorageKey = Objects.equals(encryptionPubKey, Storage.NO_PASSWORD_KEY.equals(storagePubKey) ? null : storagePubKey);
+ String newPassword = getFilePassword(encryptionPubKey);
+ String currentPassword = getDatasourcePassword();
+
+ if(!queuedWithStorageKey) {
+ update(storage, wallet, currentPassword);
+ } else if(dataSource != null && currentPassword != null && newPassword == null) {
//Removing encryption: write data first
update(storage, wallet, currentPassword);
updatePassword(storage, encryptionPubKey);
### src/test/java/com/sparrowwallet/sparrow/io/DbPersistenceTest.java
@@ -209,6 +209,66 @@ public void walletNameContainingExtensionUsesItsOwnFile() throws Exception {
Assertions.assertEquals(otherHash, getFileHash(otherFile), "another wallet file was written by a wallet name containing the extension");
}
+ /**
+ * An update is queued with the key the storage held at the time, and can still be waiting when the wallet is saved with a password added. Run
+ * after that save, it must leave the file as the save left it.
+ */
+ @Test
+ public void updateQueuedBeforeAPasswordWasAddedLeavesTheFileEncrypted() throws Exception {
+ Persistence persistence = PersistenceType.DB.getInstance();
+ Storage storage = new Storage(persistence, tempDir.resolve("Savings." + PersistenceType.DB.getExtension()).toFile());
+ storage.setKeyDeriver(new Argon2KeyDeriver());
+ storage.setEncryptionPubKey(Storage.NO_PASSWORD_KEY);
+ Wallet wallet = createWallet("Savings");
+ storage.saveWallet(wallet);
+
+ setPassword(storage, "pass");
+ persistence.updateWallet(storage, wallet); //as it was queued, with no key
+ storage.closeAndWait();
+
+ Assertions.assertTrue(Storage.isEncrypted(storage.getWalletFile()));
+ Assertions.assertTrue(isWalletValid(storage.getWalletFile(), "pass"));
+ }
+
+ /**
+ * The same in the other direction: an update queued while the wallet had a password does not put it back once it has been removed.
+ */
+ @Test
+ public void updateQueuedBeforeAPasswordWasRemovedLeavesTheFileUnencrypted() throws Exception {
+ Persistence persistence = PersistenceType.DB.getInstance();
+ Storage storage = new Storage(persistence, tempDir.resolve("Savings." + PersistenceType.DB.getExtension()).toFile());
+ storage.setKeyDeriver(new Argon2KeyDeriver());
+ storage.setEncryptionPubKey(Storage.NO_PASSWORD_KEY);
+ Wallet wallet = createWallet("Savings");
+ storage.saveWallet(wallet);
+ setPassword(storage, "pass");
+ ECKey queuedKey = storage.getEncryptionPubKey();
+
+ setPassword(storage, null);
+ persistence.updateWallet(storage, wallet, queuedKey);
+ storage.closeAndWait();
+
+ Assertions.assertFalse(Storage.isEncrypted(storage.getWalletFile()));
+ Assertions.assertTrue(isWalletValid(storage.getWalletFile(), null));
+ }
+
+ /**
+ * The path that must keep working: a password added without a full save reaches the file through the update queued with the new key.
+ */
+ @Test
+ public void updateQueuedWithANewPasswordEncryptsTheFile() throws Exception {
+ Storage storage = createUnencryptedWallet("Savings");
+ Wallet wallet = createWallet("Savings");
+ storage.saveWallet(wallet);
+
+ storage.setEncryptionPubKey(ECKey.fromPublicOnly(storage.getKeyDeriver().deriveECKey("pass")));
+ storage.updateWallet(wallet);
+ storage.closeAndWait();
+
+ Assertions.assertTrue(Storage.isEncrypted(storage.getWalletFile()));
+ Assertions.assertTrue(isWalletValid(storage.getWalletFile(), "pass"));
+ }
+
@Test
public void passwordRemovalDecryptsWalletFile() throws Exception {
Storage storage = createUnencryptedWallet("Savings");Why this scored 58/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.