f00d6575ca remove dead store in ecdsa_signature_parse_der_lax (fanquake)
Pull request description:
ACKs for top commit:
elichai:
utACK f00d6575ca, it does look like we don't use that assignment
jonasnick:
ACK f00d6575ca
Tree-SHA512: 9aa54c901f299341c309411b0247720f5152a131dd346c19be7ee21865e3a822e8cf91b869e28ef6288adaf31660bc2e18874e304052468a9be6b7027674af30
2e7fc5b537 Fix uninitialized variables in ecmult_multi test (Jonas Nick)
Pull request description:
Fixes#756
ACKs for top commit:
real-or-random:
ACK 2e7fc5b537 I inspected the diff. I did not test it and I did not check whether if makes the warning go away
elichai:
tACK 2e7fc5b537
Tree-SHA512: 674400134f5487236f5b6e8b3020b346d43662511628cdf6dd1bd7ba1de985bf93f5be11f5650f250ff37b5f87eb4b01d90ed53d41193c05a420d3f5a2d63470
28609507e7 Add tests for the cmov implementations (Elichai Turkel)
73596a85a2 Add ecdsa_sign_recoverable to the ctime tests (Elichai Turkel)
2876af4f8d Split ecdsa_sign logic into a new function and use it from ecdsa_sign and recovery (Elichai Turkel)
Pull request description:
Hi,
The recovery module was overlooked in #708 and #710, so this adds it to the `valgrind_ctime_test` and replaces the secret dependent branching with the cmovs,
I created a new function `secp256k1_ecdsa_sign_inner` (feel free to bikeshed) which does the logic both for ecdsa_sign and for ecdsa_sign_recoverable, such that next time when things get changed/improved in ecdsa it will affect the recoverable signing too.
ACKs for top commit:
jonasnick:
ACK 28609507e7
real-or-random:
ACK 28609507e7 read the diff, tested with valgrind including ctime tests
Tree-SHA512: 4730301dcb62241d79f18eb8fed7e9ab0e20d1663a788832cb6cf4126baa7075807dc31896764b6f82d52742fdb636abc6b75e4344c6f117305904c628a5ad59
f79a7adcf5 Add valgrind uninit check to cmovs output (Elichai Turkel)
a39c2b09de Fixed UB(arithmetics on uninit values) in cmovs (Elichai Turkel)
Pull request description:
This should fix#753.
Used @peterdettman's solution here for the `ECMULT_CONST_TABLE_GET_GE` https://github.com/bitcoin-core/secp256k1/issues/753#issuecomment-631316091
and in ecdsa_sign I initialize `s` and `r` to a zero scalar.
The second commit adds a valgrind check to the cmovs that could've caught this (in ecdsa_sign, not in ecmult_const because there's a scalar clear there under `VERIFY_SETUP`)
ACKs for top commit:
sipa:
utACK f79a7adcf5
jonasnick:
ACK f79a7adcf5
real-or-random:
ACK f79a7adcf5
Tree-SHA512: 6fd7b7c84f392bda733a973f4dcfc12bf1478aac2591e2c87b69e637847d3b063c4243cc8feccaffc3a5824c18183a5e66bd4251c2322abaf63bb6439b38defe
5e8747ae2a autoconf: Use ":" instead of "dnl" as a noop (Tim Ruffing)
Pull request description:
Fixes#424.
Top commit has no ACKs.
Tree-SHA512: a83664afbc6ca1254c4767161bfbec82f3489a8a248ba7a5a46ed9ec2a39232cf92f504accadd4dbb1a6ea4791dbf7f0e1f030e51f02f49eb9a38a2e509ee6c2
71757da5cc Explictly pass SECP256K1_BENCH_ITERS to the benchmarks in travis.sh (Elichai Turkel)
99bd661d71 Replace travis_wait with a loop printing "\a" to stdout every minute (Elichai Turkel)
bc818b160c Bump travis Ubuntu from xenial(16.04) to bionic(18.04) (Elichai Turkel)
0c5ff9066e Add macOS support to travis (Elichai Turkel)
b6807d91d8 Move travis script into a standalone sh file (Elichai Turkel)
Pull request description:
ACKs for top commit:
real-or-random:
ACK 71757da5cc I inspected the diff
jonasnick:
ACK 71757da5cc
Tree-SHA512: e8fab725ef5ed98c795f39d7f26b5d967a6bd730d40eb7d9793986858bf34770b0350c1b7b1d14ae608dfff9375a0750ec67c8e6d0d4b562ab917f5e645aa67b
7e3952ae82 Clarify documentation of tweak functions. (Jonas Nick)
89853a0f2e Make tweak function documentation more consistent. (Jonas Nick)
41fc785602 Make ec_privkey functions aliases for ec_seckey_negate, ec_seckey_tweak_add and ec_seckey_mul (Jonas Nick)
22911ee6da Rename private key to secret key in public API (with the exception of function names) (Jonas Nick)
5a73f14d6c Mention that value is unspecified for In/Out parameters if the function returns 0 (Jonas Nick)
f03df0e6d7 Define valid ECDSA keys in the documentation of seckey_verify (Jonas Nick)
5894e1f1df Return 0 if the given seckey is invalid in privkey_negate, privkey_tweak_add and privkey_tweak_mul (Jonas Nick)
8f814cddb9 Add test for boundary conditions of scalar_set_b32 with respect to overflows (Jonas Nick)
3fec982608 Use scalar_set_b32_seckey in ecdsa_sign, pubkey_create and seckey_verify (Jonas Nick)
9ab2cbe0eb 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 7e3952ae82 I read the diff carefully and tested the changes
apoelstra:
ACK 7e3952ae82
Tree-SHA512: 8e9a66799cd3b6ec1c3acb731d6778035417e3dca9300d840e2437346ff0ac94f0c9be4de20aa2fac9bb4ae2f8a36d4e6a34795a640b9cfbfee8311decb102f0
37ed51a7ea Make ecdsa_sig_sign constant-time again after reverting 25e3cfb (Tim Ruffing)
93d343bfc5 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 37ed51a7ea makes sense.
jonasnick:
ACK 37ed51a7ea
Tree-SHA512: 82b5b8e29f48e84fd7a0681b62923d3bd87d724b38ef18e8c7969b0dcc5a405ebb26c14b5c5f4c7ba0ccabd152d1531d217809d1daf40872fe0c1e079b55c64b
4e284655d9 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 4e284655d9 trivial
Tree-SHA512: d6c3daa0384fc1bba36a46933641c97661b18e88c711343e2c8f91f39aa707eddd0ba77c8d5c43aaead883eeed7f4458ed1dec228d692d713572231aa6010fb0
ffef45c98a 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 ffef45c98a
Tree-SHA512: 381aed7f99fd739f4059b2e526ba9cd75b55b4fa86c9cc040fbf6b93055ce8558cc69c4ccf5d8a422b17022ca376cc9a608cf5af8d5841d62c5953f40825f5ff
3b7d26b23c build: add SECP_TEST_INCLUDES to bench_verify CPPFLAGS (fanquake)
84b5fc5bc3 build: fix OpenSSL EC detection on macOS (fanquake)
Pull request description:
ACKs for top commit:
real-or-random:
ACK 3b7d26b23c the diff looks good to me
jonasnick:
ACK 3b7d26b23c
Tree-SHA512: 381aed7f99fd739f4059b2e526ba9cd75b55b4fa86c9cc040fbf6b93055ce8558cc69c4ccf5d8a422b17022ca376cc9a608cf5af8d5841d62c5953f40825f5ff
This reverts commit 25e3cfbf9b. 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.
01993878bb Add test for memczero() (Tim Ruffing)
52a03512c1 Suppress a harmless variable-time optimization by clang in memczero (Tim Ruffing)
Pull request description:
ACKs for top commit:
jonasnick:
ACK 01993878bb
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.
85b35afa76 Add running benchmarks regularly and under valgrind in travis (Elichai Turkel)
ca4906b02e Pass num of iters to benchmarks as variable, and define envvar (Elichai Turkel)
02dd5f1bbb free the ctx at the end of bench_ecdh (Elichai Turkel)
Pull request description:
ACKs for top commit:
real-or-random:
ACK 85b35afa76 I looked at the diff and tested the ecdh benchmark using valgrind
jonasnick:
ACK 85b35afa76
Tree-SHA512: 029456d2c8f6c2c45c689279683ae30b067872bbfaee76a657f7fc3a059e2dffcde09ed29e3610de3adb055405126b6c3656c7ab5f5aaa1f45af2b32d4693ec4
ca739cba23 Compile with optimization flag -O2 by default instead of -O3 (Jonas Nick)
83fb1bcef4 Remove -O2 from default CFLAGS because this would override the -O3 flag (see AC_PROG_CC in the Autoconf manual) (Jonas Nick)
ecba8138ec Append instead of Prepend user-CFLAGS to default CFLAGS allowing the user to override default variables (Jonas Nick)
613c34cd86 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 ca739cba23
theuni:
ACK ca739cba23.
elichai:
ACK ca739cba23
Tree-SHA512: be92589faa461d245203385d44b489c7d6917b0c68472b8d7576806c0250cf5ff61d5c99ce04eebb8ff5279b9987185d4e5d2da979683fb1c489fdf3e5b59630
