wallet: decrypt_message: consolidate checks from UIs
What the developer wrote
Authored by SomberNight
wallet: decrypt_message: consolidate checks from UIs
What changed, and why it matters
This commit moves input-validation and wallet-type checks for message decryption out of the user-interface and command-line layers and into the core wallet code. It also removes a faster, separate decryption path for imported wallets so all wallet types now go through the same central check. The change is a defensive consolidation: it makes it harder for future user interfaces or scripts to accidentally skip the watching-only, multisig, type, and hex-pubkey checks. There is no direct evidence in the diff of an exploitable bug being fixed, but the consolidation reduces the chance that a missing check in one UI could lead to a confusing error or unexpected behavior.
Treat as a hardening/refactoring change rather than an urgent security fix. Reviewers should verify that all call sites of wallet.decrypt_message now correctly handle UserFacingException, and that no other wallet subclasses or plugins still bypass these checks. Consider adding tests for the consolidated error conditions.
Security signals we found
Input validation consolidated into a single core method
Watching-only wallet check moved from UI to wallet layer
Multisig wallet unsupported-operation check moved from UI to wallet layer
Type and hex-format checks for pubkey and message moved from UI to wallet layer
Removal of Imported_Wallet-specific decryption shortcut to reduce bypass risk
No explicit security bug or CVE referenced in commit message
Evidence from the diff
The patch refactors Abstract_Wallet.decrypt_message() to accept keyword-only arguments and adds explicit guards: watching-only wallet rejection, multisig wallet rejection, message type check (must be str), and pubkey hex-string check. The command-line Commands.decrypt and the Qt main_window no longer perform these validations themselves; they delegate to the wallet method. Additionally, the Imported_Wallet.decrypt_message override is removed, forcing imported wallets through the same validated path in the superclass. A performance comment notes that for Imported_Wallet the address index is simply the pubkey itself, avoiding the slow pubkeys_to_address lookup. encrypt_message() only receives a docstring update.
Changed components
electrum/wallet.pyelectrum/commands.pyelectrum/gui/qt/main_window.pyInspect captured patch +25 / −17
diff --git a/electrum/commands.py b/electrum/commands.py
index 3f83148..ec8556c 100644
--- a/electrum/commands.py
+++ b/electrum/commands.py
@@ -1235,11 +1235,7 @@ class Commands(Logger):
arg:str:encrypted:Encrypted message
arg:str:pubkey:Public key of one of your wallet addresses
"""
- if not is_hex_str(pubkey):
- raise UserFacingException(f"pubkey must be a hex string instead of {repr(pubkey)}")
- if not isinstance(encrypted, str):
- raise UserFacingException(f"encrypted must be a str instead of {repr(encrypted)}")
- decrypted = wallet.decrypt_message(pubkey, encrypted, password)
+ decrypted = wallet.decrypt_message(pubkey=pubkey, message=encrypted, password=password)
return decrypted.decode('utf-8')
@command('w')
diff --git a/electrum/gui/qt/main_window.py b/electrum/gui/qt/main_window.py
index de31458..32dfa38 100644
--- a/electrum/gui/qt/main_window.py
+++ b/electrum/gui/qt/main_window.py
@@ -2231,11 +2231,13 @@ class ElectrumWindow(QMainWindow, MessageBoxMixin, Logger, QtEventListener):
encrypted_e: QTextEdit,
password,
) -> None:
- if self.wallet.is_watching_only():
- self.show_message(_('This is a watching-only wallet.'))
- return
ciphertext = encrypted_e.toPlainText()
- task = partial(self.wallet.decrypt_message, pubkey_e.text(), ciphertext, password)
+ task = partial(
+ self.wallet.decrypt_message,
+ pubkey=pubkey_e.text(),
+ message=ciphertext,
+ password=password,
+ )
def setText(text):
try:
diff --git a/electrum/wallet.py b/electrum/wallet.py
index 5c02870..f37a23b 100644
--- a/electrum/wallet.py
+++ b/electrum/wallet.py
@@ -3253,13 +3253,27 @@ class Abstract_Wallet(ABC, Logger, EventListener):
message = util.to_bytes(message)
return bitcoin.verify_usermessage_with_address(address, sig, message)
- def decrypt_message(self, pubkey: str, message: str, password) -> bytes:
- addr = self.pubkeys_to_address([pubkey])
- index = self.get_address_index(addr)
- return self.keystore.decrypt_message(index, message, password)
+ def decrypt_message(self, *, pubkey: Any | str, message: str, password) -> bytes:
+ """Caller must handle UserFacingException."""
+ if self.is_watching_only():
+ raise UserFacingException(_("This is a watching-only wallet."))
+ if isinstance(self, Multisig_Wallet): # FIXME does not work with multisig wallets. (see #5856)
+ raise UserFacingException(_("Decrypting messages is currently not implemented for multisig wallets."))
+ if not isinstance(message, str):
+ raise UserFacingException(f"message must be a str instead of {type(message)}")
+ if not is_hex_str(pubkey):
+ raise UserFacingException(f"pubkey must be a hex string instead of {type(pubkey)}")
+ if isinstance(self, Imported_Wallet):
+ # this branch is significantly faster. Imported_Wallet.pubkeys_to_address is slow.
+ addr_index = pubkey
+ else:
+ addr = self.pubkeys_to_address([pubkey]) # note: broken for multisig
+ addr_index = self.get_address_index(addr)
+ return self.keystore.decrypt_message(addr_index, message, password)
@classmethod
def encrypt_message(cls, *, pubkey: Any | str, message: Any | str) -> str:
+ """Caller must handle UserFacingException."""
try:
message = util.to_bytes(message)
except TypeError:
@@ -4028,10 +4042,6 @@ class Imported_Wallet(Simple_Wallet):
return addr
return None
- def decrypt_message(self, pubkey: str, message, password) -> bytes:
- # this is significantly faster than the implementation in the superclass
- return self.keystore.decrypt_message(pubkey, message, password)
-
class Deterministic_Wallet(Abstract_Wallet):
gap_limit_for_change: int
Why this scored 27/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.