tests: cover rejection of excessive 0xFF padding in DER integers #1989

pull amartya1523 wants to merge 1 commits into bitcoin-core:master from amartya1523:tests-der-ff-padding changing 1 files +33 −0

amartya1523 commented at 4:46 PM on November 3, 2026: contributor

secp256k1_der_parse_integer rejects a negative integer whose leading 0xFF byte is redundant (X.690 8.3.2), but nothing in the test suite checks it. random_ber_signature can produce such encodings, but doesn't mark them as certainly_not_der, so run_ecdsa_der_parse stays 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).

tests: cover rejection of excessive 0xFF padding in DER integers

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.

4aa2803d5e

pinheadmz commented at 5:20 PM on November 3, 2026: none

@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.
amartya1523 commented at 5:29 PM on November 3, 2026: contributor

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.

in src/tests.c:7560 in 4aa2803d5e

7552 | @@ -7553,6 +7553,38 @@ static void run_ecdsa_der_parse_long_form(void) {
7553 |      CHECK(secp256k1_ecdsa_signature_parse_der(CTX, &sig, buf, len) == 0);
7554 |  }
7555 |  
7556 | +/* Tests the minimal encoding of negative integers (X.690-0207 8.3.2).
7557 | + *
7558 | + * A negative R or S is not rejected: secp256k1_der_parse_integer flags it as
7559 | + * overflowing and, as for any overflow, sets the scalar to zero. A leading
7560 | + * 0xFF byte is not allowed if the next byte has its top bit set, though.

real-or-random commented at 8:09 AM on November 4, 2026:

I think it would be nicer to move the descriptions of the exact checked conditions above the correspondling CHECK line.

in src/tests.c:7562 in 4aa2803d5e

7557 | + *
7558 | + * A negative R or S is not rejected: secp256k1_der_parse_integer flags it as
7559 | + * overflowing and, as for any overflow, sets the scalar to zero. A leading
7560 | + * 0xFF byte is not allowed if the next byte has its top bit set, though.
7561 | + * random_ber_signature can emit such padding, but does not mark those
7562 | + * signatures as certainly_not_der, so run_ecdsa_der_parse would not notice if

real-or-random commented at 8:09 AM on November 4, 2026:

I think we can drop this comment. It's hard to maintain because it creates a coupling with random_ber_signature

real-or-random commented at 8:09 AM on November 4, 2026: contributor

Concept ACK
theStack added the label assurance on Nov 4, 2026

Contributors

Labels

Linked (view graph)

#1924 tests: add coverage for the DER long form length encoding