Merge bitcoin/bitcoin#36186: test: return False for a too-short ECDSA signature
What the developer wrote
Authored by merge-script
Merge bitcoin/bitcoin#36186: test: return False for a too-short ECDSA signature
00d0c5107b92c8972ab106c680579e06fd4fb100 test: return False for a too-short ECDSA signature (Ferdinando Ametrano)
Pull request description:
`verify_ecdsa` reads `sig[1]` before it checks `len(sig) < 4`, so a 0- or 1-byte signature raises `IndexError` instead of returning `False`, which the comment promises for any DER encoding error.
Move the length check ahead of the byte access, and add a regression test.
ACKs for top commit:
optout21:
reACK 00d0c5107b92c8972ab106c680579e06fd4fb100
sedited:
ACK 00d0c5107b92c8972ab106c680579e06fd4fb100
Tree-SHA512: 01b8cfee145c539c7338f8edfaae455119585026ec7d4464e27255160b741504f0d2542c4c264d40cc2e8583404d353276b8e15c23bc6a96b42262257e8c7f05
What changed, and why it matters
This is a small fix in Bitcoin Core's own test helper code. A helper function used only in tests could crash with an IndexError when given an extremely short fake signature, instead of cleanly returning False. The change moves a length check earlier so the function behaves as documented. It does not affect live Bitcoin network code, wallet signing, or transaction validation.
No urgent action. Treat as a normal test-quality fix. Reviewers may verify the new regression test covers the reported short-signature cases and that no other test helpers have similar out-of-order bounds checks.
Security signals we found
Out-of-order bounds check leading to IndexError in test helper
Regression test added for malformed short DER signatures
Test-only code path, no production validation logic changed
Evidence from the diff
The commit modifies test/functional/test_framework/key.py’s verify_ecdsa(). Previously it read sig[1] before checking len(sig) < 4, so 0- or 1-byte inputs raised IndexError. The patch reorders the checks so length and header-byte checks happen before indexed reads, and adds a regression test for empty, 1-byte, 2-byte and 3-byte signatures. This is test-framework-only code; bitcoind consensus or mempool ECDSA verification is unaffected.
Changed components
test/functional/test_framework/key.pyverify_ecdsa() helper used by functional testsInspect captured patch +11 / −2
### test/functional/test_framework/key.py
@@ -65,12 +65,12 @@ def verify_ecdsa(self, sig, msg, low_s=True):
# Extract r and s from the DER formatted signature. Return false for
# any DER encoding errors.
- if (sig[1] + 2 != len(sig)):
- return False
if (len(sig) < 4):
return False
if (sig[0] != 0x30):
return False
+ if (sig[1] + 2 != len(sig)):
+ return False
if (sig[2] != 0x02):
return False
rlen = sig[3]
@@ -312,6 +312,15 @@ def test_ecdsa_and_schnorr(self):
self.assertFalse(verify_pubkey.verify_ecdsa(sig_ecdsa, msg))
self.assertFalse(verify_schnorr(verify_xonly_pubkey, sig_schnorr, msg))
+ def test_verify_ecdsa_rejects_short_sig(self):
+ """A signature too short to hold a DER header returns False, not IndexError."""
+ privkey = ECKey()
+ privkey.set(generate_privkey(), compressed=True)
+ pubkey = privkey.get_pubkey()
+ msg = bytes(32)
+ for sig in [b'', b'\x30', b'\x30\x00', b'\x30\x01\x02']:
+ self.assertFalse(pubkey.verify_ecdsa(sig, msg))
+
def test_schnorr_testvectors(self):
"""Implement the BIP340 test vectors (read from bip340_test_vectors.csv)."""
num_tests = 0Why this scored 20/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.