Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

daemon.load_wallet to raise if given redundant password #8799

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+125 −20
Draft

daemon.load_wallet to raise if given redundant password #8799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 20 additions & 18 deletions electrum/daemon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,10 @@ def _load_wallet(
if db.get_action():
raise WalletUnfinished(db)
wallet = Wallet(db, config=config)
if password:
# if a password was given, we check it.
# e.g. for unpassworded wallet, if given pw, we should raise.
wallet.check_password(password)
Comment on lines +518 to +521
Copy link
Member Author

Choose a reason for hiding this comment

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

@ecdsa commented out of band that load_wallet should not care about the keystore, only about the storage (wallet.check_password tests both storage and keystore), and e.g. if a password is given to load_wallet for a wallet that only has keystore encryption, load_wallet should unconditionally raise, even if the provided password matches the keystore

diff --git a/electrum/daemon.py b/electrum/daemon.py
index 4cfd0012db..3225e590d5 100644
--- a/electrum/daemon.py
+++ b/electrum/daemon.py
@@ -510,6 +510,9 @@ class Daemon(Logger):
             if not password:
                 raise InvalidPassword('No password given')
             storage.decrypt(password)
+        else:
+            if password:
+                raise InvalidPassword("password given but wallet storage is not encrypted")
         # read data, pass it to db
         db = WalletDB(storage.read(), storage=storage, upgrade=upgrade)
         if db.get_action():

return wallet

