Eliminate harmless non-constant time operations on secret data. #710

gmaxwell · 2020-01-11T01:07:17Z

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)

gmaxwell · 2020-01-11T01:10:21Z

Only the "Eliminate harmless" "declassify" commits are new, the rest are in other PRs.

This also doesn't touch the privkey tweak functions yet, because I had forgotten about them.

With this PR the valgrind test is totally clean except for overflow and s==0 in signing (and except for whatever functions like the tweak that should be tested but aren't tested yet).

Nonce overflow is a real non-constant timeness but security irrelevant because it never happens.

s==0 test is not actually a branch on secret data, but is instead a false positive owing to the fact that I haven't put in anything yet to tell valgrind that the data is no longer secret at that point-- though that will be easy to do.

This was made somewhat more complicated by preserving the output-zeroization on failure.

gmaxwell · 2020-01-11T13:38:08Z

With the latest commit the tests run clean.

gmaxwell · 2020-01-15T18:33:01Z

I added a fixups commit which fixes the seckey verify and tweak operations which were previously not tested.

There is a remaining issue that the the scalar code uses "th += (c0 < tl) ? 1 : 0;" which doesn't always compile to constant time code. E.g. 32-bit scalar on power9 w/ GCC 9.2.1 turns some of the muladds into branches (e.g. src/scalar_8x32_impl.h:444).

gmaxwell · 2020-01-15T18:54:40Z

I could use some help figuring out an alternative for that sort of carry which is equally fast and doesn't sometimes get turned into branches.

Similarly, testing this with different compilers/build-options/arches would be useful to learn if there are any other missed cases.

I think this is probably enough progress to merge-- it does at least make the tests clean on my desktop--, if people would like I can drop off the tester commit and I could work towards merging this much even though there are a few platform/compiler specific issues remaining.

gmaxwell · 2020-01-25T20:16:59Z

Anyone here?

elichai

I reviewed all the cmov implementations in godbolt with latest 3 versions of gcc,clang,icc and WINE MSVC 2017 x64

Personally I'd prefer to split between the changes and the valgrind addition./

I like that the valgrind call is wrapped in #if defined(VALGRIND) but Idk about the addition of int to the context just for this, but I don't feel knowledgeable enough to comment on this seriously.

src/ecmult_gen_impl.h

src/field_10x26_impl.h

src/field_5x52_impl.h

src/modules/ecdh/main_impl.h

src/scalar_8x32_impl.h

src/scalar_4x64_impl.h

src/secp256k1.c

src/util.h

src/secp256k1.c

sipa

Concept ACK, nothing seems painfully invasive here or detrimental to performance.

src/ecmult_gen_impl.h

src/modules/ecdh/main_impl.h

src/secp256k1.c

real-or-random · 2020-02-01T15:22:41Z

I'll have a closer look next week.

real-or-random

Let me repeat that this great stuff and very helpful.

real-or-random · 2020-02-03T15:28:38Z

src/eckey_impl.h


    secp256k1_scalar_mul(key, key, tweak);
-    return 1;
+    return ret;


note: This is superficially related to #701 because now this does the tweaking even if the tweak is zero, which funnily zeroizes the key then (even if we will decide that the public wrapper function won't.)

src/ecdsa_impl.h

src/scalar_impl.h

src/secp256k1.c

src/ecmult_gen_impl.h

src/secp256k1.c

src/valgrind_ctime_test.c

jonasnick

Needs rebase, but otherwise looks good to me.

gmaxwell · 2020-02-11T01:20:43Z

Dropped the testing commit, it's now only in #708 which is rebased on top of this branch. I'll keep rebasing 708 based on this one now.

gmaxwell · 2020-02-11T05:38:40Z

I think I've lost track of where this is in review. I fixed some things, explained some things, closed some comments, but feel kind of lost.

real-or-random · 2020-02-11T11:15:27Z

I think I've lost track of where this is in review. I fixed some things, explained some things, closed some comments, but feel kind of lost.

I understand... I always feel lost when looking at this PR because the comments are on many different commits... I pinged you on the two conversations which you haven't addressed. They're both not crucial, it's just my try of making everyone feel less lost and organize the PR.

There's nothing else from my side, I can ACK this after your reply.

real-or-random · 2020-02-11T12:38:17Z

I think you also missed that one:
#710 (comment)

gmaxwell · 2020-02-11T12:46:00Z

:-/ In fact I didn't miss it, I made the change and then must have lost it in a rebase. I wonder if I lost any other changes at the same time.

jonasnick

ACK 3eb69f6

src/secp256k1.c

src/ecmult_gen_impl.h

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)

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.

