Utils: Improvements to ECDSA key-handling code #10657

I guess that's not possible...

This change breaks the intentional compatibility with OpenSSL behavior; it also makes the commit message inaccurately described.

It doesn't matter for the Bitcoin system anymore because only strict DER matters there now, but this is otherwise an undisclosed consensus change (admittedly where OpenSSL creates inconsistency to begin with).

We made this change because the previous check was platform-dependent and therefore difficult to review for determinism (and also we don't use the lax code at all, and have removed it). As far as being an undisclosed consensus change, it's obviously not one that would end up affecting running consensus (because compilation would fail on the target machines this affects), but ACK that it would have been clearer in a separate commit. I can revert it for this PR if desired.

If lenbyte >= 4 here, then rlen calculated in the while loop just below will be >= 2²⁴ (because lenbyte at this point is the length of the representation of rlen after stripping leading zeros). So this function would in any case return 0 at line 90 if it does not return 0 here: the condition rlen > inputlen - pos would be true at line 90 since inputlen is at most the maximum block size, which is less than 2²⁴ bytes. @str4d's reasoning is not quite correct; the above argument isn't obvious and doesn't hold just because of the static_assert. In any case there is no consensus change (it doesn't matter whether we return 0 here or at line 90).

If this had actually made a difference, then there would have been a consensus incompatibility between 32-bit and 64-bit platforms, which would surely have been unintended. (I know that originally only 64-bit platforms were supported, but my understanding is that the current code is supposed to support both with no consensus differences.)

Edit: correct 2³² to 2²⁴. (The most significant byte, which has weight at least 2²⁴ when lenbyte >= 4, must be at least 1.)

If this had actually made a difference, then there would have been a consensus incompatibility between 32-bit and 64-bit platforms, which would surely have been unintended.

It was intended to preserve exact compatibility with OpenSSL which has the same behavior. (particularly during a time when this behavior was embargoed both for us and OpenSSL)

And it very much made a different prior to BIP66, it doesn't now-- but this parsing function exists entirely for compatibility with OpenSSL and would not have been used if Bitcoin used BIP66 since day one.

It didn't make any difference prior to BIP66, by the argument I gave above -- fortunately, because it would have been possible to fork 32-bit nodes from 64-bit nodes otherwise.

It is basically never correct to have a platform-dependent consensus check. In this case, platform-dependent consensus code happened not to result in a difference in behaviour, essentially by luck. IMHO it's a good idea to clean that up.

@daira It is absolutely never correct to have platform-dependent consensus checks. However, Bitcoin used to indirectly have such a check through OpenSSL (fixed in CVE-2014-8275, see https://www.openssl.org/news/secadv/20150108.txt), and it was in fact possible to fork 32-bit nodes from 64-bit nodes. BIP66 was exactly intended to fix that problem (see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2015-July/009697.html). The original code here just preserved the behaviour. No problem with removing the dependency now, though.

same here.

Same argument as above with rlen replaced by slen, and line 90 replaced by line 129.

Help me understand how you believe there is a potential overflow here.

My meat based range analysis states: Pos always has value 2 here. Size_t lenbyte is 0-127 inclusive (after the subtraction takes it from a range of 128-255 inclusive). This means that the left side (which is all size_t) has a range of 2-129.

What am I missing? (As an aside, zcash really screwed up if there is any reason to use this lax parser in it; this code exists for compatibility with OpenSSL...)

And likewise for the below.

See https://github.com/zcash/zcash/pull/2335#discussion_r114272216:

This is a different style to before but it's still overflow-prone (for integer overflows this time).

So this change was included for consistency with the other changes, not necessarily because this specific instance suffers from overflow, but because that style of pointer comparison can be vulnerable, and it is better to have good style precedent. There's no telling what future people might happen to do with that code snippet.

(As an aside, zcash really screwed up if there is any reason to use this lax parser in it; this code exists for compatibility with OpenSSL...)

In fact, we stripped out this code as unused and unnecessary in https://github.com/zcash/zcash/pull/2360, but for the purposes of upstreaming to Bitcoin Core where it was still present, we intentionally separated that change from these ones.

These seem like rather unrelated changes? Can they go in a separate PR?

They were related inasmuch as I was pulling in a collection of libsecp256k1 PRs from upstream, and this was a necessary libsecp256k1-related addition. From a Bitcoin PR perspective, I can split this out into a separate PR if desired.

yikes, please don't put these in the global namespace in a very public header file.

Same namespace issue here.