Skip to content

schnorrsig: Securely clear buf containing k or its negation #1731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 2, 2025
Merged

schnorrsig: Securely clear buf containing k or its negation #1731

merged 1 commit into from
Sep 2, 2025

Conversation

john-moffett
Copy link
Contributor

Follow-up to #1579. buf still holds the nonce or its negation, so ought to be cleared.

real-or-random
real-or-random approved these changes Sep 2, 2025
View reviewed changes
Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 366b125

It's a bit crazy that we clear rj because the non-uniqueness of the Jacobian representation could leak information about k but we forgot the damn buffer where k has been serialized.

Love that someone is reading the code. I take a closer look occasionally when I need to convince myself that it is correct, but it's not a very systematic effort.

@real-or-random
Copy link
Contributor

@theStack Want to review this? :)

theStack
theStack approved these changes Sep 2, 2025
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-review ACK 366b125

Nice find, and a bit surprising indeed that this was missed 😬
nit: as this buffer is only used for the serialization of the nonce, we could rename it to reflect that, e.g. to nonce_buf (fwiw, in the ECDSA signing routine the name nonce32 is used)

@john-moffett
Rename and clear var containing k or -k
325d65a 
buf currently holds k or -k and isn't cleared, so clear it and rename to
nonce32 to clarify its sensitivity and match how it is named in the
corresponding ECDSA sign_inner.
@john-moffett
Copy link
Contributor Author

we could rename it to reflect that, e.g. to nonce_buf (fwiw, in the ECDSA signing routine the name nonce32 is used)

Good idea, as it makes it clearer that it's sensitive information, too.

theStack
theStack approved these changes Sep 2, 2025
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review re-ACK 325d65a

real-or-random
real-or-random approved these changes Sep 2, 2025
View reviewed changes
Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 325d65a