jonasnick

reACK 7b50483

sipa · 2020-02-22T04:34:19Z

utACK 7b50483

real-or-random · 2020-02-24T13:01:47Z

ACK 7b50483 I read the code carefully and tested it

…e non ct operators 2860950 Add tests for the cmov implementations (Elichai Turkel) 73596a8 Add ecdsa_sign_recoverable to the ctime tests (Elichai Turkel) 2876af4 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 2860950 real-or-random: ACK 2860950 read the diff, tested with valgrind including ctime tests Tree-SHA512: 4730301dcb62241d79f18eb8fed7e9ab0e20d1663a788832cb6cf4126baa7075807dc31896764b6f82d52742fdb636abc6b75e4344c6f117305904c628a5ad59

e10439c scripted-diff: rename privkey with seckey in secp256k1 interface (Pieter Wuille) ca8bc42 Drop --disable-jni from libsecp256k1 configure options (Pieter Wuille) ddc2419 Update MSVC build config for libsecp256k1 (Pieter Wuille) 67f232b Squashed 'src/secp256k1/' changes from b19c000..2ed54da (Pieter Wuille) Pull request description: It's been abound a year since the subtree was updated. Here is a list of the included PRs: * bitcoin-core/secp256k1#755: Recovery signing: add to constant time test, and eliminate non ct operators * bitcoin-core/secp256k1#754: Fix uninit values passed into cmov * bitcoin-core/secp256k1#752: autoconf: Use ":" instead of "dnl" as a noop * bitcoin-core/secp256k1#750: Add macOS to the CI * bitcoin-core/secp256k1#701: Make ec_ arithmetic more consistent and add documentation * bitcoin-core/secp256k1#732: Retry if r is zero during signing * bitcoin-core/secp256k1#742: Fix typo in ecmult_const_impl.h * bitcoin-core/secp256k1#740: Make recovery/main_impl.h non-executable * bitcoin-core/secp256k1#735: build: fix OpenSSL EC detection on macOS * bitcoin-core/secp256k1#728: Suppress a harmless variable-time optimization by clang in memczero * bitcoin-core/secp256k1#722: Context isn't freed in the ECDH benchmark * bitcoin-core/secp256k1#700: Allow overriding default flags * bitcoin-core/secp256k1#708: Constant-time behaviour test using valgrind memtest. * bitcoin-core/secp256k1#710: Eliminate harmless non-constant time operations on secret data. * bitcoin-core/secp256k1#718: Clarify that a secp256k1_ecdh_hash_function must return 0 or 1 * bitcoin-core/secp256k1#714: doc: document the length requirements of output parameter. * bitcoin-core/secp256k1#682: Remove Java Native Interface * bitcoin-core/secp256k1#713: Docstrings * bitcoin-core/secp256k1#704: README: add a section for test coverage * bitcoin-core/secp256k1#709: Remove secret-dependant non-constant time operation in ecmult_const. * bitcoin-core/secp256k1#703: Overhaul README.md * bitcoin-core/secp256k1#689: Remove "except in benchmarks" exception for fp math * bitcoin-core/secp256k1#679: Add SECURITY.md * bitcoin-core/secp256k1#685: Fix issue where travis does not show the ./tests seed… * bitcoin-core/secp256k1#690: Add valgrind check to travis * bitcoin-core/secp256k1#678: Preventing compiler optimizations in benchmarks without a memory fence * bitcoin-core/secp256k1#688: Fix ASM setting in travis * bitcoin-core/secp256k1#684: Make no-float policy explicit * bitcoin-core/secp256k1#677: Remove note about heap allocation in secp256k1_ecmult_odd_multiples_table_storage_var * bitcoin-core/secp256k1#647: Increase robustness against UB in secp256k1_scalar_cadd_bit * bitcoin-core/secp256k1#664: Remove mention of ec_privkey_export because it doesn't exist * bitcoin-core/secp256k1#337: variable sized precomputed table for signing * bitcoin-core/secp256k1#661: Make ./configure string consistent * bitcoin-core/secp256k1#657: Fix a nit in the recovery tests * bitcoin-core/secp256k1#650: secp256k1/src/tests.c: Properly handle sscanf return value * bitcoin-core/secp256k1#654: Fix typo (∞) * bitcoin-core/secp256k1#583: JNI: fix use sig array * bitcoin-core/secp256k1#644: Avoid optimizing out a verify_check * bitcoin-core/secp256k1#652: README.md: update instruction to run tests * bitcoin-core/secp256k1#651: Fix typo in secp256k1_preallocated.h * bitcoin-core/secp256k1#640: scalar_impl.h: fix includes * bitcoin-core/secp256k1#655: jni: Use only Guava for hex encoding and decoding * bitcoin-core/secp256k1#634: Add a descriptive comment for secp256k1_ecmult_const. * bitcoin-core/secp256k1#631: typo in comment for secp256k1_ec_pubkey_tweak_mul () * bitcoin-core/secp256k1#629: Avoid calling _is_zero when _set_b32 fails. * bitcoin-core/secp256k1#630: Note intention of timing sidechannel freeness. * bitcoin-core/secp256k1#628: Fix ability to compile tests without -DVERIFY. * bitcoin-core/secp256k1#627: Guard memcmp in tests against mixed size inputs. * bitcoin-core/secp256k1#578: Avoid implementation-defined and undefined behavior when dealing with sizes * bitcoin-core/secp256k1#595: Allow to use external default callbacks * bitcoin-core/secp256k1#600: scratch space: use single allocation * bitcoin-core/secp256k1#592: Use trivial algorithm in ecmult_multi if scratch space is small * bitcoin-core/secp256k1#566: Enable context creation in preallocated memory * bitcoin-core/secp256k1#596: Make WINDOW_G configurable * bitcoin-core/secp256k1#561: Respect LDFLAGS and #undef STATIC_PRECOMPUTATION if using basic config * bitcoin-core/secp256k1#533: Make sure we're not using an uninitialized variable in secp256k1_wnaf_const(...) * bitcoin-core/secp256k1#617: Pass scalar by reference in secp256k1_wnaf_const() * bitcoin-core/secp256k1#619: Clear a copied secret key after negation * bitcoin-core/secp256k1#612: Allow field_10x26_arm.s to compile for ARMv7 architecture ACKs for top commit: real-or-random: ACK e10439c I verified the diff (subtree matches my local tree, manual inspection of other commits) but I didn't tested the resulting code fanquake: ACK e10439c Sjors: ACK e10439c jonasnick: reACK e10439c Tree-SHA512: eb6284a485da78e9d2ed3f771df85560d47c770ebf480a0d4121ab356ad26be101a2b973efe412f26e6c142bc1dbd2efbb5cc08774233e41918c59fe3dff3387

