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.
2e759ec753446aab0272ba32c5f1b7dc3a4dc75c Overhaul README.md (Tim Ruffing)
Pull request description:
* Update feature list
* Be more positive about the state and quality of the library
* Mention ECDSA key operations explicitly in short library description
* Say "secret key" instead of "private key"
cc @gmaxwell who suggested a similar wording for the disclaimer.
ACKs for top commit:
sipa:
ACK 2e759ec753446aab0272ba32c5f1b7dc3a4dc75c
jonasnick:
ACK 2e759ec753446aab0272ba32c5f1b7dc3a4dc75c
Tree-SHA512: 2e1c87e7fa28d9dab682af227f845e7d48ac79a9fbe10be47ae4567abc2e066ba2f852c000db7d697ece8e4bbeeb851ea647465f870ac29dc3654031bf15a1ad
* Update feature list
* Be more positive about the state and quality of the library
* Mention ECDSA key operations explicitly in short library description
* Say "secret key" instead of "private key
* Define "experimental"
Co-Authored-By: Gregory Maxwell <greg@xiph.org>
bde2a32286c697dd1056aa3eb1ea2a5353f0bede Convert bench.h to fixed-point math (Wladimir J. van der Laan)
Pull request description:
Convert `bench.h` to fixed-point math, removing all use of float math from the repository:
- Use 64-bit integer microsecond timestamps
- Use decimal fixed-point math for formatting numbers
It turned out to be a little trickier than I expected because of formatting and rounding. But, output should be the same before and after.
I used the following to test the number formatting: https://gist.github.com/laanwj/f971bfbe018e39c19677a21ff954d0c7
ACKs for top commit:
real-or-random:
ACK bde2a32286c697dd1056aa3eb1ea2a5353f0bede I've read the code in detail and I've tested it. I haven't explicitly tested the formatting function with known/hardcoded inputs.
Tree-SHA512: 41ab6024b88c65a4b194272097c70d527bedb396dc7ab9d3d93165f1a19d31092798370f66399443a8d5393d0a6dcf5825679de5a325550865cfdef3586bf64c
- Use 64-bit integer microsecond timestamps
- Use fixed-point math for formatting numbers
Then, remove "except in benchmarks" exception from `README.md`.
78c38363412db3ea1cd1f0cc42dd1624c078ee32 Add SECURITY.md (Jonas Nick)
Pull request description:
Fixes#646
WIP because the secp256k1-security@bitcoincore.org email address doesn't exist yet. But it seems like the right place for vulnerability reports. security@bitcoincore.org would have the downside that it perhaps reaches more people than necessary. Ideally secp256k1-security would just forward to the three maintainers listed in SECURITY.md. @sipa @apoelstra is it okay to put you there? Fwiw I'm opting out for now because three people should be enough.
@sipa do you know who to talk to about adding secp256k1-security@bitcoincore.org and the specifics about how it would work?
ACKs for top commit:
real-or-random:
ACK 78c38363412db3ea1cd1f0cc42dd1624c078ee32 I looked at the diff and verified my fingerprint
Tree-SHA512: 53a989615665cf8cf0c6a70d3bc2c4b71b68178cae40b2a7881aa9eba24732d126ba1e258a9fc127c69b47bb3025943097300cfcbbe18736cbf92ff4f3a901e0
