Page MenuHomePhabricator

Merge #10657: Utils: Improvements to ECDSA key-handling code

Authored by markblundeberg on Jul 7 2019, 18:36.



PR10657 backport
63179d0 Scope the ECDSA constant sizes to CPubKey / CKey classes (Jack Grigg)
1ce9f0a Ensure that ECDSA constant sizes are correctly-sized (Jack Grigg)
48abe78 Remove redundant = 0 initialisations (Jack Grigg)
17fa391 Specify ECDSA constant sizes as constants (Jack Grigg)
e4a1086 Update Debian copyright list (Jack Grigg)
e181dbe Add comments (Jack Grigg)
a3603ac Fix potential overflows in ECDSA DER parsers (Jack Grigg)

Pull request description:

Mostly trivial, but includes fixes to potential overflows in the ECDSA DER parsers.

Cherry-picked from Zcash PR

Also backported fixup to use ptrdiff_t instead of size_t, so we don't
generate a signed-unsigned-comparison warning:

Note: the "potential overflows" here isn't anything substantial, rather
just conversion to the good-practice way to compare pointers. The
conversion from lenbyte >= sizeof(size_t) to lenbyte >= 4 does remove
platform-dependent behaviour, but only for signatures that violate
DERSIG and are >16 MiB in size, see discussion on PR.

Test Plan

make check

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 6790
Build 11627: Bitcoin ABC Buildbot (legacy)
Build 11626: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Jul 8 2019, 07:10
Fabien added inline comments.
25 ↗(On Diff #10054)

What's the point of updating the comment ?

55 ↗(On Diff #10054)


56 ↗(On Diff #10054)


57 ↗(On Diff #10054)


This revision now requires changes to proceed.Jul 8 2019, 07:10
markblundeberg marked 4 inline comments as done.

update per comments

25 ↗(On Diff #10054)

ah, thanks, this got accidentally dragged in from a merge conflict

deadalnix requested changes to this revision.Jul 9 2019, 01:53

The comment in the interpreter should probably be added somewhere in sigencoding.cpp

81 ↗(On Diff #10083)

As gmax points out, this is a consensus change. Because we enforce strict encoding, this shouldn't be a problem, BUT this surely warrant a full IBD without checkpoint or assumevalid to make sure there isn't something sneaky in the blockchain already.

120 ↗(On Diff #10083)


220 ↗(On Diff #10083)

As note for discussion we had on other diffs. This constant is 65. PUBLIC_KEY_SIZE, used bellow is also 65. Yet we are using two different constants with two different names, because they don't MEAN the same thing.

This revision now requires changes to proceed.Jul 9 2019, 01:53

The comment in the interpreter should probably be added somewhere in sigencoding.cpp

Yeah I tried to find a home for it but nothing 100% matches due to how we've modded it. I can add part of the comment to CheckRawECDSASignatureEncoding but we should drop the comment about "extra hashtype byte" since the byte (if it exists) is already stripped at that point.

81 ↗(On Diff #10083)

No problem -- @jasonbcox can you exercise your script and kick off a noassumevalid IBD? :-)

include comment (except part about hashtype byte) into sigencoding.cpp

(in CheckRawECDSASignatureEncoding there is no hashtype byte)

This revision is now accepted and ready to land.Jul 20 2019, 06:17