Page MenuHomePhabricator

Eliminate harmless non-constant time operations on secret data.
ClosedPublic

Authored by deadalnix on Mar 20 2020, 00:37.

Details

Summary
  • Eliminate harmless non-constant time operations on secret data.

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)

  • Adds a declassify operation to aid constant-time analysis.

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.

This is a backport of secp256k1 PR710

Test Plan
ninja check-secp256k1

Diff Detail

Repository
rABC Bitcoin ABC
Branch
secppr710
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9903
Build 17665: Default Diff Build & Tests
Build 17664: arc lint + arc unit

Event Timeline

Did you run the Travis build ?
Should we add a cmake option to enable valgrind as well ? That would at least make the CI cover the same tests between the 2 builds.

Comments can be addressed later on

This revision is now accepted and ready to land.Mar 26 2020, 16:31