Page MenuHomePhabricator

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

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

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Project
Summary

PR10657 backport https://github.com/bitcoin/bitcoin/pull/10657/files
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 https://github.com/zcash/zcash/pull/2335

Also backported fixup to use ptrdiff_t instead of size_t, so we don't
generate a signed-unsigned-comparison warning:
PR12351 https://github.com/bitcoin/bitcoin/pull/12351/files

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

Repository
rABC Bitcoin ABC
Branch
PR10657
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6790
Build 11627: Bitcoin ABC Teamcity Staging
Build 11626: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Sun, Jul 7, 18:36
Herald added a reviewer: Restricted Project. · View Herald TranscriptSun, Jul 7, 18:36
Fabien requested changes to this revision.Mon, Jul 8, 07:10
Fabien added inline comments.
src/key.h
25 ↗(On Diff #10054)

What's the point of updating the comment ?

src/pubkey.h
55 ↗(On Diff #10054)

Revert

56 ↗(On Diff #10054)

Braces

57 ↗(On Diff #10054)

Dito

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

update per comments

markblundeberg added inline comments.Mon, Jul 8, 14:25
src/key.h
25 ↗(On Diff #10054)

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

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

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

src/pubkey.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)

dito

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.Tue, Jul 9, 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.

src/pubkey.cpp
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)

deadalnix accepted this revision.Fri, Jul 12, 23:57
Fabien accepted this revision.Sat, Jul 20, 06:17
This revision is now accepted and ready to land.Sat, Jul 20, 06:17