47a7b8382fd6f1458d859b315cf3bcd3b9790b68 Clear field elements when writing infinity (Elichai Turkel)
61d1ecb02847be9d65ffe9df2d2408d85f3a0711 Added test with additions resulting in infinity (Elichai Turkel)
Pull request description:
Currently if `secp256k1_gej_add_var` / `secp256k1_gej_add_ge_var` /` secp256k1_gej_add_zinv_var` receive `P + (-P)` it will set `gej->infinity = 1` but doesn't call initialize the field elements.
Notice that this is the only branch in the function that results in an uninitialized output.
By using `secp256k1_gej_set_infinity()` it will set the field elements to zero while also setting the infinity flag.
I also added a test that fails with valgrind on current master but passes with the fix.
EDIT: This isn't a bug or something necessary, I just personally found this helpful.
ACKs for top commit:
real-or-random:
ACK 47a7b8382fd6f1458d859b315cf3bcd3b9790b68
Tree-SHA512: cdc2efc242a1b04b4f081183c07d4b2602cdba705e6b30b548df4e115e54fb97691f4b1a28f142f02d5e523c020721337a297b17d732acde147b910f5c53bd0a
8bc6aeffa9a191e677cb9e3a22fff130f16990f3 Add SHA256 selftest (Pieter Wuille)
5e5fb28b4a45d7e35e55b5f5feead2be07bccc28 Use additional system macros to figure out endianness (Pieter Wuille)
Pull request description:
These are all the architecture macros I could find with known endianness. Use those as a fallback when __BYTE_ORDER__ isn't available.
See https://github.com/bitcoin-core/secp256k1/pull/787#issuecomment-673764652
It also adds a SHA256 selftest, so that improperly overriding the endianness detection will be detected at runtime.
ACKs for top commit:
real-or-random:
ACK 8bc6aeffa9a191e677cb9e3a22fff130f16990f3 I read the diff, and tested that the self-test passes/fails with/without the correct endianness setting
gmaxwell:
ACK 8bc6aeffa9a191e677cb9e3a22fff130f16990f3 looks good and I also ran the tests on MIPS-BE and verified that forcing it to LE makes the runtime test fail.
Tree-SHA512: aca4cebcd0715dcf5b58f5763cb4283af238987f43bd83a650e38e127f348131692b2eed7ae5b2ae96046d9b971fc77c6ab44467689399fe470a605c3458ecc5
bceefd6547635132ba17f022a52db18f17e00df6 Add test logs to gitignore (Jake Rawsthorne)
Pull request description:
Was just running the tests for https://github.com/bitcoin-core/secp256k1/pull/558 and noticed these logs weren't ignored
ACKs for top commit:
real-or-random:
ACK bceefd6547
sipa:
ACK bceefd6547635132ba17f022a52db18f17e00df6
Tree-SHA512: 690906bc80abc547e1ef78d8654900c2f4054fd8cb8c2e0a6f6b95a5875930b8e1e3a69a5dca86b198e4a2601788f584c8b2ff6f5a85da230b12954e07aeff37
60f7f2de5de917c2bee32a4cd79cc3818b6a94a0 Don't assume that ALIGNMENT > 1 in tests (Tim Ruffing)
ada6361dece4265823478e0019a8c196e9285a26 Use ROUND_TO_ALIGN in scratch_create (Jonas Nick)
8ecc6ce50ead28a0b8bab2f1fb18a58ee5204a13 Add check preventing rounding to alignment from wrapping around in scratch_alloc (Jonas Nick)
4edaf06fb02a9ac9cd115e0c967bb0ef35cae01d Add check preventing integer multiplication wrapping around in scratch_max_allocation (Jonas Nick)
Pull request description:
This PR increases the general robustness of scratch spaces. It does not fix an existing vulnerability because scratch spaces aren't used anywhere in master. Additionally, it must be prevented anyway that an attacker has (indirect) control over the arguments touched in this PR.
ACKs for top commit:
sipa:
ACK 60f7f2de5de917c2bee32a4cd79cc3818b6a94a0
Tree-SHA512: ecdd794b55a01d1d6d24098f3abff34cb8bb6f33737ec4ec93714aa631c9d397b213cc3603a916ad79f4b09d6b2f8973bf87fc07b81b25a530cc72c4dbafaba9
1c325199d590e018cdfb5ea2ab541774009bf7f7 Remove the extremely outdated TODO file. (Gregory Maxwell)
Pull request description:
This had two things in it-- tests for the scalar/field code and
constant time signing and keygen.
The signing and keygen have been thoroughly constant time for years
and there are now powerful tests to verify it... no further work
on constant-time is needed at least on ordinary platforms (other
sidechannels-- sure).
The scalar and field code have extensive tests. They could use
better static test vectors but they're well tested.
TODOs for the project are currently better documented on github
right now. This file could return in the future with current
info, if needed.
ACKs for top commit:
real-or-random:
ACK 1c325199d5
Tree-SHA512: 65c730ad2816b28991cdb74df6da4671abe76a74a0d0b306f13612b4bbe9b54f9a623b18fc288e0ec13572d9fdbab6f376ce7aafc9fe601644239629b84fb15c
This had two things in it-- tests for the scalar/field code and
constant time signing and keygen.
The signing and keygen have been thoroughly constant time for years
and there are now powerful tests to verify it... no further work
on constant-time is needed at least on ordinary platforms (other
sidechannels-- sure).
The scalar and field code have extensive tests. They could use
better static test vectors but they're well tested.
TODOs for the project are currently better documented on github
right now. This file could return in the future with current
info, if needed.
7c068998bac3e4a254d8542458b2068e38fca435 Compile-time check assumptions on integer types (Pieter Wuille)
02b6c87b52dbac1557b689ab2ebc8b91d67fd0f3 Add support for (signed) __int128 (Pieter Wuille)
Pull request description:
A compile-time check is implemented in a new `src/assumptions.h` which verifies several aspects that are implementation-defined in C:
* size of bytes
* conversion between unsigned and (negative) signed types
* right-shifts of negative signed types.
ACKs for top commit:
gmaxwell:
ACK 7c068998bac3e4a254d8542458b2068e38fca435
real-or-random:
ACK 7c068998bac3e4a254d8542458b2068e38fca435 code review and tested
Tree-SHA512: 3903251973681c88d64d4af0f6cb40fde11eb436804c5b6202c3715b78b1a48bcb287f601b394fd0b503437e3832ba011885e992fe65098b33edc430d9b1f67d
0dccf98a21beb245f6cd9ed76fb7368529df09c7 Use preprocessor macros instead of autoconf to detect endianness (Tim Ruffing)
Pull request description:
This does not fix any particular issue but it's preferable to not
rely on autoconf. This avoids endianness mess for users on BE hosts
if they use their build without autoconf.
The macros are carefully written to err on the side of the caution,
e.g., we #error if the user manually configures a different endianness
than what we detect.
Supersedes #770 .
ACKs for top commit:
sipa:
ACK 0dccf98a21beb245f6cd9ed76fb7368529df09c7
gmaxwell:
ACK 0dccf98a21beb245f6cd9ed76fb7368529df09c7
Tree-SHA512: 6779458de5cb6eaef2ac37f9d4b8fa6c9b299f58f6e5b72f2b0d7e36c12ea06074e483acfb85085a147e0f4b51cd67d897f61a67250ec1cea284a0f7680eb2e8
79f1f7a4f123765cf07be92ae894d882c5845191 Autodetect __int128 availability on the C side (Pieter Wuille)
0d7727f95e52d99c13f55c64e9d1f799ba7d7967 Add SECP256K1_FE_STORAGE_CONST_GET to 5x52 field (Pieter Wuille)
Pull request description:
This PR does two things:
* It removes the ability to select the 5x52 field with a 8x32 scalar, or the 10x26 field with a 4x64 scalar. It's both 128-bit wide versions, or neither.
* The choice is made automatically by the C code, unless overridden by a USE_FORCE_WIDEMUL_INT{64,128} define (which is available through `configure` with a hidden option --with-test-override-wide-multiplication={auto,int64,int128}).
This reduces the reliance on autoconf for this performance-critical configuration option, and also reduces the number of different combinations to test.
This removes one theoretically useful combination: if you had x86_64 asm but no __int128 support in your compiler, it was possible to use the 64-bit field before but the 32-bit scalar. I think this doesn't matter as all compilers/systems that support (our) x86_64 asm also support __int128. Furthermore, #767 will break this.
As an unexpected side effect, this also means the `gen_context` static precomputation tool will now use __int128 based implementations when available (which required an addition to the 5x52 field; see first commit).
ACKs for top commit:
real-or-random:
ACK 79f1f7a4f123765cf07be92ae894d882c5845191 diff looks good and tests pass
elichai:
tACK 79f1f7a4f123765cf07be92ae894d882c5845191
Tree-SHA512: 4171732668e5c9cae5230e3a43dd6df195567e1232b89c12c5db429986b6519bb4d77334cb0bac8ce13a00a24dfffdff69b46c89b4d59bc6d297a996ea4efd3d
This does not fix any particular issue but it's preferable to not
rely on autoconf. This avoids endianness mess for users on BE hosts
if they use their build without autoconf.
The macros are carefully written to err on the side of the caution,
e.g., we #error if the user manually configures a different endianness
than what we detect.
57d3a3c64cf3d435d5d45e323cf9cbe21da8c6cf Avoid linking libcrypto in the valgrind ct test. (Gregory Maxwell)
Pull request description:
Libcrypto isn't useful here and on some systems UB in OpenSSL's
init causes failures.
Fixes#775.
ACKs for top commit:
real-or-random:
ACK 57d3a3c64cf3d435d5d45e323cf9cbe21da8c6cf
elichai:
tACK 57d3a3c64cf3d435d5d45e323cf9cbe21da8c6cf
Tree-SHA512: 0b10b3e9cc0871a9a93271c72be9d1663ea163745071cb4951a99664c048ab5b6f46bb7cff36e7000e8fb26df7ee164f536f61210bece376478f9f774f34e83d
Instead of supporting configuration of the field and scalar size independently,
both are now controlled by the availability of a 64x64->128 bit multiplication
(currently only through __int128). This is autodetected from the C code through
__SIZEOF_INT128__, but can be overridden using configure's
--with-test-override-wide-multiply, or by defining
USE_FORCE_WIDEMUL_{INT64,INT128} manually.
So far this has not been needed, as it's only used by the static precomputation
which always builds with 32-bit fields.
This prepares for the ability to have __int128 detected on the C side, breaking
that restriction.
39295362cfc856aae1c37cc1194c2f6d53fd6f25 Test travis s390x (big endian) (Pieter Wuille)
Pull request description:
ACKs for top commit:
real-or-random:
ACK 39295362cfc856aae1c37cc1194c2f6d53fd6f25 Travis works and says it's big endian
Tree-SHA512: 939b98fe369e575e8bf56899a28cb5aafdb9ccfaaee3cb611027e053edc8220d2787c34359cd01508899b8b7e105c89853a4ab44c382252538c797d00c09345b
18d36327fddad18ba81af2cf7fe6c8a16952dc22 secp256k1_gej_double_nonzero supports infinity (Pieter Wuille)
Pull request description:
Our existing function `secp256k1_gej_double_nonzero` actually supports infinity if only it wouldn't check that the input isn't infinity.
Drop the check, rename it to `secp256k1_gej_double`, and adapt the tests.
ACKs for top commit:
real-or-random:
ACK 18d36327fddad18ba81af2cf7fe6c8a16952dc22 I looked at the diff and ran tests locally
gmaxwell:
ACK 18d36327fddad18ba81af2cf7fe6c8a16952dc22
Tree-SHA512: 79dc42099c318f0bdfe7961495ab3fbbe87551c3cc373557a371914bb65638b129ddfd360e694959349f184e2d71a540abdbef04211e7eb70ee17b691632b915
When $USE_HOST or $EXTRAFLAGS are empty, we pass (due to quoting) an
empty string as a parameter to ./configure, which then believes we want
to use a deprecated syntax for specifing a host or a target and yells at us:
> configure: WARNING: you should use --build, --host, --target
The fixes are:
- $EXTRAFLAGS could contain multiple flags and should not be quoted at all.
- We can get rid of $USE_HOST by specifying --host="$HOST" directly.
67a429f31fd3d1b37c5365cc58b70588b8645d62 Suppress a harmless variable-time optimization by clang in _int_cmov (Tim Ruffing)
5b196338f0c8dc07bf0eece37b46d8686c4da3ce Remove redundant "? 1 : 0" after comparisons in scalar code (Tim Ruffing)
Pull request description:
Attempt at resolving #771 .
This surprisingly seems to improve the situation at least for the compilers available on godbolt.
ACKs for top commit:
gmaxwell:
ACK 67a429f31fd3d1b37c5365cc58b70588b8645d62
elichai:
tACK 67a429f31fd3d1b37c5365cc58b70588b8645d62
Tree-SHA512: ee8b0c86831ec8c3d5a9abcad773ed8a0f267e5c47012e4e1423b10a64c26b4cf6e3c466c3df765ba7e636787a3fe134d633926d67b599287f12c51be924f478
2e1b9e0458317d03b682c1f5dd63aedb52c86b04 tests: Abort if malloc() fails during context cloning tests (Tim Ruffing)
Pull request description:
Found by the clang static analyzer.
This is the worst true positive that it found. I feel somewhat proud.
ACKs for top commit:
elichai:
tACK 2e1b9e0458317d03b682c1f5dd63aedb52c86b04
Tree-SHA512: bf9a3b6c2b8beaafd230ece00a9a69dd884a35b6d2243502ebfded3f77a454e80ef922791bd48c17aa4814a275550957071c045912080a616dd5ed704a70aab7
37dba329c6cb0f7a4228a11dc26aa3a342a3a5d0 Remove unnecessary sign variable from wnaf_const (Jonas Nick)
6bb0b77e158fc2f9e56e4b65b08bcb660d4c588b Fix test_constant_wnaf for -1 and add a test for it. (Jonas Nick)
Pull request description:
There currently is a single branch in the `ecmul_const` function that is not being exercised by the tests. This branch is unreachable and therefore I'm suggesting to remove it.
For your convenience the paper the wnaf algorithm can be found [here (The Width-w NAF Method Provides Small Memory and Fast Elliptic Scalar Multiplications Secure against Side Channel Attacks)](http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.563.1267&rep=rep1&type=pdf). Similarly, unless I'm missing something important, I don't see how their algorithm needs to consider `sign(u[i-1])` unless `d` can be negative - which doesn't make much sense to me either.
ACKs for top commit:
real-or-random:
ACK 37dba329c6cb0f7a4228a11dc26aa3a342a3a5d0 I verified the correctness of the change and claimed invariant by manual inspection. I tested the code, both with 32bit and 64bit scalars.
Tree-SHA512: 9db45f76bd881d00a81923b6d2ae1c3e0f49a82a5d55347f01e1ce4e924d9a3bf55483a0697f25039c327e33edca6796ba3205c068d9f2f99aa5d655e46b15be
1309c03c45beece646a7d21fdb6a0e3d38adee2b Fix some compile problems on weird/old compilers. (Gregory Maxwell)
Pull request description:
The visibility attribute is a GCC 4+ feature.
GCC 2.95 also warns about the unsigned/signed comparision.
ACKs for top commit:
real-or-random:
ACK 1309c03c45beece646a7d21fdb6a0e3d38adee2b I inspected the diff
Tree-SHA512: b5a5175416b67b2619f68ad82a208052ad678955e59c2f3457799abd1dd6fd817c40f6bc2941b2bda207c6f58ad0fbe46221a2f92b726e824702c4c0b177377c
f00d6575ca0dcca11e085b20afa4d73dc8742ddc remove dead store in ecdsa_signature_parse_der_lax (fanquake)
Pull request description:
ACKs for top commit:
elichai:
utACK f00d6575ca0dcca11e085b20afa4d73dc8742ddc, it does look like we don't use that assignment
jonasnick:
ACK f00d6575ca0dcca11e085b20afa4d73dc8742ddc
Tree-SHA512: 9aa54c901f299341c309411b0247720f5152a131dd346c19be7ee21865e3a822e8cf91b869e28ef6288adaf31660bc2e18874e304052468a9be6b7027674af30
2e7fc5b5372067ecd33b2304e8c88ed6de98ff13 Fix uninitialized variables in ecmult_multi test (Jonas Nick)
Pull request description:
Fixes#756
ACKs for top commit:
real-or-random:
ACK 2e7fc5b5372067ecd33b2304e8c88ed6de98ff13 I inspected the diff. I did not test it and I did not check whether if makes the warning go away
elichai:
tACK 2e7fc5b5372067ecd33b2304e8c88ed6de98ff13
Tree-SHA512: 674400134f5487236f5b6e8b3020b346d43662511628cdf6dd1bd7ba1de985bf93f5be11f5650f250ff37b5f87eb4b01d90ed53d41193c05a420d3f5a2d63470
28609507e70a172dd8f39de4aa55f851452fc0b4 Add tests for the cmov implementations (Elichai Turkel)
73596a85a2ab9c885e58a7a2a8876355a6ae68e4 Add ecdsa_sign_recoverable to the ctime tests (Elichai Turkel)
2876af4f8da952e39c06bc229d68cd4892ea2c85 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 28609507e70a172dd8f39de4aa55f851452fc0b4
real-or-random:
ACK 28609507e70a172dd8f39de4aa55f851452fc0b4 read the diff, tested with valgrind including ctime tests
Tree-SHA512: 4730301dcb62241d79f18eb8fed7e9ab0e20d1663a788832cb6cf4126baa7075807dc31896764b6f82d52742fdb636abc6b75e4344c6f117305904c628a5ad59
f79a7adcf555ccc78b591850ea15c64fbfbca152 Add valgrind uninit check to cmovs output (Elichai Turkel)
a39c2b09de304b8f24716b59219ae37c2538c242 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 f79a7adcf555ccc78b591850ea15c64fbfbca152
jonasnick:
ACK f79a7adcf555ccc78b591850ea15c64fbfbca152
real-or-random:
ACK f79a7adcf555ccc78b591850ea15c64fbfbca152
Tree-SHA512: 6fd7b7c84f392bda733a973f4dcfc12bf1478aac2591e2c87b69e637847d3b063c4243cc8feccaffc3a5824c18183a5e66bd4251c2322abaf63bb6439b38defe
5e8747ae2a0c915d079837d238f8b84841a4ce5c autoconf: Use ":" instead of "dnl" as a noop (Tim Ruffing)
Pull request description:
Fixes#424.
Top commit has no ACKs.
Tree-SHA512: a83664afbc6ca1254c4767161bfbec82f3489a8a248ba7a5a46ed9ec2a39232cf92f504accadd4dbb1a6ea4791dbf7f0e1f030e51f02f49eb9a38a2e509ee6c2
71757da5ccece100b1eca6c70b4d87e2542cff97 Explictly pass SECP256K1_BENCH_ITERS to the benchmarks in travis.sh (Elichai Turkel)
99bd661d71fe17be7b3de4707a0264c41a63ebe8 Replace travis_wait with a loop printing "\a" to stdout every minute (Elichai Turkel)
bc818b160cdaaccff2162206cc15915fa5f0cca8 Bump travis Ubuntu from xenial(16.04) to bionic(18.04) (Elichai Turkel)
0c5ff9066e6fa41b1fbd5d0b8c2f02e8a04e96ea Add macOS support to travis (Elichai Turkel)
b6807d91d83e9597ffecec999eb761b8571a1f26 Move travis script into a standalone sh file (Elichai Turkel)
Pull request description:
ACKs for top commit:
real-or-random:
ACK 71757da5ccece100b1eca6c70b4d87e2542cff97 I inspected the diff
jonasnick:
ACK 71757da5ccece100b1eca6c70b4d87e2542cff97
Tree-SHA512: e8fab725ef5ed98c795f39d7f26b5d967a6bd730d40eb7d9793986858bf34770b0350c1b7b1d14ae608dfff9375a0750ec67c8e6d0d4b562ab917f5e645aa67b
