fix(core): fix bug in multisig verification.
What the developer wrote
Authored by Andrew Kozlik
fix(core): fix bug in multisig verification.
(cherry picked from commit bf4775e2a7bf74916585197501a6f70c24b90449)
What changed, and why it matters
This update fixes a bug in how Trezor verifies ownership proofs for Bitcoin multisig wallets. Before the fix, a malicious or malformed proof could reuse the same valid signature twice and trick the device into accepting it as if two different keys had signed. The fix ensures each signature is checked against a distinct public key, so duplicate signatures are correctly rejected. The vendor marks this as a security fix.
Treat this as a security fix and include it in any firmware release. Users relying on multisig ownership proofs (e.g., SLIP-0019 proof-of-ownership) should upgrade. Review whether the flawed verifier could have affected other call sites or signature-checking flows.
Security signals we found
Security-relevant changelog fragment named +multisig.security
Fix changes signature verification logic for multisig ownership proofs
New regression test explicitly constructs an invalid proof by duplicating a signature and asserts it is rejected
Cherry-picked from another commit, indicating backport of a fix
Evidence from the diff
In core/src/apps/bitcoin/verification.py, SignatureVerifier looped over signatures and advanced the public-key index i inside a while-loop only when secp256k1.verify returned False. Because i was incremented unconditionally after a failed verify, a signature that verified against public_keys[i] would leave i unchanged, allowing the same public key to be matched again by the next signature. The patch separates verification from index advancement: it tries public_keys[i], then increments i regardless of whether the current signature matched, so each signature must correspond to a different public key. A new test (test_p2wsh_invalid_proof) demonstrates that a 2-of-2 P2WSH ownership proof with the same signature duplicated now raises DataError(‘Invalid signature’).
Changed components
Trezor Core firmwareapps/bitcoin/verification.pyBitcoin multisig ownership proof verificationP2WSH and similar multisig address types using SignatureVerifierInspect captured patch +81 / −1
diff --git a/core/.changelog.d/+multisig.security b/core/.changelog.d/+multisig.security
new file mode 100644
index 00000000..52881fc1
--- /dev/null
+++ b/core/.changelog.d/+multisig.security
@@ -0,0 +1 @@
+Fixed bug in multisig verification.
diff --git a/core/src/apps/bitcoin/verification.py b/core/src/apps/bitcoin/verification.py
index 557c11a9..40e57803 100644
--- a/core/src/apps/bitcoin/verification.py
+++ b/core/src/apps/bitcoin/verification.py
@@ -143,7 +143,11 @@ class SignatureVerifier:
i = 0
for der_signature, _ in self.signatures:
signature = der.decode_signature(der_signature)
- while not secp256k1.verify(self.public_keys[i], signature, digest):
+ valid = False
+ while not valid:
+ # If the signature does not match any public key, then public_keys[i] will
+ # raise an IndexError, resulting in an invalid signature exception.
+ valid = secp256k1.verify(self.public_keys[i], signature, digest)
i += 1
except Exception:
raise DataError("Invalid signature")
diff --git a/core/tests/test_apps.bitcoin.ownership_proof.py b/core/tests/test_apps.bitcoin.ownership_proof.py
index f0fe0af0..efaab336 100644
--- a/core/tests/test_apps.bitcoin.ownership_proof.py
+++ b/core/tests/test_apps.bitcoin.ownership_proof.py
@@ -4,6 +4,7 @@ from common import * # isort:skip
from trezor.crypto import bip39
from trezor.enums import InputScriptType
from trezor.messages import HDNodeType, MultisigRedeemScriptType
+from trezor.wire import DataError
from apps.bitcoin import ownership, scripts
from apps.bitcoin.addresses import (
@@ -587,6 +588,80 @@ class TestOwnershipProof(unittest.TestCase):
)
)
+ def test_p2wsh_invalid_proof(self):
+ # Creates an invalid OwnershipProof for a 2-of-2 multisig address in which the same signature appears twice.
+
+ coin = coins.by_name("Bitcoin")
+ seed1 = bip39.seed(" ".join(["all"] * 12), "")
+ seed2 = bip39.seed(" ".join(["all"] * 12), "TREZOR")
+ commitment_data = b"TREZOR"
+
+ nodes = []
+ keychains = []
+ for seed in [seed1, seed2]:
+ keychain = Keychain(
+ seed,
+ coin.curve_name,
+ [AlwaysMatchingSchema],
+ slip21_namespaces=[[b"SLIP-0019"]],
+ )
+ keychains.append(keychain)
+ node = keychain.derive([84 | HARDENED, 0 | HARDENED, 0 | HARDENED])
+ nodes.append(
+ HDNodeType(
+ depth=node.depth(),
+ child_num=node.child_num(),
+ fingerprint=node.fingerprint(),
+ chain_code=node.chain_code(),
+ public_key=node.public_key(),
+ )
+ )
+
+ multisig = MultisigRedeemScriptType(
+ nodes=nodes,
+ address_n=[1, 0],
+ signatures=[b"", b""],
+ m=2,
+ )
+
+ pubkeys = multisig_get_pubkeys(multisig)
+ address = _address_multisig_p2wsh(pubkeys, multisig.m, coin.bech32_prefix)
+ script_pubkey = scripts.output_derive_script(address, coin)
+ ownership_ids = [b"\x00" * 32, b"\x00" * 32]
+
+ # Sign with the first key.
+ _, signature = ownership.generate_proof(
+ node=keychains[0].derive([84 | HARDENED, 0 | HARDENED, 0 | HARDENED, 1, 0]),
+ script_type=InputScriptType.SPENDWITNESS,
+ multisig=multisig,
+ coin=coin,
+ user_confirmed=False,
+ ownership_ids=ownership_ids,
+ script_pubkey=script_pubkey,
+ commitment_data=commitment_data,
+ )
+
+ # Use the first signature for the second key.
+ multisig.signatures[1] = signature
+
+ # Sign with the first key again.
+ proof, signature = ownership.generate_proof(
+ node=keychains[0].derive([84 | HARDENED, 0 | HARDENED, 0 | HARDENED, 1, 0]),
+ script_type=InputScriptType.SPENDWITNESS,
+ multisig=multisig,
+ coin=coin,
+ user_confirmed=False,
+ ownership_ids=ownership_ids,
+ script_pubkey=script_pubkey,
+ commitment_data=commitment_data,
+ )
+
+ with self.assertRaises(DataError) as e:
+ ownership.verify_nonownership(
+ proof, script_pubkey, commitment_data, keychain, coin
+ )
+ self.assertEqual(e.value.message, "Invalid signature")
+
if __name__ == "__main__":
unittest.main()
Why this scored 60/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.