ACK: code reviewed, and fixes unit test errors running on OSX with OpenSSL 1.0.1k

untested ACK

tested ACK

@sipa afaik the iterator isn't guaranteed to be a pointer to the element there. Added an early return instead, you ok with that?

Tested ACK. (tests and a invalidate/reconsider loop back to 338221. Reindexed without checkpoints from 0 to 198k (still in progress))

Also +1 Tested ACK from an anonymous tester on IRC.

ut ACK

Tested ACK. As mentioned by Gavin, verified this fixes the unit tests on OSX when building with 1.0.1k

Un

tested ack, invalidated to 322000 reconsidered upto 327086 (still running)

tested ACK, -checkpoints=0 reindexes on testnet and mainnet.

This patch serves no purpose other than providing a "hack" around a flaw in the Bitcoin core. Previously OpenSSL did not look for invalid DER encoded data with possible trailing garbage but in the latest version it does. The actual problem is that Bitcoin accepts invalid DER encoded signatures and this is completely unrelated to OpenSSL. Now you are taking the invalid-DER encoded data and DER encoding it before calling ECDSA_verify. Because of this the same operation will be performed twice for every signature verification wasting CPU cycles. You MUST always DER encode your signature before passing it off to OpenSSL and this is nothing new. So this being "low" on OpenSSL's list is correct from my expert opinion as it is only performing validation and Bitcoin is passing invalid data.

ECDSA_verify:

int ECDSA_verify(int type, const unsigned char *dgst, int dgst_len,
    const unsigned char *sigbuf, int sig_len, EC_KEY *eckey)
{
ECDSA_SIG *s;
const unsigned char *p = sigbuf;
unsigned char *der = NULL;
int derlen = -1;
int ret=-1;

s = ECDSA_SIG_new();
if (s == NULL) return(ret);
if (d2i_ECDSA_SIG(&s, &p, sig_len) == NULL) goto err;
/* Ensure signature uses DER and doesn't have trailing garbage */
derlen = i2d_ECDSA_SIG(s, &der);
if (derlen != sig_len || memcmp(sigbuf, der, derlen))
    goto err;
ret=ECDSA_do_verify(dgst, dgst_len, s, eckey);
err:
if (derlen > 0)
    {
    OPENSSL_cleanse(der, derlen);
    OPENSSL_free(der);
    }
ECDSA_SIG_free(s);
return(ret);
}

@john-connor It is quite relevant in consensus systems, which must maintain bug-for-bug compatibility. It is impossible to rewind history, and un-accept signatures that have been previously accepted.

@jgarzik The bug is in Bitcoin, not OpenSSL. This patch does not fix the bug. It is a hack.

@jgarzik I know the blockchain is permanently flawed because of this bug but gavin and everybody else is blaming OpenSSL it seems. Accept the fact that Bitcoin core has a major flaw in that it does not DER encode signatures. This is something that could become fatal to Bitcoin in the future if OpenSSL were to further enforce it's fully understood rules. Why not fix the problem and hard fork instead of hacking the code?

@john-connor "Trailing garbage" is technically incorrect. What OpenSSL's ECDSA_verify previously supported was a very broad subset of BER. For example, encoding R and S with leading zeros. This is substantive, non-backwards compatible, arguably somewhat under-disclosed (seems that you were also unaware of the actual change, in that you refer to it as "trailing garbage"), breaking API change from the existing and widely known behavior. Signatures come from the outside world (Bitcoin Core itself has never generated anything but strictly minimal encodings), and the decoding functions are OpenSSL internal. As a result previously accepted encodings (and not just 'trailing garbage') will be rejected.

While this may not matter for most applications, a switch to be more conservative is not something that can be done in an uncoordinated way for a consensus system without resulting in an opening for funds loss. This is just the state of things, and not a question of "flaw" or blame. My description (http://sourceforge.net/p/bitcoin/mailman/message/33221963/) of the matter is just a statement of the facts about it.

@rnicoll

The documentation does say that.... but openssl itself seems to be calling ECDSA_SIG_new() before d2i_ECDSA_SIG.

Possibly there's a memory leak in openssl itself here.

@rnicoll Objectively, the current version does not leak (I tested with valgrind.) OpenSSL itself allocates first, so preserving its own behavior seemed to be conservative. The code in question appears to check the nullness of the pointer and allocates only if null, meaning the documentation is actually dangerously incorrect (since if you believed the documentation you may send uninitialized memory in there and have it fail to allocate).

Thanks for noticing th documentation disagreed, previously I'd just valgrinded and checked the code.

@gmaxwell Great, thanks for checking.

d2i_ECDSA_SIG does indeed segfault if passed an uninitialized pointer.

https://gist.github.com/pstratem/7d31cf5f35d78640247f

Would there be a new point release or a patch for 0.9.3? This cannot be cleanly applied to 0.9.3 as-is (ecwrapper.cpp is missing), and I thought it would be better to ask anyway to avoid blindly patching critical piece of software.

Try checking out and/or cherry-picking from the v0.9.4 tag, it has the same changes.

I'll just checkout v0.9.4 then. Thanks!