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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36186.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
64 | @@ -65,10 +65,10 @@ def verify_ecdsa(self, sig, msg, low_s=True): 65 | 66 | # Extract r and s from the DER formatted signature. Return false for 67 | # any DER encoding errors. 68 | - if (sig[1] + 2 != len(sig)): 69 | - return False 70 | if (len(sig) < 4): 71 | return False 72 | + if (sig[1] + 2 != len(sig)):
316 | + """A signature too short to hold a DER header returns False, not IndexError.""" 317 | + privkey = ECKey() 318 | + privkey.set(generate_privkey(), compressed=True) 319 | + pubkey = privkey.get_pubkey() 320 | + msg = bytes(32) 321 | + for sig in [b'', b'\x30', b'\x30\x00', b'\x30\x00\x00']:
bea69bc test: return False for a too-short ECDSA signature:
I suggest using slightly adjusted values, so that the bytes satisfy other conditions (even though the length requirement will fail them):
for sig in [b'', b'\x30', b'\x30\x00', b'\x30\x01\x02']:
ACK bea69bc600c97c94b1c0bf738b92640e8b06bf03
Trivial minor fix to key.py test code. Left some minor comments.
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
first, and add a regression test.
Thanks — both addressed. The length check now runs before any indexed access into sig (len(sig) < 4 first, then the sig[0]/sig[1]/sig[2] checks in index order), and the test exercises [b'', b'\x30', b'\x30\x00', b'\x30\x01\x02'].
reACK 00d0c5107b92c8972ab106c680579e06fd4fb100
A simple test-only change. Fixes a harmless evident bug in a test code. The effect was, that for some low-prio special test data the test triggered an exception instead of properly failing. The change is simple, and a test for it has been added to it as well. Verified that the test passes, and that it fails without the fix.
ACK 00d0c5107b92c8972ab106c680579e06fd4fb100