7e3952ae82af2a98619de74614f2d3f0ff072941 Clarify documentation of tweak functions. (Jonas Nick)
89853a0f2e2b324bd32e78641aeaad6d1e427e81 Make tweak function documentation more consistent. (Jonas Nick)
41fc78560223aa035d9fe2cbeedb3ee632c740e2 Make ec_privkey functions aliases for ec_seckey_negate, ec_seckey_tweak_add and ec_seckey_mul (Jonas Nick)
22911ee6da7197c45226ab4bcd84b8c2773baf9f Rename private key to secret key in public API (with the exception of function names) (Jonas Nick)
5a73f14d6cfab043f94b2032d6958e15f84065fe Mention that value is unspecified for In/Out parameters if the function returns 0 (Jonas Nick)
f03df0e6d7d272808c9d27df40bb92716b341d03 Define valid ECDSA keys in the documentation of seckey_verify (Jonas Nick)
5894e1f1df74b23435cfd1e9a8b25f22ac631ebe Return 0 if the given seckey is invalid in privkey_negate, privkey_tweak_add and privkey_tweak_mul (Jonas Nick)
8f814cddb94f4018646569143d10ebbb98daa454 Add test for boundary conditions of scalar_set_b32 with respect to overflows (Jonas Nick)
3fec9826086aa45ebbac1ff6fc3bb7b25ca78b1d Use scalar_set_b32_seckey in ecdsa_sign, pubkey_create and seckey_verify (Jonas Nick)
9ab2cbe0ebf8dfafd5499ea7a79c35f0d0cfe5e2 Add scalar_set_b32_seckey which does the same as scalar_set_b32 and also returns whether it's a valid secret key (Jonas Nick)
Pull request description:
Fixes#671. Supersedes #668.
This PR unifies handling of invalid secret keys by introducing a new function `scalar_set_b32_secret` which returns false if the b32 overflows or is 0. By using this in `privkey_{negate, tweak_add, tweak_mul}` these function will now return 0 if the secret key is invalid which matches the behavior of `ecdsa_sign` and `pubkey_create`.
Instead of deciding whether to zeroize the secret key on failure, I only added documentation for now that the value is undefined on failure.
ACKs for top commit:
real-or-random:
ACK 7e3952ae82af2a98619de74614f2d3f0ff072941 I read the diff carefully and tested the changes
apoelstra:
ACK 7e3952ae82af2a98619de74614f2d3f0ff072941
Tree-SHA512: 8e9a66799cd3b6ec1c3acb731d6778035417e3dca9300d840e2437346ff0ac94f0c9be4de20aa2fac9bb4ae2f8a36d4e6a34795a640b9cfbfee8311decb102f0
37ed51a7eafa5fd9955ceb66213251cdd8b8b8c4 Make ecdsa_sig_sign constant-time again after reverting 25e3cfb (Tim Ruffing)
93d343bfc5323e56f6a60cb41d60b96368cc09c7 Revert "ecdsa_impl: replace scalar if-checks with VERIFY_CHECKs in ecdsa_sig_sign" (Tim Ruffing)
Pull request description:
ACKs for top commit:
elichai:
ACK 37ed51a7eafa5fd9955ceb66213251cdd8b8b8c4 makes sense.
jonasnick:
ACK 37ed51a7eafa5fd9955ceb66213251cdd8b8b8c4
Tree-SHA512: 82b5b8e29f48e84fd7a0681b62923d3bd87d724b38ef18e8c7969b0dcc5a405ebb26c14b5c5f4c7ba0ccabd152d1531d217809d1daf40872fe0c1e079b55c64b
4e284655d99c8f0ee030aa29bcd036e5e5bd2a40 Fix typo in ecmult_const_impl.h (f-daniel)
Pull request description:
Fix small typo in the reference given for the wNAF method ( `secp256k1_wnaf_const`)
ACKs for top commit:
real-or-random:
ACK 4e284655d99c8f0ee030aa29bcd036e5e5bd2a40 trivial
Tree-SHA512: d6c3daa0384fc1bba36a46933641c97661b18e88c711343e2c8f91f39aa707eddd0ba77c8d5c43aaead883eeed7f4458ed1dec228d692d713572231aa6010fb0
ffef45c98a55934ab555ecf96f0b19d0e4cfe830 Make recovery/main_impl.h non-executable (Elichai Turkel)
Pull request description:
Opened it because of https://github.com/bitcoin/bitcoin/pull/18650
I assume it doesn't matter?
But because I'm not sure I preferred to open this then let the info go away in case someone thinks it does matter.
ACKs for top commit:
real-or-random:
ACK ffef45c98a55934ab555ecf96f0b19d0e4cfe830
Tree-SHA512: 381aed7f99fd739f4059b2e526ba9cd75b55b4fa86c9cc040fbf6b93055ce8558cc69c4ccf5d8a422b17022ca376cc9a608cf5af8d5841d62c5953f40825f5ff
3b7d26b23c9811deee244ede7bb344bb43544153 build: add SECP_TEST_INCLUDES to bench_verify CPPFLAGS (fanquake)
84b5fc5bc34cfb6e9df4f06b3b6fc98c3373dd0e build: fix OpenSSL EC detection on macOS (fanquake)
Pull request description:
ACKs for top commit:
real-or-random:
ACK 3b7d26b23c9811deee244ede7bb344bb43544153 the diff looks good to me
jonasnick:
ACK 3b7d26b23c9811deee244ede7bb344bb43544153
Tree-SHA512: 381aed7f99fd739f4059b2e526ba9cd75b55b4fa86c9cc040fbf6b93055ce8558cc69c4ccf5d8a422b17022ca376cc9a608cf5af8d5841d62c5953f40825f5ff
This reverts commit 25e3cfbf9b52d2f5afa543f967a73aa8850d2038. The reverted
commit was probably based on the assumption that this is about the touched
checks cover the secret nonce k instead of r, which is the x-coord of the public
nonce. A signature with a zero r is invalid by the spec, so we should return 0
to make the caller retry with a different nonce. Overflow is not an issue.
Fixes#720.
01993878bb2e7f24f42dac84d6949242143bb7f8 Add test for memczero() (Tim Ruffing)
52a03512c1d800603b5c923c1a28bdba12dadb30 Suppress a harmless variable-time optimization by clang in memczero (Tim Ruffing)
Pull request description:
ACKs for top commit:
jonasnick:
ACK 01993878bb2e7f24f42dac84d6949242143bb7f8
Tree-SHA512: ed385f6e3909299b9979254bd0208a9d0c68caa9f57039e392aa0d5424ed8002c13f8c037bfbd2697c2f6b54af5731209eba9725c5e88be3ee3077c159d134e2
This has been not been caught by the new constant-time tests because
valgrind currently gives us a zero exit code even if finds errors, see
https://github.com/bitcoin-core/secp256k1/pull/723#discussion_r388246806 .
This commit also simplifies the arithmetic in memczero.
Note that the timing leak here was the bit whether a secret key was
out of range. This leak is harmless and not exploitable. It is just
our overcautious practice to prefer constant-time code even here.
85b35afa765ba23ac3682cf15800769b0a3b834d Add running benchmarks regularly and under valgrind in travis (Elichai Turkel)
ca4906b02e69644d83e13b3c09cc9147fc1232a5 Pass num of iters to benchmarks as variable, and define envvar (Elichai Turkel)
02dd5f1bbb3af3660ecff276c3a108371979b67c free the ctx at the end of bench_ecdh (Elichai Turkel)
Pull request description:
ACKs for top commit:
real-or-random:
ACK 85b35afa765ba23ac3682cf15800769b0a3b834d I looked at the diff and tested the ecdh benchmark using valgrind
jonasnick:
ACK 85b35afa765ba23ac3682cf15800769b0a3b834d
Tree-SHA512: 029456d2c8f6c2c45c689279683ae30b067872bbfaee76a657f7fc3a059e2dffcde09ed29e3610de3adb055405126b6c3656c7ab5f5aaa1f45af2b32d4693ec4
ca739cba238cdd7c513abfc719b0b0eb957c9458 Compile with optimization flag -O2 by default instead of -O3 (Jonas Nick)
83fb1bcef49b1c12ef349f62d90bfcc83f0f7398 Remove -O2 from default CFLAGS because this would override the -O3 flag (see AC_PROG_CC in the Autoconf manual) (Jonas Nick)
ecba8138ec163f5ad4c303df9f8744810a3dfc03 Append instead of Prepend user-CFLAGS to default CFLAGS allowing the user to override default variables (Jonas Nick)
613c34cd869e56dee2ea5fb701f05b07da7069c8 Remove test in configure.ac because it doesn't have an effect (Jonas Nick)
Pull request description:
Right now, it's not easy to reduce the optimization level with `CFLAGS` because `configure` overwrites any optimization flag with `-O3`. The [automake documentation](https://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html) states that:
> The reason ‘$(CPPFLAGS)’ appears after ‘$(AM_CPPFLAGS)’ or ‘$(mumble_CPPFLAGS)’ in the compile command is that users should always have the last say.
and also that it's incorrect to redefine CFLAGS in the first place
> You should never redefine a user variable such as CPPFLAGS in Makefile.am. [...] You should not add options to these user variables within configure either, for the same reason
With this PR `CFLAGS` is still redefined, but user-provided flags appear after the default `CFLAGS` which means that they override the default flags (at least in clang and gcc). Otherwise, the default configuration is not changed. This also means that if CFLAGS are defined by the user, then -g is not added (which does not seem to make much sense). In order to keep the `-O3` despite the reordering we need to explicitly tell autoconf to not append `-O2` by setting the default to `-g` with `: ${CFLAGS="-g"}` as per [the manual](https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/autoconf.html#C-Compiler) (EDIT: link fix).
ACKs for top commit:
real-or-random:
ACK ca739cba238cdd7c513abfc719b0b0eb957c9458
theuni:
ACK ca739cba238cdd7c513abfc719b0b0eb957c9458.
elichai:
ACK ca739cba238cdd7c513abfc719b0b0eb957c9458
Tree-SHA512: be92589faa461d245203385d44b489c7d6917b0c68472b8d7576806c0250cf5ff61d5c99ce04eebb8ff5279b9987185d4e5d2da979683fb1c489fdf3e5b59630
08fb6c49261f8aefaaa8ea2ca6d84a53e037e812 Run valgrind_ctime_test in travis (Jonas Nick)
3d2302257f19533932cd53547e9745b6283a907d Constant-time behaviour test using valgrind memtest. (Gregory Maxwell)
Pull request description:
Valgrind does bit-level tracking of the "uninitialized" status of memory,
property tracks memory which is tainted by any uninitialized memory, and
warns if any branch or array access depends on an uninitialized bit.
That is exactly the verification we need on secret data to test for
constant-time behaviour. All we need to do is tell valgrind our
secret key is actually uninitialized memory.
This adds a valgrind_ctime_test which is compiled if valgrind is installed:
Run it with libtool --mode=execute:
$ libtool --mode=execute valgrind ./valgrind_ctime_test
ACKs for top commit:
sipa:
ACK 08fb6c49261f8aefaaa8ea2ca6d84a53e037e812
real-or-random:
ACK 08fb6c49261f8aefaaa8ea2ca6d84a53e037e812
jonasnick:
ACK 08fb6c49261f8aefaaa8ea2ca6d84a53e037e812
Tree-SHA512: d2eb829fb09f43ad1af70898e0eb9cf3f002c6bc418eca9e3e01a9c2c6e87c092aed23d6b0f311ddccbce1cce5f8ef39162cf9b2e68b83d160bc3d249e881493
Valgrind does bit-level tracking of the "uninitialized" status of memory,
property tracks memory which is tainted by any uninitialized memory, and
warns if any branch or array access depends on an uninitialized bit.
That is exactly the verification we need on secret data to test for
constant-time behaviour. All we need to do is tell valgrind our
secret key is actually uninitialized memory.
This adds a valgrind_ctime_test which is compiled if valgrind is installed:
Run it with libtool --mode=execute:
$ libtool --mode=execute valgrind ./valgrind_ctime_test
7b50483ad789081ba158799e5b94330f62932607 Adds a declassify operation to aid constant-time analysis. (Gregory Maxwell)
34a67c773b0871e5797c7ab506d004e80911f120 Eliminate harmless non-constant time operations on secret data. (Gregory Maxwell)
Pull request description:
There were several places where the code was non-constant time
for invalid secret inputs. These are harmless under sane use
but get in the way of automatic const-time validation.
(Nonce overflow in signing is not addressed, nor is s==0 in signing)
ACKs for top commit:
sipa:
utACK 7b50483ad789081ba158799e5b94330f62932607
real-or-random:
ACK 7b50483ad789081ba158799e5b94330f62932607 I read the code carefully and tested it
jonasnick:
reACK 7b50483ad789081ba158799e5b94330f62932607
Tree-SHA512: 0776c3a86e723d2f97b9b9cb31d0d0e59dfcf308093b3f46fbc859f73f9957f3fa977d03b57727232040368d058701ef107838f9b1ec98f925ec78ddad495c4e
eb45ef33842ead425137d589521dc231ee92a10d Clarify that a secp256k1_ecdh_hash_function must return 0 or 1 (Tim Ruffing)
Pull request description:
and improve style of the ECDH docs.
ACKs for top commit:
sipa:
utACK eb45ef33842ead425137d589521dc231ee92a10d
jonasnick:
ACK eb45ef33842ead425137d589521dc231ee92a10d
elichai:
ACK eb45ef33842ead425137d589521dc231ee92a10d
apoelstra:
utACK eb45ef3384
Tree-SHA512: fa1e34fbbe2fd53b633c48c70fbd9d6eec4be1303b660ff87945d49333264ef5c28a4db9407161907697f37ca657a1ee7b50e58861689de526ad4d685dedeae6
ECDSA signing has a retry loop for the exceptionally unlikely case
that S==0. S is not a secret at this point and this case is so
rare that it will never be observed but branching on it will trip
up tools analysing if the code is constant time with respect to
secrets.
Derandomized ECDSA can also loop on k being zero or overflowing,
and while k is a secret these cases are too rare (1:2^255) to
ever observe and are also of no concern.
This adds a function for marking memory as no-longer-secret and
sets it up for use with the valgrind memcheck constant-time
test.
There were several places where the code was non-constant time
for invalid secret inputs. These are harmless under sane use
but get in the way of automatic const-time validation.
(Nonce overflow in signing is not addressed, nor is s==0 in
signing)
4b48a431060948dc5e29aa590d646a72aa138968 doc: document the length requirements of output parameter. (Rusty Russell)
Pull request description:
It's subtle, since it is actually only touched by hashfp (though
we assert it's non-NULL), but give explicit advice in the default
case.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
ACKs for top commit:
jonasnick:
ACK 4b48a431060948dc5e29aa590d646a72aa138968
real-or-random:
ACK 4b48a431060948dc5e29aa590d646a72aa138968 diff inspection
Tree-SHA512: d6bedb495e46b27ac9b558e77d814884d782ea78569a2296688eccf374bc880d13846546ad449c2a677865cf6ed56fcbc8be58c21f9daca5084831074e20d769
642cd062bdd2d28a8a84d4cb6dedbfe435ee5869 Remove Java Native Interface (Jonas Nick)
Pull request description:
This was discussed in #508. The main reasons are that the existing Java Native Interface (JNI) bindings would need way more work to remain useful to Java developers but the maintainers and regular contributors of libsecp are not very familiar with Java (and evidently are motivated enough to improve the situation). We don't know who relies on these bindings with the exception of ACINQ who have their own fork at https://github.com/ACINQ/secp256k1/tree/jni-embed/src/java (@sstone). Bitcoinj can optionally use the libsecp bindings.
Ideally, there would be a separate repository owned by Java developers with just the bindings. Until this exists, Java developers relying on libsecp can use ACINQs fork or an older commit of libsecp.
ACKs for top commit:
real-or-random:
ACK 642cd062bdd2d28a8a84d4cb6dedbfe435ee5869 I read the diff
real-or-random:
ACK 642cd062bdd2d28a8a84d4cb6dedbfe435ee5869 I read the diff, and I verified that the diff to 7d9066a66c0f13cabb0c4f71aca30edd3494f0d5, which has been ACKed by sipa, is only the additonal removal of ax_jni_include_dir.m4
Tree-SHA512: 9e573f2b01897bd5f301707062b41de53424517b537ce0834d9049d003cfd73fa1bcc024b543256016e4c9a1126f7c7fef559b84dc4914083b5a2d0ad5e57ea8
It's subtle, since it is actually only touched by hashfp (though
we assert it's non-NULL), but give explicit advice in the default
case.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
acb7f97eb82dfbbdb797354e1550b910055b4422 README: add a section for test coverage (Marko Bencun)
Pull request description:
It is a hassle to figure out the exact commands to create a good
report.
ACKs for top commit:
real-or-random:
ACK acb7f97eb82dfbbdb797354e1550b910055b4422
sipa:
ACK acb7f97eb82dfbbdb797354e1550b910055b4422
Tree-SHA512: d39f3e0b289229b2ce085406f6d716fdd54038df9ee5273a18a05140d1eddd4149149e881cc7a13f2126347217b9c56a0c12adf558c49879c5f556695242afc6
d567b779fe446fd18820a9d2968ecb703c8dea19 Clarify comments about use of rzr on ge functions and abs function. (Gregory Maxwell)
2241ae6d14df187e2c8d6fe5b44e3d850474af38 Remove secret-dependant non-constant time operation in ecmult_const. (Gregory Maxwell)
Pull request description:
ECMULT_CONST_TABLE_GET_GE was branching on its secret input.
Also makes secp256k1_gej_double_var implemented as a wrapper
on secp256k1_gej_double_nonzero instead of the other way
around. This wasn't a constant time bug but it was fragile
and could easily become one in the future if the double_var
algorithm is changed.
ACKs for top commit:
real-or-random:
ACK d567b779fe446fd18820a9d2968ecb703c8dea19 I read the diff carefully and tested the code with ECDH enabled and various settings, also on valgrind
sipa:
ACK d567b779fe446fd18820a9d2968ecb703c8dea19
Tree-SHA512: f00a921dcc6cc024cfb3ac1a34c1be619b96f1f17ec0ee0f3ff4ea02035ee288e55469491ed3183e2c4e5560cc068c10aafb657dff95a610706e5b9a8cd13966
ECMULT_CONST_TABLE_GET_GE was branching on its secret input.
Also makes secp256k1_gej_double_var implemented as a wrapper
on secp256k1_gej_double_nonzero instead of the other way
around. This wasn't a constant time bug but it was fragile
and could easily become one in the future if the double_var
algorithm is changed.