@with_wallet_lock
Expand Down Expand Up @@ -641,33 +645,31 @@ def _check_password_for_directory(self, *, old_password, new_password=None, wall
if not os.path.isfile(path):
continue
wallet = self.get_wallet(path)
# note: we only create a new wallet object if one was not loaded into the wallet already.
# note: we only create a new wallet object if one was not loaded into the daemon already.
# This is to avoid having two wallet objects contending for the same file.
# Take care: this only works if the daemon knows about all wallet objects.
# if other code already has created a Wallet() for a file but did not tell the daemon,
# hard-to-understand bugs will follow...
if wallet is None:
try:
wallet = self._load_wallet(path, old_password, upgrade=True, config=self.config)
except util.InvalidPassword:
pass
except Exception:
self.logger.exception(f'failed to load wallet at {path!r}:')
try:
for old_password_real in (old_password, None):
try:
if wallet is None:
wallet = self._load_wallet(path, old_password_real, upgrade=True, config=self.config)
if wallet is None:
continue
wallet.check_password(old_password_real)
except util.InvalidPassword:
wallet = None
else:
break
except Exception:
wallet = None
self.logger.exception(f'failed to load wallet at {path!r}:')
if wallet is None:
failed.append(path)
continue
if not wallet.storage.is_encrypted():
is_unified = False
try:
try:
wallet.check_password(old_password)
old_password_real = old_password
except util.InvalidPassword:
wallet.check_password(None)
old_password_real = None
except Exception:
failed.append(path)
continue
if new_password:
self.logger.info(f'updating password for wallet: {path!r}')
wallet.update_password(old_password_real, new_password, encrypt_storage=True)
Expand Down
104 changes: 104 additions & 0 deletions electrum/tests/test_daemon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from electrum.simple_config import SimpleConfig
from electrum.wallet import restore_wallet_from_text, Abstract_Wallet
from electrum import util
from electrum.crypto import CiphertextFormatError

from . import ElectrumTestCase, as_testnet

Expand Down Expand Up @@ -236,3 +237,106 @@ async def test_wp_command_with_diskfile_wallet_no_password(self):
# in unit tests or custom code, the "wallet" param is often an Abstract_Wallet:
self.assertEqual("bitter grass shiver impose acquire brush forget axis eager alone wine silver",
await cmds.getseed(wallet=wallet))


class TestCommandsWithDaemonIncorrectPassword(DaemonTestCase):
"""Test how the commands react when given an incorrect pw, no pw, redundant pw, correct pw;
how this interacts with unlocking, etc."""
TESTNET = True

async def test_sto_encrypted_wallet(self):
cmds = Commands(config=self.config, daemon=self.daemon)
wpath = self._restore_wallet_from_text("bitter grass shiver impose acquire brush forget axis eager alone wine silver", password="123456", encrypt_file=True)

# try without password:
with self.assertRaises(util.InvalidPassword):
await cmds.load_wallet(wallet_path=wpath, password=None)
self.assertIsNone(self.daemon.get_wallet(wpath))
# try with incorrect pw:
with self.assertRaises(util.InvalidPassword):
await cmds.load_wallet(wallet_path=wpath, password="badpass")
self.assertIsNone(self.daemon.get_wallet(wpath))
# try with correct pw:
await cmds.load_wallet(wallet_path=wpath, password="123456")
self.assertIsInstance(self.daemon.get_wallet(wpath), Abstract_Wallet)

# try without password:
with self.assertRaises(Exception) as ctx:
await cmds.getseed(wallet=wpath)
self.assertTrue("Password required" in ctx.exception.args[0])
# try with incorrect pw:
with self.assertRaises(util.InvalidPassword):
await cmds.getseed(wallet=wpath, password="wrongpass")
# try with correct pw:
self.assertEqual("bitter grass shiver impose acquire brush forget axis eager alone wine silver",
await cmds.getseed(wallet=wpath, password="123456"))

async def test_ks_encrypted_wallet(self):
cmds = Commands(config=self.config, daemon=self.daemon)
wpath = self._restore_wallet_from_text("bitter grass shiver impose acquire brush forget axis eager alone wine silver", password="123456", encrypt_file=False)

async def run_command():
# try without password:
with self.assertRaises(Exception) as ctx:
await cmds.getseed(wallet=wpath)
self.assertTrue("Password required" in ctx.exception.args[0])
# try with incorrect pw:
with self.assertRaises(util.InvalidPassword):
await cmds.getseed(wallet=wpath, password="wrongpass")
# try with correct pw:
self.assertEqual("bitter grass shiver impose acquire brush forget axis eager alone wine silver",
await cmds.getseed(wallet=wpath, password="123456"))

# try with incorrect pw:
with self.assertRaises(util.InvalidPassword):
await cmds.load_wallet(wallet_path=wpath, password="badpass")
self.assertIsNone(self.daemon.get_wallet(wpath))

# try without password (ok):
await cmds.load_wallet(wallet_path=wpath, password=None)
self.assertIsInstance(self.daemon.get_wallet(wpath), Abstract_Wallet)
await run_command()
# undo load_wallet:
await cmds.close_wallet(wallet_path=wpath)
self.assertIsNone(self.daemon.get_wallet(wpath))

# try with correct pw (ok):
await cmds.load_wallet(wallet_path=wpath, password="123456")
self.assertIsInstance(self.daemon.get_wallet(wpath), Abstract_Wallet)
await run_command()

async def test_unpassworded_wallet(self):
cmds = Commands(config=self.config, daemon=self.daemon)
wpath = self._restore_wallet_from_text("bitter grass shiver impose acquire brush forget axis eager alone wine silver", password=None)

# try with incorrect pw:
with self.assertRaises(util.InvalidPassword):
await cmds.load_wallet(wallet_path=wpath, password="badpass")
self.assertIsNone(self.daemon.get_wallet(wpath))
# try without password (correct):
await cmds.load_wallet(wallet_path=wpath, password=None)
self.assertIsInstance(self.daemon.get_wallet(wpath), Abstract_Wallet)

# try with incorrect pw:
with self.assertRaises(CiphertextFormatError): # FIXME ideally should raise InvalidPassword instead
await cmds.getseed(wallet=wpath, password="wrongpass")
# try without password (correct):
self.assertEqual("bitter grass shiver impose acquire brush forget axis eager alone wine silver",
await cmds.getseed(wallet=wpath))

async def test_load_wallet_unlock(self):
cmds = Commands(config=self.config, daemon=self.daemon)
wpath = self._restore_wallet_from_text("bitter grass shiver impose acquire brush forget axis eager alone wine silver", password="123456", encrypt_file=True)

await cmds.load_wallet(wallet_path=wpath, password="123456", unlock=True)
self.assertIsInstance(self.daemon.get_wallet(wpath), Abstract_Wallet)

# try without password (ok because of unlock):
self.assertEqual("bitter grass shiver impose acquire brush forget axis eager alone wine silver",
await cmds.getseed(wallet=wpath))
# try with incorrect pw:
with self.assertRaises(util.InvalidPassword):
await cmds.getseed(wallet=wpath, password="wrongpass")
# try with correct pw:
self.assertEqual("bitter grass shiver impose acquire brush forget axis eager alone wine silver",
await cmds.getseed(wallet=wpath, password="123456"))
3 changes: 1 addition & 2 deletions electrum/wallet.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3268,8 +3268,7 @@ def synchronize(self) -> int:

def unlock(self, password):
self.logger.info(f'unlocking wallet')
if self.has_password():
self.check_password(password)
self.check_password(password)
self._password_in_memory = password

def get_unlocked_password(self):
Expand Down