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

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

Details

Reviewers

deadalnix
Fabien
jasonbcox

Group Reviewers

Restricted Project

Commits

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

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markblundeberg created this revision.Jul 7 2019, 18:36

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 7 2019, 18:36

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6709: Diff 10054.Jul 7 2019, 18:47

Fabien requested changes to this revision.Jul 8 2019, 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.Jul 8 2019, 07:10

update per comments

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6731: Diff 10083.Jul 8 2019, 13:33

markblundeberg added inline comments.Jul 8 2019, 14:25

src/key.h
25 ↗	(On Diff #10054)	ah, thanks, this got accidentally dragged in from a merge conflict

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.Jul 9 2019, 01:53

In D3581#84954, @deadalnix wrote:

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.