Repository navigation
tests: cover rejection of excessive 0xFF padding in DER integers - #1989
amartya1523 wants to merge 1 commit into
Conversation
secp256k1_der_parse_integer rejects a negative integer whose leading 0xFF byte is redundant, but no test checks this. random_ber_signature can produce such an encoding, yet it does not mark it as certainly_not_der, so run_ecdsa_der_parse passes even if the check is removed or either of its conditions is dropped. Add a padded encoding that must be rejected, along with the two negative encodings next to it that must still be accepted: a single 0xFF byte, and a 0xFF byte followed by a byte without its top bit set.
|
@amartya1523 you just opened 5 PRs in one hour. Please stop there, give us time to digest. In the meantime, please review our AI Policy and close any of the PRs you don't fully understand, or think are actually meaningful to the project, review resources are extremely limited. |
|
Thanks @pinheadmz , and sorry for the noise. Opening five PRs at once was too much and I’ve closed the other four. I kept this one because the excessive 0xFF padding check in secp256k1_der_parse_integer had no test: when I disabled it (or dropped either of its two conditions), the whole test suite still passed. E.g. FF 80 must be rejected because 80 alone already encodes -128, while FF (-1) and FF 7F (-129) are valid and should still parse (as overflow → zero). The new test checks exactly those three cases. I’ve read the AI policy. Going forward I’ll keep it to one PR at a time and wait for review. Happy to close this one too if it isn’t useful. |
| * A negative R or S is not rejected: secp256k1_der_parse_integer flags it as | ||
| * overflowing and, as for any overflow, sets the scalar to zero. A leading | ||
| * 0xFF byte is not allowed if the next byte has its top bit set, though. |
There was a problem hiding this comment.
I think it would be nicer to move the descriptions of the exact checked conditions above the correspondling CHECK line.
| * random_ber_signature can emit such padding, but does not mark those | ||
| * signatures as certainly_not_der, so run_ecdsa_der_parse would not notice if |
There was a problem hiding this comment.
I think we can drop this comment. It's hard to maintain because it creates a coupling with random_ber_signature
secp256k1_der_parse_integerrejects a negative integer whose leading 0xFF byte is redundant (X.690 8.3.2), but nothing in the test suite checks it.random_ber_signaturecan produce such encodings, but doesn't mark them ascertainly_not_der, sorun_ecdsa_der_parsestays green if the check is removed, or if either of its two conditions is dropped (I tried all three; a coverage build also shows 0 hits on that branch).This adds a small deterministic test with the padded encoding that must be rejected, plus the two neighbouring negative encodings that must still parse (as an overflow to zero): a lone 0xFF, and 0xFF 0x7F. It follows the same pattern as the recent long-form length test (#1924).