Skip to content

Commit

Permalink
consensus: guard against openssl's new strict DER checks
Browse files Browse the repository at this point in the history 
New versions of OpenSSL will reject non-canonical DER signatures. However,
it'll happily decode them. Decode then re-encode before verification in order
to ensure that it is properly consumed.

Github-Pull: #5634
Rebased-From: 488ed32
  • Loading branch information
@theuni @laanwj
theuni authored and laanwj committed Jan 10, 2015
1 parent 263b65e commit ace39db
Showing 1 changed file with 13 additions and 3 deletions.
16 changes: 13 additions & 3 deletions src/ecwrapper.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,20 @@ bool CECKey::SetPubKey(const unsigned char* pubkey, size_t size) {
}

bool CECKey::Verify(const uint256 &hash, const std::vector<unsigned char>& vchSig) {
// -1 = error, 0 = bad sig, 1 = good
if (ECDSA_verify(0, (unsigned char*)&hash, sizeof(hash), &vchSig[0], vchSig.size(), pkey) != 1)
// New versions of OpenSSL will reject non-canonical DER signatures. de/re-serialize first.
unsigned char *norm_der = NULL;

This comment has been minimized.

Sign in to view

Copy link
@SergioDemianLerner

SergioDemianLerner Jan 10, 2015

Contributor

I cannot find the reference in the documentation where it is said that when passing a norm_der == NULL to i2d_ECDSA_SIG(), it will allocate a string dynamically. The documentation states that if the argument is NULL (nor a pointer to NULL), then the length of the DER encoded ECDSA_SIG object will be returned. Obviously it's allocating something, because if not it would just core dump....
Documentation is here: https://www.openssl.org/docs/crypto/ECDSA_verify.html
ECDSA_SIG *norm_sig = ECDSA_SIG_new();
const unsigned char* sigptr = &vchSig[0];
d2i_ECDSA_SIG(&norm_sig, &sigptr, vchSig.size());

This comment has been minimized.

Sign in to view

Copy link
@SergioDemianLerner

SergioDemianLerner Jan 10, 2015

Contributor

In ECDSA_verify() OpenSSL 1.0.1g first does:

if (d2i_ECDSA_SIG(&s, &sigbuf, sig_len) == NULL) goto err;

but the code in this patch does not check the error code of d2i_ECDSA_SIG().
We cannot be sure that i2d_ECDSA_SIG() will catch such error later. Since d2i_ECDSA_SIG() is automatically generated by openssl/asn1t.h, I'm not sure when it can fail, but consensus may be broken using a specially crafted signature. I suggest the return value of d2i_ECDSA_SIG() of is checked in a similar way OpenSSL did before.
I suggest changing this to:
if (d2i_ECDSA_SIG(&norm_sig, &sigptr, vchSig.size()) == NULL)
return false;

This comment has been minimized.

Sign in to view

Copy link
@SergioDemianLerner

SergioDemianLerner Jan 10, 2015

Contributor

but call free OPENSSL_free(norm_sig) before returning.
int derlen = i2d_ECDSA_SIG(norm_sig, &norm_der);
ECDSA_SIG_free(norm_sig);
if (derlen <= 0)
return false;
return true;

// -1 = error, 0 = bad sig, 1 = good
bool ret = ECDSA_verify(0, (unsigned char*)&hash, sizeof(hash), norm_der, derlen, pkey) == 1;
OPENSSL_free(norm_der);
return ret;
}

bool CECKey::Recover(const uint256 &hash, const unsigned char *p64, int rec)
Expand Down

1 comment on commit ace39db

@rebroad
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply copy the openssl code as it was rather than use the new code with this kludge?

Please sign in to comment.