ellswift: fix overflow flag handling in secp256k1_ellswift_xdh #1821

pull SHAKE256 wants to merge 2 commits into bitcoin-core:master from Bitcoin-Cypherpunk:master changing 2 files +29 −1
  1. SHAKE256 commented at 2:40 pm on February 11, 2026: none

    The secp256k1_ellswift_xdh function uses overflow = secp256k1_scalar_is_zero(&s) which overwrites the overflow flag from the preceding secp256k1_scalar_set_b32 call. This means secret keys >= the curve order are silently accepted (reduced mod n) instead of being rejected.

    The fix changes = to |=, matching the correct pattern already used in secp256k1_ecdh (main_impl.h, line 51).

    The ECDH module’s test suite explicitly tests overflow rejection (passes secp256k1_group_order_bytes as a key and checks the function returns 0). The ellswift test suite has no corresponding test, which is why this went undetected.

    Previous PR to the wrong repository: https://github.com/bitcoin/bitcoin/pull/34558

  2. theStack approved
  3. theStack commented at 6:33 pm on February 11, 2026: contributor

    ACK db58637b94f457f03e15d64ee7b79e0c9884ffd6

    Good catch. Would make sense to add a test as well (here or in a follow-up PR).

  4. furszy commented at 7:50 pm on February 11, 2026: member
    ACK db58637b94f457f03e15d64ee7b79e0c9884ffd6
  5. real-or-random approved
  6. real-or-random commented at 1:41 pm on February 12, 2026: contributor

    utACK db58637b94f457f03e15d64ee7b79e0c9884ffd6

    Great catch. This seems to be a genuine bug. @sipa Can you review this?

  7. real-or-random added the label needs-changelog on Feb 12, 2026
  8. real-or-random added the label bug on Feb 12, 2026
  9. sipa commented at 1:45 pm on February 12, 2026: contributor
    ACK db58637b94f457f03e15d64ee7b79e0c9884ffd6. A test to prevent reverting this behavior would be nice.
  10. SHAKE256 commented at 2:49 pm on February 12, 2026: none

    Hi,

    AFK right now due to Fasnacht. Today will write the test.

    Greetings

  11. real-or-random commented at 7:32 am on February 13, 2026: contributor
    CI failures seem to be unrelated. I emptied the GitHub Actions cache and triggered a rebuild. Let’s see if this fixes CI.
  12. real-or-random commented at 7:41 pm on February 13, 2026: contributor

    I emptied the GitHub Actions cache and triggered a rebuild. Let’s see if this fixes CI.

    Unfortunately, no. @hebasto Any ideas?

  13. hebasto commented at 8:09 pm on February 13, 2026: member

    I emptied the GitHub Actions cache and triggered a rebuild. Let’s see if this fixes CI.

    Unfortunately, no. @hebasto Any ideas?

    No idea, at this moment. I’ll look into it thoroughly tomorrow.

  14. hebasto commented at 8:46 pm on February 13, 2026: member

    I emptied the GitHub Actions cache and triggered a rebuild. Let’s see if this fixes CI.

    Unfortunately, no. @hebasto Any ideas?

    No idea, at this moment. I’ll look into it thoroughly tomorrow.

    I’ve reviewed the CI code. One line looks suspicious…

  15. hebasto commented at 9:40 pm on February 13, 2026: member

    I emptied the GitHub Actions cache and triggered a rebuild. Let’s see if this fixes CI.

    Unfortunately, no. @hebasto Any ideas?

    No idea, at this moment. I’ll look into it thoroughly tomorrow.

    I’ve reviewed the CI code. One line looks suspicious…

    Fixed in #1823.

  16. SHAKE256 commented at 2:01 pm on February 14, 2026: none
    @real-or-random Could you update your comment to use my new username? Can someone merge #1823 from @hebasto and re-run the CI?
  17. real-or-random commented at 12:21 pm on February 16, 2026: contributor

    @real-or-random Could you update your comment to use my new username?

    I assume you want it to disappear forever, so I’ll delete my comment. (Editing keeps a history.)

    Can someone merge #1823 from @hebasto and re-run the CI?

    Let me see.

  18. real-or-random referenced this in commit 322d0a4358 on Feb 16, 2026
  19. hebasto commented at 12:57 pm on February 16, 2026: member
    I’m suggesting to rebase this PR.
  20. SHAKE256 commented at 1:15 pm on February 16, 2026: none
    That’s not that easy @hebasto . Both are in master. I think it’s fine how it is. I can do but then this will have to be closed, new branch, push, create PR… I will not keep my repository after the merge so the whole process would be just waste.
  21. hebasto commented at 1:24 pm on February 16, 2026: member

    That’s not that easy @hebasto . Both are in master. I think it’s fine how it is. I can do but then this will have to be closed, new branch, push, create PR… I will not keep my repository after the merge so the whole process would be just waste.

    0git fetch https://github.com/bitcoin-core/secp256k1 master
