Quoting sipa (see https://github.com/bitcoin-core/secp256k1/pull/1479#discussion_r1790079414):
"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.
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.
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>
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>
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.
447334cb06 include: Avoid visibility("default") on Windows (Tim Ruffing)
Pull request description:
Fixes#1421. See code comments for rationale.
Related meta-bug: #1181. This reminds me that we should move forward with #1359.
ACKs for top commit:
fanquake:
ACK 447334cb06
hebasto:
ACK 447334cb06, tested on Ubuntu 24.04 using the following commands:
theuni:
ACK 447334cb06
Tree-SHA512: aaa47d88fd1b1f85c3e879a2b288c0eb3beebad0cc89e85f05d0b631f83e58d5a324fb441911970865eaa292f6820d03a1b516d6e8de37a87510e2082acc6e28
8be3839fb2 Remove unused scratch space from API (Jonas Nick)
Pull request description:
We had already merged this in #1305, but it was reverted before a release (#1311) because this change is not backwards compatible but at the time we only wanted to make a patch release in order to fix an actual issue.
Due to the musig module, the next release will increment the version number from 0.5.x to 0.6.0, so it would be a good time to remove the scratch space from the API.
ACKs for top commit:
sipa:
utACK 8be3839fb2
real-or-random:
utACK 8be3839fb2
Tree-SHA512: ecd6bc1d925992f9df8e26820388fc436bbb6bc5f250950edf00406f006ca0df52ab8cd56a1b7541e57af0682ddadf6d34bd638b27557d301a5dff6c327a5ebc
c232486d84 Revert "cmake: Set `ENVIRONMENT` property for examples on Windows" (Hennadii Stepanov)
26e4a7c214 cmake: Set top-level target output locations (Hennadii Stepanov)
Pull request description:
While testing https://github.com/bitcoin-core/secp256k1/pull/1551, I noticed that when cross-compiling a shared library with examples for Windows, the `ctest` fails to run examples with Wine. Adjusting the `PATH` variable in 4af241b320/examples/CMakeLists.txt (L16-L18) does not help because `WINEPATH` is expected.
Another issue with the current implementation is that the examples cannot run individually on Windows.
This PR resolves both issues by reverting the implementation from https://github.com/bitcoin-core/secp256k1/pull/1290 in favour of the reworked and improved implementation from https://github.com/bitcoin-core/secp256k1/pull/1233.
ACKs for top commit:
theuni:
Concept ACK and utACK c232486d84.
real-or-random:
utACK c232486d84
Tree-SHA512: 479b71d15d5d5670f6f69da3da599240c345711003383ca805c821b67065c9baaf269f987792cf1029211cdbfe799aecd401e6940a471539e3929b4a90e0781d
ef7ff03407 f can never equal -m (Russell O'Connor)
Pull request description:
In fact, before reaching this particular VERIFY_CHECK, we had already successfully passed through
VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(&f, len, &modinfo->modulus, -1) > 0); /* f > -modulus */
ensuring that f is not -m.
ACKs for top commit:
sipa:
ACK ef7ff03407
real-or-random:
utACK ef7ff03407
Tree-SHA512: a8a8dcbad4dff36b9c49e40e07b212312cbf915132aea008eab6ea61b35bddb6d7782229c2cc528fb404d05132482c602cad768414d76153bb425a3d23714fff
168c92011f build: allow enabling the musig module in cmake (Jonas Nick)
f411841a46 Add module "musig" that implements MuSig2 multi-signatures (BIP 327) (Jonas Nick)
0be79660f3 util: add constant-time is_zero_array function (Jonas Nick)
c8fbdb1b97 group: add ge_to_bytes_ext and ge_from_bytes_ext (Jonas Nick)
85e224dd97 group: add ge_to_bytes and ge_from_bytes (Jonas Nick)
Pull request description:
EDIT: based on #1518. Closes#1452. Most of the code is a copy from [libsecp256k1-zkp](https://github.com/BlockstreamResearch/secp256k1-zkp). The API added in this PR is identical with the exception of two modifications:
1. I removed the unused `scratch_space` argument from `secp256k1_musig_pubkey_agg`. This argument was intended to allow using `ecmult_multi` algorithms for key aggregation in the future. But at this point it's unclear whether the `scratch_space` object will remain in its current form (see #1302).
2. Support for adaptor signatures was removed and therefore the `adaptor` argument of `musig_nonce_process` was also removed.
In contrast to the module in libsecp256k1-zkp, the module is non-experimental. I slightly cleaned up parts of the module, adjusted the code to the new definition of the VERIFY_CHECK macro and applied some simplifications that were possible because the module is now in the upstream repo (`ge_from_bytes`, `ge_to_bytes`). You can follow the changes I made to the libsecp256k1-zkp module at https://github.com/jonasnick/secp256k1-zkp/commits/musig2-upstream/.
ACKs for top commit:
sipa:
reACK 168c92011f
real-or-random:
reACK 168c92011f
theStack:
re-ACK 168c92011f
Tree-SHA512: e3a599a8d5a466107b9a86f76582b8fb9dc87ec95416c784c3ef39d1c64686e6c739806ed6ba62c91793eb7fa418a6270cf999027ee7bd3dd85c67bc2c74f677
In fact, before reaching this particular VERIFY_CHECK, we had already successfully passed through
VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(&f, len, &modinfo->modulus, -1) > 0); /* f > -modulus */
ensuring that f is not -m.
This change:
1. Collects build artifacts in dedicated locations.
2. Allows to run individual examples with a shared library on Windows.
3. Is compatible with Wine when testing cross-compiled Windows binaries
on Linux.
4. Is compatible with integration the project into a larger project
hierarchy.
7c987ec89e cmake: Call `enable_testing()` unconditionally (Hennadii Stepanov)
6aa576515e cmake: Delete `CTest` module (Hennadii Stepanov)
Pull request description:
1. Delete `CTest` module.
The `CTest` module handles `CDash` integration, which we do not use. It is not required for testing functionality.
2. Clean up cases when to invoke `enable_testing()`
The `enable_testing()` command invocation is required for `add_test()` commands, which are used only for `{noverify_}tests`, `exhaustive_tests` and examples.
ACKs for top commit:
real-or-random:
utACK 7c987ec89e
theStack:
ACK 7c987ec89e
Tree-SHA512: f2628a4c4363b86f7c49d705df192e76368997e3f01cb736fcc272330bbf481af023d29d9faec17878d3bf74e4d265870cd93953406ae4612cd717811f22dd1a
292310fbb2 doc: fix typos in `secp256k1_ecdsa_{recoverable_,}signature` API description (Sebastian Falbesoner)
Pull request description:
This small PR fixes two small typos I noticed while looking at the pubkey recovery module (s/structured/structure/).
ACKs for top commit:
real-or-random:
utACK 292310fbb2
Tree-SHA512: 7abda2d5b56d810fd83c7de9b018818a28fd5969f669b53b9dc0e1034428feef63a3157714f45002185b94c6c35617cab21861fa56dba51883d1326b4a1cb033
Downstream projects may include libsecp test executables in their test
suites. In such cases, if a `-fsanitize=...` flag is passed via the
`SECP256K1_APPEND_CFLAGS` variable, the same flag must also be passed to
the linker.
fa67b6752d refactor: Use array initialization for unterminated strings (MarcoFalke)
e34b476730 ci: Bump GCC_SNAPSHOT_MAJOR to 15 (maflcko)
Pull request description:
Follow-up to https://github.com/bitcoin-core/secp256k1/pull/1313
Clang should silently follow the `main` devel branch, but GCC needs to be bumped manually.
ACKs for top commit:
hebasto:
ACK fa67b6752d, I have reviewed the code and it looks OK.
real-or-random:
utACK fa67b6752d
Tree-SHA512: e76371e5b1ff259ec501671872352c0d46d34a96aadae04e6ee37f9457308412e18010e724df667a15c3a85997a16da191f50cd3a01ee3f20d5f16b5893d179a
The previous code is correct and harmless to initialize an array with a
non-terminated character sequence using a string literal.
However, it requires exactly specifying the array size, which can be
cumbersome.
Also, GCC-15 may issue the -Wunterminated-string-initialization warning.
[1]
Fix both issues by using array initialization. This refactoring commit
does not change behavior.
[1] Example warning:
src/modules/schnorrsig/main_impl.h:48:46: error: initializer-string for array of 'unsigned char' is too long [-Werror=unterminated-string-initialization]
48 | static const unsigned char bip340_algo[13] = "BIP0340/nonce";
| ^~~~~~~~~~~~~~~
7057d3c9af ci: Silent Homebrew's noisy reinstall warnings (Hennadii Stepanov)
Pull request description:
Homebrew's warnings are quite noisy on the master branch:
This PR silents them to allow us to focus on any other CI infra warnings once they happen.
ACKs for top commit:
real-or-random:
utACK 7057d3c9af
Tree-SHA512: 4dbdb459e97cb5956219214fd7407ec52fc018dba24ae945337ad5c94df510472cfc708f9f67d4df5b8e983bf4e78b2efd56533f28113b3463a1397d4d7a032a
763d938cf0 ci: only enable extrakeys module when schnorrsig is enabled (Jonas Nick)
af551ab9db tests: do not use functions from extrakeys module (Jonas Nick)
Pull request description:
ACKs for top commit:
sipa:
ACK 763d938cf0
hebasto:
ACK 763d938cf0.
Tree-SHA512: a5d52e69af896e7bb43c6826f269b5f26e8d1b8c0d040d71a369fdaebf894e47dcf9ff1bbe1b62864adb85437d2c93eabd655c4d690d5d894b712145c2bd252f