@real-or-random real-or-random merged commit d93380f into bitcoin-core:master Sep 2, 2025
116 checks passed
vmta added a commit to umkoin/umkoin that referenced this pull request Sep 21, 2025
@vmta
Squashed 'src/secp256k1/' changes from 9fab42525..36e76952c
2b25f56 
36e76952c Merge bitcoin-core/secp256k1#1738: check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so)
4985ac0f8 Merge bitcoin-core/secp256k1#1737: doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static)
7ebaa134a check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so)
806de38bf doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static)
03fb60ad2 Merge bitcoin-core/secp256k1#1681: doc: Recommend clang-cl when building on Windows
d93380fb3 Merge bitcoin-core/secp256k1#1731: schnorrsig: Securely clear buf containing k or its negation
8113671f8 Merge bitcoin-core/secp256k1#1729: hash: Use size_t instead of int for RFC6979 outlen copy
325d65a8c Rename and clear var containing k or -k
960ba5f9c Use size_t instead of int for RFC6979 outlen copy
737912430 ci: Add more tests for clang-cl
7379a5bed doc: Recommend clang-cl when building on Windows
f36afb8b3 Merge bitcoin-core/secp256k1#1725: tests: refactor tagged hash verification
5153cf1c9 tests: refactor tagged hash tests
d2dcf5209 Merge bitcoin-core/secp256k1#1726: docs: fix broken link to Tromer's cache.pdf paper
489a43d1b docs: fix broken link to eprint cache.pdf paper
d59971414 Merge bitcoin-core/secp256k1#1722: docs: Exclude modules' `bench_impl.h` headers from coverage report
0458def51 doc: Add `--gcov-ignore-parse-errors=all` option to `gcovr` invocations
1aecce593 doc: Add `--merge-mode-functions=separate` option to `gcovr` invocations
106a7cbf4 doc: Exclude modules' `bench_impl.h` headers from coverage report
a9e955d3e autotools, docs: Adjust help string for `--enable-coverage` option
e523e4f90 Merge bitcoin-core/secp256k1#1720: chore(ci): Fix typo in Dockerfile comment
24ba8ff16 chore(ci): Fix typo in Dockerfile comment
74b8068c5 Merge bitcoin-core/secp256k1#1717: test: update wycheproof test vectors
c25c3c8a8 test: update wycheproof test vectors
20e3b4474 Merge bitcoin-core/secp256k1#1688: cmake: Avoid contaminating parent project's cache with `BUILD_SHARED_LIBS`
2c076d907 Merge bitcoin-core/secp256k1#1711: tests: update Wycheproof
7b07b2295 cmake: Avoid contaminating parent project's cache with BUILD_SHARED_LIBS
5433648ca Fix typos and spellings
9ea54c69b tests: update Wycheproof files
b9313c6e1 Merge bitcoin-core/secp256k1#1708: release cleanup: bump version after 0.7.0
a660a4976 Merge bitcoin-core/secp256k1#1707: release: Prepare for 0.7.0
7ab8b0cc0 release cleanup: bump version after 0.7.0
a3e742d94 release: Prepare for 0.7.0
f67b0ac1a ci: Don't hardcode ABI version
020ee6049 Merge bitcoin-core/secp256k1#1706: musig/tests: initialize keypair
cde413089 musig/tests: initialize keypair
6037833c9 Merge bitcoin-core/secp256k1#1702: changelog: update
40b4a0652 changelog: update
5e74086dc Merge bitcoin-core/secp256k1#1705: musig/test: Remove dead code
7c3380423 Merge bitcoin-core/secp256k1#1696: build: Refactor visibility logic and add override
8d967a602 musig/test: Remove dead code
983711cd6 musig/tests: Refactor vectors_signverify
73a695958 Merge bitcoin-core/secp256k1#1704: cmake: Make `secp256k1_objs` inherit interface defines from `secp256k1`
bf082221f cmake: Make `secp256k1_objs` inherit interface defines from `secp256k1`
c82d84bb8 build: add CMake option for disabling symbol visibility attributes
ce7923874 build: Add SECP256K1_NO_API_VISIBILITY_ATTRIBUTES
e5297f6d7 build: Refactor visibility logic
cbbbf3bd6 Merge bitcoin-core/secp256k1#1699: ci: enable musig module for native macOS arm64 job
943479a7a Merge bitcoin-core/secp256k1#1694: Revert "cmake: configure libsecp256k1.pc during install"
3352f9d66 ci: enable musig module for native macOS arm64 job
ad60ef7ea Merge bitcoin-core/secp256k1#1689: ci: Convert `arm64` Cirrus tasks to GHA jobs
c49877909 Merge bitcoin-core/secp256k1#1687: cmake: support the use of launchers in ctest -S scripts
44b205e9e Revert "cmake: configure libsecp256k1.pc during install"
0dfe387db cmake: support the use of launchers in ctest -S scripts
89096c234 Merge bitcoin-core/secp256k1#1692: cmake: configure libsecp256k1.pc during install
7106dce6f cmake: configure libsecp256k1.pc during install
29e73f4ba Merge bitcoin-core/secp256k1#1685: cmake: Emulate Libtool's behavior on FreeBSD
746e36b14 Merge bitcoin-core/secp256k1#1678: cmake: add a helper for linking into static libs
a28c2ffa5 Merge bitcoin-core/secp256k1#1683: README: add link to musig example
2a9d37473 Merge bitcoin-core/secp256k1#1690: ci: Bump GCC snapshot major version to 16
add146e10 ci: Bump GCC snapshot major version to 16
004f57fcd ci: Move Valgrind build for `arm64` from Cirrus to GHA
5fafdfc30 ci: Move `gcc-snapshot` build for `arm64` from Cirrus to GHA
e814b79a8 ci: Switch `arm64_debian` from QEMU to native `arm64` Docker image
bcf77346b ci: Add `arm64` architecture to `docker_cache` job
b77aae922 ci: Rename Docker image tag to reflect architecture
145ae3e28 cmake: add a helper for linking into static libs
819210974 README: add link to musig example, generalize module enabling hint
95db29b14 Merge bitcoin-core/secp256k1#1679: cmake: Use `PUBLIC_HEADER` target property in installation logic
37dd422b5 cmake: Emulate Libtool's behavior on FreeBSD
f24b838be Merge bitcoin-core/secp256k1#1680: doc: Promote "Building with CMake" to standard procedure
3f31ac43e doc: Promote "Building with CMake" to standard procedure
6f67151ee cmake: Use `PUBLIC_HEADER` target property
c32715b2a cmake, move-only: Move module option processing to `src/CMakeLists.txt`
201b2b8f0 Merge bitcoin-core/secp256k1#1675: cmake: Bump minimum required CMake version to 3.22
3af71987a cmake: Bump minimum required CMake version to 3.22
92394476e Merge bitcoin-core/secp256k1#1673: Assert field magnitude at control-flow join
3a4f448cb Assert field magnitude at control-flow join

git-subtree-dir: src/secp256k1
git-subtree-split: 36e76952cbf1cf54ddd2d8756cc31a486e2ba1d9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@real-or-random real-or-random real-or-random approved these changes

+1 more reviewer

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug side-channel

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@john-moffett @real-or-random @theStack