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
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 a0771d15e67d3fe6ac1791f81d9731f73c550e5e I looked at the diff and checked that it does not break the tests
Tree-SHA512: 3ba37c2d9169867112981bba3d56680000651ef22ef684c3703f26ed3f71bf415fb23875d30059c8247ea9520c9cfad2c9207badf1b33da8fa3b7b7235a8bf16
dd98cc988f0fb3a0ab10bf1a4e28d2fbffd6c1e7 travis: Added a valgrind test without endro and enabled recovery+ecdh (Elichai Turkel)
b4c1382a87dde22d0a5075e56fb7f5d2a09f7cc7 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 dd98cc988f0fb3a0ab10bf1a4e28d2fbffd6c1e7 I looked at the diff
jonasnick:
ACK dd98cc988f0fb3a0ab10bf1a4e28d2fbffd6c1e7
Tree-SHA512: 72d7f1f4c8dd4c58501ac1003b28296d6fd140a8f7711e9e3b3c04a3fbce358ff1c89d2e1d1c5489d7668d3019981264c5cadecae3d9b48cd38c9463e287d8ad
362bb25608dbcd724a07dd5170c4ebe081c3dd84 Modified bench_scalar_split so it won't get optimized out (Elichai Turkel)
73a30c6b58f078b42a03a222c55bfe8b4dd86a2b 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 362bb25608dbcd724a07dd5170c4ebe081c3dd84 I read the diff and I ran the benchmarks
Tree-SHA512: d5e47f5d64c3b035155276f057671ceb7f5852f24c7102fee4d0141aabebf882039f3eae0d152bae89d0603bc09fa6ad9f7bc6b8c0f74a668ee252c727517804
5c5f71e Fix ASM setting in travis (Jonas Nick)
Pull request description:
Without this PR the `ASM` setting isn't taken into account in travis.
ACKs for commit 5c5f71:
real-or-random:
ACK 5c5f71eea5167b0dd9dbef246fc70132c50c9af3 I read the diff
Tree-SHA512: 741650e4b9163e0e7341fa59b9859da85d0e34fa59980e68eacf59388879281b640836532acb3d8121da18d8e75a7c2993defada6329df830a99472b71cc17fe
bae1bea3c4b46a2fb5ca76ff6bf1e98d43cff52f Make no-float policy explicit (Tim Ruffing)
Pull request description:
We don't want floating types for various reasons, e.g.,
- Their representation and often their behavior is implementation-defined.
- Many targets don't support them.
Closes#683.
ACKs for top commit:
jonasnick:
ACK bae1bea3c4b46a2fb5ca76ff6bf1e98d43cff52f
Tree-SHA512: e0027d6dda1a3e4b7d146fd3bea04e05473e08e25c0d0730018768be00351dfcf51b87b47b9e27953a21d42e0621433f13cbe55e4c20a7f7086e0191dff607a6
We don't want floating types for various reasons, e.g.,
- Their representation and often their behavior is implementation-defined.
- Many targets don't support them.
b76142f Remove note about heap allocation in secp256k1_ecmult_odd_multiples_table_storage_var which was removed in 47045270fa90f81205d989f7107769bce1e71c4d (Jonas Nick)
Pull request description:
...which was removed in 47045270fa90f81205d989f7107769bce1e71c4d. h/t @roconnor-blockstream
ACKs for commit b76142:
Tree-SHA512: 05fcd7aa5d765f1f5d31b93d40c2621e1dd9674a0db136a1e1cb216d6c01f5be1580275700cbdc08feda8f165b3b349640472d0bdec770bebb23f952225e3f52
0d82732 Improve VERIFY_CHECK of overflow in secp256k1_scalar_cadd_bit. This added check ensures that any curve order overflow doesn't go undetected due a uint32_t overflow. (Russell O'Connor)
8fe63e5 Increase robustness against UB. Thanks to elichai2 who noted that the literal '1' is a signed integer, and that shifting a signed 32-bit integer by 31 bits causes an overflow and yields undefined behaviour. While 'scalar_low_impl''s 'secp256k1_scalar_cadd_bit' is only used for testing purposes and currently the 'bit' parameter is only 0 or 1, it is better to avoid undefined behaviour in case the used domain of 'secp256k1_scalar_cadd_bit' expands. (roconnor-blockstream)
Pull request description:
Avoid possible, but unlikely undefined behaviour in `scalar_low_impl`'s `secp256k1_scalar_cadd_bit`.
Thanks to elichai2 who noted that the literal `1` is a signed integer, and that shifting a signed 32-bit integer by 31 bits causes an overflow and yields undefined behaviour.
Using the unsigned literal `1u` addresses the issue.
ACKs for commit 0d8273:
real-or-random:
ACK 0d82732a9a16cecc445e61c718ce9bdc2d228e76
jonasnick:
ACK 0d82732a9a16cecc445e61c718ce9bdc2d228e76
Tree-SHA512: 905be3b8b00aa5cc9bd6dabb543745119da8f34181d37765071f28abbc1d6ff3659e3f195b72c2f2d003006678823919668bc0d169ac8b8d4bcc5da671813c99
59782c68b41e4262f003135717705990b3fdc3ae Remove mention of ec_privkey_export because it doesn't exist (Jonas Nick)
Pull request description:
Fixes#663
There is `ec_privkey_export_der` but it takes `0` for uncompressed and not `SECP256K1_EC_UNCOMPRESSED` (which is `2`).
ACKs for top commit:
real-or-random:
ACK 59782c68b4
apoelstra:
utACK 59782c68b4
Tree-SHA512: 6167581df74264be576f921d04bb8e23e16fa3b823bac4b45299079ceee38d6c74dd14a55b7b976a2cee9bdbd74dd6e3b39c0482808c1b8e65c8c80743f113a2
dcb2e3b3fff0b287d576842aabe5c79f2fe4df30 variable signing precompute table (djb)
Pull request description:
This pull request gives an option to reduce the precomputed table size for the signing context (`ctx`) by setting `#define ECMULT_GEN_PREC_BITS [N_BITS]`.
Motivation: Per #251 and #254, the static table can be reduced to 64kB. However, this is still too big for some of my embedded applications. Setting `#define ECMULT_GEN_PREC_BITS 2` produces a 32kB table at a tradeoff of about 75% of the signing speed. Not defining this value will default to the existing implementation of 4 bits. Statistics:
```
ECMULT_GEN_PREC_BITS = 1
Precomputed table size: 32kB
./bench_sign
ecdsa_sign: min 195us / avg 200us / max 212us
ECMULT_GEN_PREC_BITS = 2
Precomputed table size: 32kB
./bench_sign
ecdsa_sign: min 119us / avg 126us / max 134us
ECMULT_GEN_PREC_BITS = 4 (default)
Precomputed table size: 64kB
./bench_sign
ecdsa_sign: min 83.5us / avg 89.6us / max 95.3us
ECMULT_GEN_PREC_BITS = 8
Precomputed table size: 512kB
./bench_sign
ecdsa_sign: min 96.4us / avg 99.4us / max 104us
```
Only values of 2 and 4 make sense. 8 bits causes a larger table size with no increase in speed. 1 bit runs, actually, but does not reduce table size and is slower than 2 bits.
ACKs for top commit:
real-or-random:
ACK dcb2e3b3fff0b287d576842aabe5c79f2fe4df30 verified that all changes to the previous ACKed 1d26b27ac90092306bfbc9cdd5123e8a5035202a were due to the rebase
jonasnick:
ACK dcb2e3b3fff0b287d576842aabe5c79f2fe4df30 read the code and tested various configurations with valgrind
Tree-SHA512: ed6f68ca23ffdc4b59d51525336b34b25521233537edbc74d32dfb3eafd8196419be17f01cbf10bd8d87ce745ce143085abc6034727f742163f7e5f13f26f56e
make ECMULT_GEN_PREC_BITS configurable
ecmult_static_context.h: add compile time config assertion (#3) - Prevents accidentally using a file which was generated with a
different configuration.
README: mention valgrind issue
With --with-ecmult-gen-precision=8, valgrind needs a max stack size
adjustment to not run into a stack switching heuristic:
http://valgrind.org/docs/manual/manual-core.html
> -max-stackframe= [default: 2000000]
> The maximum size of a stack frame. If the stack pointer moves by more than this amount then Valgrind will assume that the program is switching to a different stack.
You may need to use this option if your program has large stack-allocated arrays.
basic-config: undef ECMULT_WINDOW_SIZE before (re-)defining it
a467047e110fb55186df173afa3d5f330f6fa47c Make ./configure string consistent (Tim Ruffing)
Pull request description:
This was forgotten in some PR rebase.
ACKs for top commit:
jonasnick:
ACK a467047e110fb55186df173afa3d5f330f6fa47c
Tree-SHA512: 5aa67e886c165afa97a1e34ccfbd6bb0158ba4d4e5a4aacf6ac8b17ad9ee55132061957fd5ec383a79ad72ec7c92c745d7ad4fddca743b53e4b0e635616b29dc
b64a2e2597b66b57e23f3cb34f9c88809e34d93f Fix a nit in the recovery tests (Elichai Turkel)
Pull request description:
this signature is only valid under recid 1 not 0.
Source: https://github.com/bitcoin-core/secp256k1/blob/master/src/modules/recovery/tests_impl.h#L247
(it passes only when the sig is parsed with recid 1)
ACKs for top commit:
real-or-random:
ACK b64a2e2597b66b57e23f3cb34f9c88809e34d93f I only looked at the diff
jonasnick:
ACK b64a2e2597b66b57e23f3cb34f9c88809e34d93f read the code
Tree-SHA512: 8e6744fe87c4078181dd1b334641784bf4fee37eb87346ecf8149482a9ea2c321bbe068e6a9199d836430b54b73848d94473a9aa6b59b4a68921a6321f449736
a11c76c59a431e3492994f71a968a838e398fb58 secp256k1/src/tests.c: Properly handle sscanf return value (Mustapha Abiola)
Pull request description:
This pull request fixes a bug which allows the `sh` variable to be used uninitialised
when sscanf(3) returns EOF.
Signed-off-by: Mustapha Abiola <mustapha@trilemma.net>
ACKs for top commit:
sipa:
ACK a11c76c59a431e3492994f71a968a838e398fb58.
practicalswift:
utACK a11c76c59a431e3492994f71a968a838e398fb58
real-or-random:
ACK a11c76c59a431e3492994f71a968a838e398fb58 I looked at the code
Tree-SHA512: fd9660a18e39ecf9366db94ccbcec2682b020223f4f982a4356ddf56c2fbdafa5edcd830db37be12b661c1ec0b15c57b9f34ba59ef4460187c9c2478376fbc88
74e2dbd JNI: fix use sig array (liuyujun)
Pull request description:
ACKs for commit 74e2db:
sipa:
ACK 74e2dbd68e07f752ac326a578e3071f9efa55e55. This is clearly an improvement.
real-or-random:
ACK 74e2dbd68e07f752ac326a578e3071f9efa55e55 I've read the code but haven't tested it
Tree-SHA512: 850b32e893463be4be28185dcc127d429afe4b6076036a078b7c61d590e0f4ea89127e448760b71c087cf70ffbefc52d87db77a5131bee81f3e4f95cfbd3bd3e