…tants ade5b36 tests: add checks for scalar constants `secp256k1_scalar_{zero,one}` (Sebastian Falbesoner) 654246c refactor: take use of `secp256k1_scalar_{zero,one}` constants (Sebastian Falbesoner) Pull request description: Rather than allocating a (non-constant) scalar variable on the stack with the sole purpose of setting it to a constant value, the global constants `secp256k1_scalar_{zero,one}` (apparently introduced in 34a67c7, PR #710) can be directly used instead for the values 0 or 1. There is very likely not even a difference in run-time, but it leads to simpler and less code which might be nice. ACKs for top commit: sipa: utACK ade5b36 real-or-random: utACK ade5b36 Tree-SHA512: 0ff05a449c153f7117a4a56efef04b2087c2330f4692f3390a0b1d95573785ac7ae3fe689ed0ec2ecc64b575d2489d6e341d32567e75a1a4b4d458c3ecd406a1

gmaxwell mentioned this pull request Jan 15, 2020

Constant-time behaviour test using valgrind memtest. #708

Merged

gmaxwell changed the title ~~[WIP] Eliminate harmless non-constant time operations on secret data.~~ Eliminate harmless non-constant time operations on secret data. Jan 15, 2020

elichai reviewed Jan 26, 2020

View reviewed changes

sipa mentioned this pull request Feb 1, 2020

secp256k1_ecdh_hash_function must return 0 or 1 rust-bitcoin/rust-secp256k1#196

Closed

sipa reviewed Feb 1, 2020

View reviewed changes

src/ecmult_gen_impl.h Outdated Show resolved Hide resolved

src/modules/ecdh/main_impl.h Show resolved Hide resolved

src/secp256k1.c Outdated Show resolved Hide resolved

real-or-random reviewed Feb 3, 2020

View reviewed changes

jonasnick reviewed Feb 10, 2020

View reviewed changes

jonasnick approved these changes Feb 11, 2020

View reviewed changes

real-or-random reviewed Feb 12, 2020

View reviewed changes

src/secp256k1.c Show resolved Hide resolved

real-or-random reviewed Feb 12, 2020

View reviewed changes

src/ecmult_gen_impl.h Outdated Show resolved Hide resolved

gmaxwell added 2 commits February 20, 2020 17:27

jonasnick approved these changes Feb 21, 2020

View reviewed changes

real-or-random approved these changes Feb 24, 2020

View reviewed changes

real-or-random merged commit 96d8ccb into bitcoin-core:master Feb 24, 2020

real-or-random mentioned this pull request Mar 4, 2020

Erroneous comment and VERIFY_CHECK in ecdsa_sig_sign #720

Closed

jonasnick mentioned this pull request Mar 7, 2020

Make ec_ arithmetic more consistent and add documentation #701

Merged

benma mentioned this pull request Apr 21, 2020

rebase on bitcoin-core/secp256k1 BlockstreamResearch/secp256k1-zkp#85

Closed

elichai mentioned this pull request May 3, 2020

Add macOS to the CI #750

Merged

This was referenced May 11, 2020

Constant-time tests for libsecp256k1 rust-bitcoin/rust-secp256k1#213

Open

Bump libsecp256k1 rust-bitcoin/rust-secp256k1#216

Closed

elichai mentioned this pull request May 27, 2020

Recovery signing: add to constant time test, and eliminate non ct operators #755

Merged

sipa mentioned this pull request Jun 9, 2020

Update libsecp256k1 subtree bitcoin/bitcoin#19228

Merged

theStack mentioned this pull request May 29, 2023

refactor: take use of secp256k1_scalar_{zero,one} constants #1330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate harmless non-constant time operations on secret data. #710

Eliminate harmless non-constant time operations on secret data. #710

gmaxwell commented Jan 11, 2020

gmaxwell commented Jan 11, 2020 •

edited

Loading

gmaxwell commented Jan 11, 2020

gmaxwell commented Jan 15, 2020 •

edited

Loading

gmaxwell commented Jan 15, 2020 •

edited

Loading

gmaxwell commented Jan 25, 2020

elichai left a comment •

edited

Loading

sipa left a comment

real-or-random commented Feb 1, 2020

real-or-random left a comment

real-or-random Feb 3, 2020

jonasnick left a comment

gmaxwell commented Feb 11, 2020

gmaxwell commented Feb 11, 2020

real-or-random commented Feb 11, 2020

real-or-random commented Feb 11, 2020

gmaxwell commented Feb 11, 2020

jonasnick left a comment

jonasnick left a comment

sipa commented Feb 22, 2020

real-or-random commented Feb 24, 2020

Eliminate harmless non-constant time operations on secret data. #710

Eliminate harmless non-constant time operations on secret data. #710

Conversation

gmaxwell commented Jan 11, 2020

gmaxwell commented Jan 11, 2020 • edited Loading

gmaxwell commented Jan 11, 2020

gmaxwell commented Jan 15, 2020 • edited Loading

gmaxwell commented Jan 15, 2020 • edited Loading

gmaxwell commented Jan 25, 2020

elichai left a comment • edited Loading

Choose a reason for hiding this comment

sipa left a comment

Choose a reason for hiding this comment

real-or-random commented Feb 1, 2020

real-or-random left a comment

Choose a reason for hiding this comment

real-or-random Feb 3, 2020

Choose a reason for hiding this comment

jonasnick left a comment

Choose a reason for hiding this comment

gmaxwell commented Feb 11, 2020

gmaxwell commented Feb 11, 2020

real-or-random commented Feb 11, 2020

real-or-random commented Feb 11, 2020

gmaxwell commented Feb 11, 2020

jonasnick left a comment

Choose a reason for hiding this comment

jonasnick left a comment

Choose a reason for hiding this comment

sipa commented Feb 22, 2020

real-or-random commented Feb 24, 2020

gmaxwell commented Jan 11, 2020 •

edited

Loading

gmaxwell commented Jan 15, 2020 •

edited

Loading

gmaxwell commented Jan 15, 2020 •

edited

Loading

elichai left a comment •

edited

Loading