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

pull brunoerg wants to merge 1 commits into bitcoin-core:master from brunoerg:2026-08-der-tests changing 1 files +117 −0
  1. brunoerg commented at 2:00 PM on August 26, 2026: contributor

    random_ber_signature only emits long form lengths in signatures it marks as certainly_not_der, so run_ecdsa_der_parse asserts that such inputs are rejected but never that a well-formed long form length is accepted. As a result, inverting any of the four conditions in secp256k1_der_read_len that govern the long form goes unnoticed by the test suite.

    Add the accepting cases, together with the two rejecting ones that share their boundaries: a length below 128, which must use the short form, and length octets with a leading zero. Since DER permits the long form only for lengths of at least 128, which a signature with two in-range scalars never reaches, the R integers in the accepting cases necessarily overflow to zero; the test checks the parsed values so this stays pinned down.

    I noticed this lack in the tests while working on mutation testing for the ecdsa impl. So these tests kill the following mutants: https://secp256k1.space/src/ecdsa_impl.h#863, https://secp256k1.space/src/ecdsa_impl.h#878 and https://secp256k1.space/src/ecdsa_impl.h#896

  2. in src/tests.c:7443 in de6076cc2f
    7438 | + *
    7439 | + * random_ber_signature only emits long form lengths in signatures it marks as
    7440 | + * certainly_not_der, so run_ecdsa_der_parse never asserts that a long form
    7441 | + * length is accepted. Note that the long form is only valid in DER for lengths
    7442 | + * of at least 128, which is more than a signature with two in-range scalars
    7443 | + * needs, so the R integers below necessarily overflow to zero. */
    


    real-or-random commented at 6:55 AM on August 27, 2026:

    I get that these values overflow, but can you explain why overflow "to zero"?


    brunoerg commented at 8:32 PM on August 27, 2026:

    Afaik, it explicitly zeroes the scalar on any overflow. But anyway, the wording here is not good. I will change it.


    brunoerg commented at 8:33 PM on August 27, 2026:

    Done. Let me know whether it's better.


    real-or-random commented at 7:21 AM on August 28, 2026:

    Okay, my question was kinda stupid, but yes, the improved comment is perfectly clear.

  3. real-or-random commented at 6:56 AM on August 27, 2026: contributor

    Concept ACK

    Nice!

  4. real-or-random added the label assurance on Aug 27, 2026
  5. real-or-random added the label tweak/refactor on Aug 27, 2026
  6. tests: add coverage for the DER long form length encoding
    random_ber_signature only emits long form lengths in signatures it marks
    as certainly_not_der, so run_ecdsa_der_parse asserts that such inputs are
    rejected but never that a well-formed long form length is accepted. As a
    result, inverting any of the four conditions in secp256k1_der_read_len
    that govern the long form goes unnoticed by the test suite.
    
    Add the accepting cases, together with the two rejecting ones that share
    their boundaries: a length below 128, which must use the short form, and
    length octets with a leading zero. Since DER permits the long form only
    for lengths of at least 128, which a signature with two in-range scalars
    never reaches, the R integers in the accepting cases are longer than 32
    bytes and are therefore parsed as zero; the test checks the parsed values
    so this stays pinned down.
    ddb1dfa770
  7. brunoerg force-pushed on Aug 27, 2026
  8. real-or-random approved
  9. real-or-random commented at 7:20 AM on August 28, 2026: contributor

    utACK ddb1dfa77038442dff86ba90e6d0a30adfddef17

  10. theStack approved
  11. theStack commented at 2:44 AM on August 30, 2026: contributor

    ACK ddb1dfa77038442dff86ba90e6d0a30adfddef17

  12. theStack merged this on Aug 30, 2026
  13. theStack closed this on Aug 30, 2026

  14. brunoerg deleted the branch on Aug 30, 2026

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-09-09 02:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me