765ef53335 Clear _gej instances after point multiplication to avoid potential leaks (Sebastian Falbesoner)
349e6ab916 Introduce separate _clear functions for hash module (Tim Ruffing)
99cc9fd6d0 Don't rely on memset to set signed integers to 0 (Tim Ruffing)
97c57f42ba Implement various _clear() functions with secp256k1_memclear() (Tim Ruffing)
9bb368d146 Use secp256k1_memclear() to clear stack memory instead of memset() (Tim Ruffing)
e3497bbf00 Separate between clearing memory and setting to zero in tests (Tim Ruffing)
d79a6ccd43 Separate secp256k1_fe_set_int( . , 0 ) from secp256k1_fe_clear() (Tim Ruffing)
1c08126222 Add secp256k1_memclear() for clearing secret data (Tim Ruffing)
e7d384488e 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 e89278f211)
* 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 d49011f54c)
* 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 https://github.com/bitcoin-core/secp256k1/pull/636#issuecomment-620118629), 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. 5af122c3df/crypto/mem.c (L335) and d456068672/include/linux/string.h (L348) / d456068672/include/linux/compiler.h (L102)Fixes#185.
ACKs for top commit:
sipa:
reACK 765ef53335
real-or-random:
ACK 765ef53335
Tree-SHA512: 5a034d5ad14178c06928022459f3d4f0877d06f576b24ab07b86b3608b0b3e9273217b8309a1db606f024f3032731f13013114b1e0828964b578814d1efb2959
0f73caf7c6 test, ci: Lower default iteration count to 16 (Hennadii Stepanov)
Pull request description:
The number of test iterations in the CI remains the same.
Resolves https://github.com/bitcoin-core/secp256k1/issues/1561.
```
$ ./build/src/tests
test count = 16
random seed = 59ea2b21267ec0ef0b4d13821292489f
random run = 2936c044f82c7598a866869b9d954d42
no problems found
```
ACKs for top commit:
sipa:
utACK 0f73caf7c6
jonasnick:
ACK 0f73caf7c6
Tree-SHA512: 84b265dc5d2780b3ea0a38f50ac8871d850ef2c97f33a0a5816baf20ac71c01db8b85696b343b089d7116d9cdb9450a6ca668229d95e54a39920d0e91a3127b3
The number of test iterations in the CI remains unchanged.
Additionally, the minimum iteration counts to enable the
`test_ecmult_constants_2bit` test is adjusted from 35 to 16, so it is
run by default.
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.
We should anyway prefer to use the predefined macros from <inttypes.h>.
If I haven't missed anything, this removes the last OS-specific #if,
leaving us only with compiler-specific #if(def)s.
57eda3ba30 musig: ctimetests: fix _declassify range for generated nonce points (Sebastian Falbesoner)
Pull request description:
As noticed in https://github.com/bitcoin-core/secp256k1/pull/1614#discussion_r1796215582, the area marked as non-secret exceeds the nonce_pts array in the second iteration of the for loop. Fix that by passing the correct size to the _declassify call.
ACKs for top commit:
sipa:
utACK 57eda3ba30
real-or-random:
utACK 57eda3ba30
Tree-SHA512: ff8074e3d1078d66a52d08c661997856ff586b3b4564a865a75212b32fafd7906d58885371bd63005007fde554ebcad121ab66125abe4331cf0aac63fc018ed0
The area marked as non-secret exceeds the nonce_pts array in the
second iteration of the for loop. Fix that by passing the correct
size to the _declassify call.
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
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.
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";
| ^~~~~~~~~~~~~~~
Note that the already existing function `random_fe_magnitude` is removed
and the call-sites are adapted to pass the magnitude range of 8
(the maximum for secp256k1_fe_mul and secp256k1_fe_sqr) explicitly.
Function secp256k1_ecmult_multi_var expects to be called with a non-NULL
error_callback parameter. Fix the invocation in test_ecmult_accumulate
to do this.
While at it, wrap the call in a CHECK macro to ensure it succeeds.
Fixes: https://github.com/bitcoin-core/secp256k1/issues/1527
The existing code needs to deal with the edge case that bit_pos >= 256,
which would lead to an out-of-bounds read from secp256k1_scalar.
Instead, recode the scalar into an array of uint32_t with enough zero
padding at the end to alleviate the issue. This also simplifies the
code, and is necessary for a security improvement in a follow-up
commit.
Original code by Peter Dettman, with modifications by Pieter Wuille.
This introduces the signed-digit multi-comb multiplication algorithm
for constant-time G multiplications (ecmult_gen). It is based on
section 3.3 of "Fast and compact elliptic-curve cryptography" by
Mike Hamburg (see https://eprint.iacr.org/2012/309).
Original implementation by Peter Dettman, with changes by Pieter Wuille
to use scalars for recoding, and additional comments.
