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
  15. fanquake referenced this in commit da3401e50f on Sep 9, 2026
  16. sedited referenced this in commit 9d4325f403 on Sep 9, 2026
  17. Kino1994 referenced this in commit 8f97fb87cd on Sep 13, 2026
  18. apoelstra referenced this in commit 1d310595ca on Sep 22, 2026
  19. ajtowns referenced this in commit e2d79ee55a on Oct 23, 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-11-12 07:15 UTC

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