keystore: Old_KeyStore: fix check_password(None) if ks has password
What the developer wrote
Authored by SomberNight
keystore: Old_KeyStore: fix check_password(None) if ks has password
check_password(None) should raise InvalidPassword when called on a keystore that has a password.
regression from https://github.com/spesmilo/electrum/commit/f86bdc86a2f2cc78f12e072b1eb5ae06f40d2178
fixes https://github.com/spesmilo/electrum/issues/10142
What changed, and why it matters
This commit fixes a bug in Electrum where an old-style wallet with a password would incorrectly accept a blank/empty password (None) as valid. The fix makes sure that if a wallet has a password, calling check_password(None) raises an InvalidPassword error instead of succeeding. This is a security-hardening fix for a regression introduced by an earlier change.
Apply the patch and run the new regression test. Review other keystore subclasses for similar password=None handling inconsistencies. Consider whether any UI or API callers rely on check_password(None) behavior for old-seed wallets and update them if needed.
Security signals we found
Fixes a regression where password validation could be bypassed for old-seed keystores
check_password(None) no longer succeeds on password-protected old-seed wallets
Adds regression test for password behavior on old-seed standard wallets
References issue #10142, indicating user-reported bug
Evidence from the diff
In electrum/keystore.py, Old_KeyStore._get_hex_seed() now raises InvalidPassword() when the seed is not already stored as a hex string and the provided password is None. This prevents check_password(None) from returning successfully on password-protected old-seed keystores. A regression test was added in tests/test_wallet.py to verify that after setting a password on an old-seed standard wallet, check_password(None) and check_password(‘wrong password’) both raise InvalidPassword, while check_password(‘1234’) succeeds.
Changed components
electrum/keystore.py: Old_KeyStore._get_hex_seed()tests/test_wallet.py: TestWalletPassword.test_update_password_of_standard_wallet_oldseedInspect captured patch +16 / −0
diff --git a/electrum/keystore.py b/electrum/keystore.py
index 0408894..340dcd4 100644
--- a/electrum/keystore.py
+++ b/electrum/keystore.py
@@ -737,6 +737,8 @@ class Old_KeyStore(MasterPublicKeyMixin, Deterministic_KeyStore):
return Old_KeyStore({'mpk': self.mpk})
def _get_hex_seed(self, password) -> str:
+ if not is_hex_str(self.seed) and password is None:
+ raise InvalidPassword()
hex_str = pw_decode(self.seed, password, version=self.pw_hash_version)
assert is_hex_str(hex_str), f"expected hex str, got {type(hex_str)} with {len(hex_str)=}"
return hex_str
diff --git a/tests/test_wallet.py b/tests/test_wallet.py
index 759bcbd..77ae701 100644
--- a/tests/test_wallet.py
+++ b/tests/test_wallet.py
@@ -347,6 +347,20 @@ class TestWalletPassword(WalletTestCase):
wallet.check_password("wrong password")
wallet.check_password("1234")
+ async def test_update_password_of_standard_wallet_oldseed(self):
+ d = restore_wallet_from_text__for_unittest(
+ "powerful random nobody notice nothing important anyway look away hidden message over", path=self.wallet_path, config=self.config)
+ wallet = d['wallet'] # type: Standard_Wallet
+
+ wallet.check_password(None)
+
+ wallet.update_password(None, "1234")
+ with self.assertRaises(InvalidPassword):
+ wallet.check_password(None)
+ with self.assertRaises(InvalidPassword):
+ wallet.check_password("wrong password")
+ wallet.check_password("1234")
+
async def test_update_password_with_app_restarts(self):
wallet_str = '{"addr_history":{"1364Js2VG66BwRdkaoxAaFtdPb1eQgn8Dr":[],"15CyDgLffJsJgQrhcyooFH4gnVDG82pUrA":[],"1Exet2BhHsFxKTwhnfdsBMkPYLGvobxuW6":[]},"addresses":{"change":[],"receiving":["1364Js2VG66BwRdkaoxAaFtdPb1eQgn8Dr","1Exet2BhHsFxKTwhnfdsBMkPYLGvobxuW6","15CyDgLffJsJgQrhcyooFH4gnVDG82pUrA"]},"keystore":{"keypairs":{"0344b1588589958b0bcab03435061539e9bcf54677c104904044e4f8901f4ebdf5":"L2sED74axVXC4H8szBJ4rQJrkfem7UMc6usLCPUoEWxDCFGUaGUM","0389508c13999d08ffae0f434a085f4185922d64765c0bff2f66e36ad7f745cc5f":"L3Gi6EQLvYw8gEEUckmqawkevfj9s8hxoQDFveQJGZHTfyWnbk1U","04575f52b82f159fa649d2a4c353eb7435f30206f0a6cb9674fbd659f45082c37d559ffd19bea9c0d3b7dcc07a7b79f4cffb76026d5d4dff35341efe99056e22d2":"5JyVyXU1LiRXATvRTQvR9Kp8Rx1X84j2x49iGkjSsXipydtByUq"},"type":"imported"},"pruned_txo":{},"seed_version":13,"stored_height":-1,"transactions":{},"tx_fees":{},"txi":{},"txo":{},"use_encryption":false,"verified_tx3":{},"wallet_type":"standard","winpos-qt":[100,100,840,405]}'
storage = WalletStorage(self.wallet_path)
Why this scored 44/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.