1git checkout FETCH_HEAD
2git cherry-pick db58637b94f457f03e15d64ee7b79e0c9884ffd6
3git cherry-pick d1a1a300cfbc9de148bd13e908bb0c085c101bfc
4git branch -f master HEAD
5git switch master 
6git push --force
  22. hebasto commented at 1:26 pm on February 16, 2026: member

    @SHAKE256

    Btw, you might also want to change commits author’s name.

  23. real-or-random commented at 1:29 pm on February 16, 2026: contributor

    That’s not that easy @hebasto . Both are in master. I think it’s fine how it is. I can do but then this will have to be closed, new branch, push, create PR… I will not keep my repository after the merge so the whole process would be just waste.

    I think there’s a misunderstanding. It’s just two commands: :)

    0git rebase origin/master 
1git push --force-with-lease

    (assuming origin is this repo)

    See also https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes for background.

    But if you really don’t want to do it, someone else could take over and open a new PR. In general, I think we’d want to avoid merging PRs with failing CI. I don’t think it’s a super strict rule, but we should follow it unless there’s some good reason not to.

  24. ellswift: fix overflow flag handling in secp256k1_ellswift_xdh 
    The secp256k1_ellswift_xdh function uses overflow = secp256k1_scalar_is_zero(&s) which overwrites the overflow flag from the preceding secp256k1_scalar_set_b32 call. This means secret keys >= the curve order are silently accepted (reduced mod n) instead of being rejected.

The fix changes = to |=, matching the correct pattern already used in secp256k1_ecdh (main_impl.h, line 51).

The ECDH module's test suite explicitly tests overflow rejection (passes secp256k1_group_order_bytes as a key and checks the function returns 0). The ellswift test suite has no corresponding test, which is why this went undetected.
    307b49f1b9
  25. SHAKE256 force-pushed on Feb 16, 2026
  26. SHAKE256 commented at 1:51 pm on February 16, 2026: none
    Well… keeping the old username in the commit. 🤣
  27. in src/modules/ellswift/tests_impl.h:477 in 2cca2b71a9 
    472+
473+    testutil_random_scalar_order(&rand_scalar);
474+    secp256k1_scalar_get_b32(s_good, &rand_scalar);
475+
476+    ret = secp256k1_ellswift_create(CTX, ell_a64, s_good, NULL);
477+    CHECK(ret);
    real-or-random commented at 2:26 pm on February 16, 2026:

    nit:

    0    CHECK(secp256k1_ellswift_create(CTX, ell_a64, s_good, NULL) == 1);

    And you could get rid of ret.

  28. in src/modules/ellswift/tests_impl.h:488 in 2cca2b71a9 
    483+    memcpy(s_overflow_plus1, secp256k1_group_order_bytes, 32);
484+    s_overflow_plus1[31] += 1;
485+    CHECK(secp256k1_ellswift_xdh(CTX, output, ell_a64, ell_b64, s_zero, 0, &ellswift_xdh_hash_x32, NULL) == 0);
486+    CHECK(secp256k1_ellswift_xdh(CTX, output, ell_a64, ell_b64, s_overflow, 0, &ellswift_xdh_hash_x32, NULL) == 0);
487+    CHECK(secp256k1_ellswift_xdh(CTX, output, ell_a64, ell_b64, s_overflow_plus1, 0, &ellswift_xdh_hash_x32, NULL) == 0);
488+    s_overflow[31] -= 1;
    real-or-random commented at 2:28 pm on February 16, 2026:
    Instead of modifying s_overflow, I suggest adding an s_overflow_minus1. This is better for readability. Then you can get rid of s_overflow entirely and use secp256k1_group_order_bytes directly.
  29. real-or-random approved
  30. real-or-random commented at 2:30 pm on February 16, 2026: contributor
    ACK 2cca2b71a919f25547049076e87b3b919ca033a7
  31. real-or-random commented at 2:35 pm on February 16, 2026: contributor

    Well… keeping the old username in the commit. 🤣

    If you really care:

    0git config --global user.name "New Author Name"
1git config --global user.email "<email@address.example>"
2git rebase --exec 'git commit --amend --no-edit --reset-author' 322d0a435829f80fbb839abdb469f2a22c84c369

    Of course, if you have set up your new name/email already, you can skip the first two commands.

  32. Add tests for bad scalar inputs in ellswift XDH b99a94c382
  33. SHAKE256 force-pushed on Feb 16, 2026
  34. real-or-random approved
  35. real-or-random commented at 3:51 pm on February 16, 2026: contributor
    utACK b99a94c3827e1b8e8505648758512eb3cf4aae03


SHAKE256 theStack furszy real-or-random sipa hebasto

Labels
bug needs-changelog

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-02-16 20:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me