ellswift: fix overflow flag handling in secp256k1_ellswift_xdh #1821

pull ghost wants to merge 2 commits into bitcoin-core:master from Bitcoin-Cypherpunk:master changing 2 files +29 −1
  1. ghost 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. ghost 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. ghost 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. 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.

    git fetch https://github.com/bitcoin-core/secp256k1 master
git checkout FETCH_HEAD
git cherry-pick db58637b94f457f03e15d64ee7b79e0c9884ffd6
git cherry-pick d1a1a300cfbc9de148bd13e908bb0c085c101bfc
git branch -f master HEAD
git switch master 
git push --force
  21. hebasto commented at 1:26 PM on February 16, 2026: member

    @SHAKE256

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

  22. 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: :)

    git rebase origin/master 
git 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.

  23. 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
  24. unknown force-pushed on Feb 16, 2026
  25. ghost commented at 1:51 PM on February 16, 2026: none

    Well... keeping the old username in the commit. 🤣

  26. 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:

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

    And you could get rid of ret.

  27. 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.

  28. real-or-random approved
  29. real-or-random commented at 2:30 PM on February 16, 2026: contributor

    ACK 2cca2b71a919f25547049076e87b3b919ca033a7

  30. 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:

    git config --global user.name "New Author Name"
git config --global user.email "<email@address.example>"
git 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.

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

    utACK b99a94c3827e1b8e8505648758512eb3cf4aae03

  35. theStack approved
  36. theStack commented at 11:06 PM on February 16, 2026: contributor

    re-ACK b99a94c3827e1b8e8505648758512eb3cf4aae03

  37. kevkevinpal commented at 11:32 PM on February 16, 2026: contributor

    ACK b99a94c3827e1b8e8505648758512eb3cf4aae03

    I was able to validate that the new tests fail without the patch to src/modules/ellswift/main_impl.h by doing the following

    Testing new patch

    git fetch origin pull/1821/head:PR1821
git checkout PR1821
cmake -B build && cmake --build build -j 10
ctest --test-dir build
### Observe tests pass ###

    Testing test without patch

    git checkout origin/master -- src/modules/ellswift/main_impl.h
rm -rf ./build
cmake -B build && cmake --build build -j 10
ctest --test-dir build
### Observe new test failing ###

    Output on failure

    $ ctest --test-dir build
Internal ctest changing into directory: /mnt/shared_drive/DEVDIR/secp256k1/build
Test project /mnt/shared_drive/DEVDIR/secp256k1/build
    Start 1: secp256k1_noverify_tests
1/3 Test [#1](/github-metadata-backup-bitcoin-core-secp256k1/1/): secp256k1_noverify_tests .........Subprocess aborted***Exception:  11.30 sec
    Start 2: secp256k1_tests
2/3 Test [#2](/github-metadata-backup-bitcoin-core-secp256k1/2/): secp256k1_tests ..................Subprocess aborted***Exception:  23.87 sec
    Start 3: secp256k1_exhaustive_tests
3/3 Test [#3](/github-metadata-backup-bitcoin-core-secp256k1/3/): secp256k1_exhaustive_tests .......   Passed    4.23 sec

33% tests passed, 2 tests failed out of 3

Total Test time (real) =  39.40 sec

The following tests FAILED:
          1 - secp256k1_noverify_tests (Subprocess aborted)
          2 - secp256k1_tests (Subprocess aborted)
Errors while running CTest
Output from these tests are in: /mnt/shared_drive/DEVDIR/secp256k1/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.
  38. real-or-random merged this on Feb 17, 2026
  39. real-or-random closed this on Feb 17, 2026
  40. fanquake referenced this in commit fa3704594c on Feb 19, 2026
  41. fanquake referenced this in commit 46ca85d851 on Feb 24, 2026
  42. fanquake referenced this in commit 9478a62d21 on Feb 25, 2026
  43. fanquake referenced this in commit 0dab9edba4 on Mar 3, 2026
  44. fanquake referenced this in commit a50dbd2bcd on Mar 4, 2026
  45. real-or-random referenced this in commit 7a5f153d71 on Mar 4, 2026
  46. fanquake referenced this in commit ec7ea286b0 on Mar 10, 2026
  47. fanquake referenced this in commit 6458812af8 on Mar 17, 2026
  48. fanquake referenced this in commit 791949a724 on Mar 21, 2026
  49. fanquake referenced this in commit cffa9320f2 on Mar 23, 2026
  50. theStack referenced this in commit 994739fc33 on Mar 26, 2026
  51. fanquake referenced this in commit dfc5233b8c on Mar 27, 2026
  52. theStack referenced this in commit 0326234c51 on Apr 2, 2026
  53. fanquake referenced this in commit dfd54c959e on Apr 9, 2026
  54. luke-jr referenced this in commit fed5dd96cd on Apr 13, 2026
  55. vmta referenced this in commit 1ddc2f947f on Apr 26, 2026
  56. vmta referenced this in commit 56c40fe100 on Apr 27, 2026
Contributors
ghosttheStackfurszyreal-or-randomsipahebastokevkevinpal
Labels
bugneeds-changelog
Linked (view graph)
#1823 ci: Load Docker image by ID from builder step
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-05-19 06:52 UTC