Upstream PRs 1579, 1631, 1633, 1634, 1641, 1650, 1646, 1654#322
Merged
real-or-random merged 31 commits intoBlockstreamResearch:masterfrom Feb 20, 2026
Merged
Upstream PRs 1579, 1631, 1633, 1634, 1641, 1650, 1646, 1654#322real-or-random merged 31 commits intoBlockstreamResearch:masterfrom
real-or-random merged 31 commits intoBlockstreamResearch:masterfrom
Conversation
This code is not supposed to handle secret data.
We rely on memset() and an __asm__ memory barrier where it's available or on SecureZeroMemory() on Windows. The fallback implementation uses a volatile function pointer to memset which the compiler is not clever enough to optimize.
There are two uses of the secp256k1_fe_clear() function that are now separated
into these two functions in order to reflect the intent:
1) initializing the memory prior to being used -> converted to fe_set_int( . , 0 )
2) zeroing the memory after being used such that no sensitive data remains. ->
remains as fe_clear()
In the latter case, 'magnitude' and 'normalized' need to be overwritten when
VERIFY is enabled.
Co-Authored-By: isle2983 <isle2983@yahoo.com>
Co-Authored-By: isle2983 <isle2983@yahoo.com> Co-Authored-By: Pieter Wuille <pieter.wuille@gmail.com>
All of the invocations of secp256k1_memclear() operate on stack memory and happen after the function is done with the memory object. This commit replaces existing memset() invocations and also adds secp256k1_memclear() to code locations where clearing was missing; there is no guarantee that this commit covers all code locations where clearing is necessary. Co-Authored-By: isle2983 <isle2983@yahoo.com>
This gives the caller more control about whether the state should be cleaned (= should be considered secret). Moreover, it gives the caller the possibility to clean a hash struct without finalizing it.
Quoting sipa (see bitcoin-core/secp256k1#1479 (comment)): "When performing an EC multiplication A = aG for secret a, the resulting _affine_ coordinates of A are presumed to not leak information about a (ECDLP), but the same is not necessarily true for the Jacobian coordinates that come out of our multiplication algorithm." For the ECDH point multiplication result, the result in Jacobi coordinates should be cleared not only to avoid leaking the scalar, but even more so as it's a representation of the resulting shared secret.
…ting optimized out (revival of #636) 765ef53 Clear _gej instances after point multiplication to avoid potential leaks (Sebastian Falbesoner) 349e6ab Introduce separate _clear functions for hash module (Tim Ruffing) 99cc9fd Don't rely on memset to set signed integers to 0 (Tim Ruffing) 97c57f4 Implement various _clear() functions with secp256k1_memclear() (Tim Ruffing) 9bb368d Use secp256k1_memclear() to clear stack memory instead of memset() (Tim Ruffing) e3497bb Separate between clearing memory and setting to zero in tests (Tim Ruffing) d79a6cc Separate secp256k1_fe_set_int( . , 0 ) from secp256k1_fe_clear() (Tim Ruffing) 1c08126 Add secp256k1_memclear() for clearing secret data (Tim Ruffing) e7d3844 Don't clear secrets in pippenger implementation (Tim Ruffing) Pull request description: This PR picks up #636 (which in turn picked up #448, so this is take number three) and is essentially a rebase on master. Some changes to the original PR: * the clearing function now has the `secp256k1_` prefix again, since the related helper `_memczero` got it as well (see PR #835 / commit e89278f) * the original commit b17a7df8145a6a86d49c354c7e7b59a432ea5346 ("Make _set_fe_int( . , 0 ) set magnitude to 0") is not needed anymore, since it was already applied in PR #943 (commit d49011f) * clearing of stack memory with `secp256k1_memclear` is now also done on modules that have been newly introduced since then, i.e. schnorr and ellswift (of course, there is still no guarantee that all places where clearing is necessary are covered) So far I haven't looked at any disassembly and possible performance implications yet (there were some concerns expressed in bitcoin-core/secp256k1#636 (comment)), happy to go deeper there if this gets Concept ACKed. The proposed method of using a memory barrier to prevent optimizating away the memset is still used in BoringSSL (where it was originally picked up from) and in the Linux Kernel, see e.g. https://github.com/google/boringssl/blob/5af122c3dfc163b5d1859f1f450756e8e320a142/crypto/mem.c#L335 and https://github.com/torvalds/linux/blob/d4560686726f7a357922f300fc81f5964be8df04/include/linux/string.h#L348 / https://github.com/torvalds/linux/blob/d4560686726f7a357922f300fc81f5964be8df04/include/linux/compiler.h#L102 Fixes BlockstreamResearch#185. ACKs for top commit: sipa: reACK 765ef53 real-or-random: ACK 765ef53 Tree-SHA512: 5a034d5ad14178c06928022459f3d4f0877d06f576b24ab07b86b3608b0b3e9273217b8309a1db606f024f3032731f13013114b1e0828964b578814d1efb2959
39d5dfd release: prepare for 0.6.0 (Jonas Nick) df2eceb build: add ellswift.md and musig.md to release tarball (Jonas Nick) a306bb7 tools: fix check-abi.sh after cmake out locations were changed (Jonas Nick) 145868a Do not export `secp256k1_musig_nonce_gen_internal` (Hennadii Stepanov) Pull request description: ACKs for top commit: sipa: utACK 39d5dfd real-or-random: ACK 39d5dfd mod the CI results Tree-SHA512: 9b4623ca03aafcd1e04b0809382faeb3b427d3d07062f065177c7608e4feb30abd52cb10fa8c06b7ae17a82b32455e995b6bd39e3ef6239d5fc65c78873385b0
…r 0.6.0 c97059f release cleanup: bump version after 0.6.0 (Jonas Nick) Pull request description: ACKs for top commit: sipa: utACK c97059f hebasto: ACK c97059f. real-or-random: utACK c97059f Tree-SHA512: 2fccf8b4647f04397066410f7daf565a015aa7f1e20c9aa7151af3c47e7140dedf3032f5e79909b2705bb09f27af30d41485a7cbf0675725f7cf221c6c537d9b
Some files contained English misspellings or math issues (`lamba` instead of `lambda`).
3970545 Fix some misspellings (Nicolas Iooss) Pull request description: Hello, Some files contained English misspellings or math issues (`lamba` instead of `lambda`), mainly in comments. Fixing them helps readability. By the way, the misspellings found in the Wycheproof test vector file were also reported upstream: C2SP/wycheproof#124 ACKs for top commit: real-or-random: utACK 3970545 Tree-SHA512: 36327e8bb58ef3c0408cf4966bb33f51c84b1614809d8711d86eaf3d4e5336ae8c663593cb5f0e9c56adbb2d7f2ca62a9b84cae1b76b9811c110f87f1defa624
… README 2ac9f55 doc: Improve cmake instructions in README (Fabian Jahr) Pull request description: Minor improvement suggestion for the readme. I find this alternative way of using cmake a bit more comfortable because I don't like to change the directory. It's just a suggestion based on personal preference, if this is too minor of an improvement feel free to close. ACKs for top commit: hebasto: ACK 2ac9f55. real-or-random: utACK 2ac9f55 Tree-SHA512: 5f7bc8b5ff91fb7a115a0e57224c66b018cfc824784e0def1064d07f9be66efe55e1a71e034f6a3d6489e063995c1ae17a9e91c990a0944d600cc957c038909d
…t key in BIP-340 nonce function a82287f schnorrsig: clear out masked secret key in BIP-340 nonce function (Sebastian Falbesoner) Pull request description: ACKs for top commit: real-or-random: utACK a82287f jonasnick: ACK a82287f Tree-SHA512: 0e77ddc299e204edae238759e549d4e8314abb730a654580a109ec05daf53a625be1cc37664a9e00fd41cf34a94abede96b547e661cdf18c40c50141e7b4ee0e
…ying GPG signatures b682dbc README: add instructions for verifying GPG signatures (James O'Beirne) Pull request description: ACKs for top commit: sipa: ACK b682dbc jonasnick: ACK b682dbc Tree-SHA512: 77ec0014e1a98e13ef38537177ea10175f064e7314e41474cd13a9c95c734ae1cca09effa2e2184a8c1495f3621e418d0df098fde4890d972d914cd7e80aa2d7
Fixes issue #1609.
…umbers for indicating program execution status 13d3896 CONTRIBUTING: mention that `EXIT_` codes should be used (Sebastian Falbesoner) c855581 test, bench, precompute_ecmult: use `EXIT_...` constants for `main` return values (Sebastian Falbesoner) 965393f examples: use `EXIT_...` constants for `main` return values (Sebastian Falbesoner) Pull request description: This simple PR addresses #1609 for all example and internal binaries. Alternative to #1618, which is stale (the author confirmed to me that they are not working on that PR anymore). The last commits adds a suggestion to CONTRIBUTING.md, not sure though if we want to go that far. ACKs for top commit: jonasnick: ACK 13d3896 real-or-random: utACK 13d3896 Tree-SHA512: 513eba4b712ba3d5f23a5fdc51cb27c5347b29bcaba39501345913c220be6f093a41186911032d2ddc898b848de84f05f374b3554ffcf92610728b2a23c0bb36
Collaborator
|
I think bitcoin-core/secp256k1#1654 should also be ported. We have zkp-specific executable (at least the bppp benchmark comes to my mind). |
Contributor
Author
The #1654 zkp-specific changes are already included, they were applied directly in the merge commit since the changes are minimal and straightforward. Should I extract the #1654 changes into a separate commit for consistency? And, does everything else look good? |
real-or-random
merged commit 2d30d39
into
BlockstreamResearch:master
106 of 116 checks passed
Collaborator
Sorry, I didn't expect this since you had another explicit "porting" commit. I think either way is fine in the end, but we should decide on one consistent way to do it in the future. I believe a separate commit is slightly cleaner. Changes in merge commits can be weird to work with, and separate commits are also cleaner when it comes to authorship information. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge bitcoin-core/secp256k1#1579: Clear sensitive memory without getting optimized out (revival of #636)
Merge bitcoin-core/secp256k1#1631: release: prepare for 0.6.0
Merge bitcoin-core/secp256k1#1633: release cleanup: bump version after 0.6.0
Merge bitcoin-core/secp256k1#1634: Fix some misspellings
Merge bitcoin-core/secp256k1#1641: doc: Improve cmake instructions in README
Merge bitcoin-core/secp256k1#1650: schnorrsig: clear out masked secret key in BIP-340 nonce function
Merge bitcoin-core/secp256k1#1646: README: add instructions for verifying GPG signatures
Merge bitcoin-core/secp256k1#1654: use
EXIT_constants over magic numbers for indicating program execution statusThis PR can be recreated with
./contrib/sync-upstream.sh -b master range c0d9480.Tips:
git show --remerge-diff <pr-branch>to show the conflict resolution in the merge commit.git read-tree --reset -u <pr-branch>to replay these resolutions during the conflict resolution stage when recreating the PR branch locally.Be aware that this may discard your index as well as the uncommitted changes and untracked files in your worktree.
zkp fixes for Valgrind compatibility
PR #1579 introduced
secp256k1_memclearthat marks cleared memory as undefined. Updated rangeproof to usememset/secp256k1_scalar_set_intfor initialization, keepingmemclearonly for cleanup.