ellswift: fix overflow flag handling in secp256k1_ellswift_xdh

[ZhiqiangXiongmao]

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: bitcoin/bitcoin#34558

[theStack]

ACK db58637

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

[furszy]

ACK db58637

[real-or-random]

utACK db58637

Great catch. This seems to be a genuine bug.

@sipa Can you review this?

[sipa]

ACK db58637. A test to prevent reverting this behavior would be nice.

[ZhiqiangXiongmao]

Hi,

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

Greetings

[real-or-random]

CI failures seem to be unrelated. I emptied the GitHub Actions cache and triggered a rebuild. Let's see if this fixes CI.

[real-or-random]

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

Unfortunately, no. @hebasto Any ideas?

[hebasto]

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.

[hebasto]

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

[hebasto]

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.

[ZhiqiangXiongmao]

@real-or-random Could you update your comment to use my new username?
Can someone merge #1823 from @hebasto and re-run the CI?

[real-or-random]

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

[hebasto]

I'm suggesting to rebase this PR.

[hebasto]

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

[hebasto]

@SHAKE256

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

[real-or-random]

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.

[ZhiqiangXiongmao]

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

[real-or-random]

ACK 2cca2b7

[real-or-random]

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.

[real-or-random]

utACK b99a94c

[theStack]

re-ACK b99a94c

[kevkevinpal]

ACK b99a94c

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: secp256k1_noverify_tests .........Subprocess aborted***Exception:  11.30 sec
    Start 2: secp256k1_tests
2/3 Test #2: secp256k1_tests ..................Subprocess aborted***Exception:  23.87 sec
    Start 3: secp256k1_exhaustive_tests
3/3 Test #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.