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
08fb6c4926 Run valgrind_ctime_test in travis (Jonas Nick)
3d2302257f 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 08fb6c4926
real-or-random:
ACK 08fb6c4926
jonasnick:
ACK 08fb6c4926
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
7b50483ad7 Adds a declassify operation to aid constant-time analysis. (Gregory Maxwell)
34a67c773b 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 7b50483ad7
real-or-random:
ACK 7b50483ad7 I read the code carefully and tested it
jonasnick:
reACK 7b50483ad7
Tree-SHA512: 0776c3a86e723d2f97b9b9cb31d0d0e59dfcf308093b3f46fbc859f73f9957f3fa977d03b57727232040368d058701ef107838f9b1ec98f925ec78ddad495c4e
eb45ef3384 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 eb45ef3384
jonasnick:
ACK eb45ef3384
elichai:
ACK eb45ef3384
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)
4b48a43106 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 4b48a43106
real-or-random:
ACK 4b48a43106 diff inspection
Tree-SHA512: d6bedb495e46b27ac9b558e77d814884d782ea78569a2296688eccf374bc880d13846546ad449c2a677865cf6ed56fcbc8be58c21f9daca5084831074e20d769
642cd062bd 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 642cd062bd I read the diff
real-or-random:
ACK 642cd062bd 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>
acb7f97eb8 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 acb7f97eb8
sipa:
ACK acb7f97eb8
Tree-SHA512: d39f3e0b289229b2ce085406f6d716fdd54038df9ee5273a18a05140d1eddd4149149e881cc7a13f2126347217b9c56a0c12adf558c49879c5f556695242afc6
d567b779fe Clarify comments about use of rzr on ge functions and abs function. (Gregory Maxwell)
2241ae6d14 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 d567b779fe I read the diff carefully and tested the code with ECDH enabled and various settings, also on valgrind
sipa:
ACK d567b779fe
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.
2e759ec753 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 2e759ec753
jonasnick:
ACK 2e759ec753
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>
bde2a32286 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 bde2a32286 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`.
78c3836341 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 78c3836341 I looked at the diff and verified my fingerprint
Tree-SHA512: 53a989615665cf8cf0c6a70d3bc2c4b71b68178cae40b2a7881aa9eba24732d126ba1e258a9fc127c69b47bb3025943097300cfcbbe18736cbf92ff4f3a901e0
a0771d1 Explicitly disable buffering for stderr in tests (Jonas Nick)
fb424fb Make travis show the ./tests seed by removing stdout buffering and always cat tests.log after a travis run. (Jonas Nick)
Pull request description:
…by removing stdout buffering and always cat tests.log after a travis run. Fixes#645.
I noticed that according to the [doc](https://www.gnu.org/software/automake/manual/html_node/Parallel-Test-Harness.html) tests.log should contain stdout as well as stderr. But it doesn't because stdout isn't flushed. I removed buffering completely to avoid having to call `fflush` twice.
Travis is instructed to always show the seed which seems helpful with `after_script` by `cat`ing `./tests.log`. In case the tests fail it looks like https://travis-ci.org/jonasnick/secp256k1/jobs/606446234.
ACKs for commit a0771d:
real-or-random:
ACK a0771d15e6 I looked at the diff and checked that it does not break the tests
Tree-SHA512: 3ba37c2d9169867112981bba3d56680000651ef22ef684c3703f26ed3f71bf415fb23875d30059c8247ea9520c9cfad2c9207badf1b33da8fa3b7b7235a8bf16
dd98cc988f travis: Added a valgrind test without endro and enabled recovery+ecdh (Elichai Turkel)
b4c1382a87 Add valgrind check to travis (Elichai Turkel)
Pull request description:
As discussed in https://github.com/bitcoin-core/secp256k1/pull/687
This adds valgrind check to the repo.
It doesn't run on recovery+ecdh because of the time.
No openssl because of uninitialized mem.
I debated between with and without ASM, but decided with ASM because it might be more fragile(?).
I wasn't sure if I should pass `-DVALGRIND` via `CFLAGS` or `CPPFLAGS`, it seems like because this is only C then there shouldn't even be `CPPFLAGS` but looks like we use `CPPFLAGS` in other places for the preprocessor definitions.
If people are worried about the time it takes we can mark it as `allow_failure` although I don't think it's a problem here because there's only a handful of PRs and they're usually open for weeks.
ACKs for top commit:
real-or-random:
ACK dd98cc988f I looked at the diff
jonasnick:
ACK dd98cc988f
Tree-SHA512: 72d7f1f4c8dd4c58501ac1003b28296d6fd140a8f7711e9e3b3c04a3fbce358ff1c89d2e1d1c5489d7668d3019981264c5cadecae3d9b48cd38c9463e287d8ad
362bb25608 Modified bench_scalar_split so it won't get optimized out (Elichai Turkel)
73a30c6b58 Added accumulators and checks on benchmarks so they won't get optimized out (Elichai Turkel)
Pull request description:
As asked https://github.com/bitcoin-core/secp256k1/pull/667#issuecomment-546885951 this is the parts of #667 that don't require an assembly memory fence.
I splitted them to 2 commits, one with obvious easy ones. and another that changes the logic a bit to achieve this (See https://github.com/bitcoin-core/secp256k1/pull/667#discussion_r337248398 )
ACKs for top commit:
jonasnick:
ACK 362bb256
real-or-random:
ACK 362bb25608 I read the diff and I ran the benchmarks
Tree-SHA512: d5e47f5d64c3b035155276f057671ceb7f5852f24c7102fee4d0141aabebf882039f3eae0d152bae89d0603bc09fa6ad9f7bc6b8c0f74a668ee252c727517804
Jonas Nick
Tim Ruffing
Elichai Turkel
Gregory Maxwell
Rusty Russell
Marko Bencun
Pieter Wuille
Wladimir J. van der Laan