From a3603ac6f07966036e56554cd754a57791a3491a Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 3 May 2017 00:14:55 +1200 Subject: Fix potential overflows in ECDSA DER parsers --- src/key.cpp | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) (limited to 'src/key.cpp') diff --git a/src/key.cpp b/src/key.cpp index 5a991fc1d2..f581bcb0c8 100644 --- a/src/key.cpp +++ b/src/key.cpp @@ -1,4 +1,5 @@ // Copyright (c) 2009-2016 The Bitcoin Core developers +// Copyright (c) 2017 The Zcash developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -18,41 +19,46 @@ static secp256k1_context* secp256k1_context_sign = NULL; /** These functions are taken from the libsecp256k1 distribution and are very ugly. */ static int ec_privkey_import_der(const secp256k1_context* ctx, unsigned char *out32, const unsigned char *privkey, size_t privkeylen) { const unsigned char *end = privkey + privkeylen; - int lenb = 0; - int len = 0; + size_t lenb = 0; + size_t len = 0; memset(out32, 0, 32); /* sequence header */ - if (end < privkey+1 || *privkey != 0x30) { + if (end - privkey < 1 || *privkey != 0x30u) { return 0; } privkey++; /* sequence length constructor */ - if (end < privkey+1 || !(*privkey & 0x80)) { + if (end - privkey < 1 || !(*privkey & 0x80u)) { return 0; } - lenb = *privkey & ~0x80; privkey++; + lenb = *privkey & ~0x80u; privkey++; if (lenb < 1 || lenb > 2) { return 0; } - if (end < privkey+lenb) { + if (end - privkey < lenb) { return 0; } /* sequence length */ - len = privkey[lenb-1] | (lenb > 1 ? privkey[lenb-2] << 8 : 0); + len = privkey[lenb-1] | (lenb > 1 ? privkey[lenb-2] << 8 : 0u); privkey += lenb; - if (end < privkey+len) { + if (end - privkey < len) { return 0; } /* sequence element 0: version number (=1) */ - if (end < privkey+3 || privkey[0] != 0x02 || privkey[1] != 0x01 || privkey[2] != 0x01) { + if (end - privkey < 3 || privkey[0] != 0x02u || privkey[1] != 0x01u || privkey[2] != 0x01u) { return 0; } privkey += 3; /* sequence element 1: octet string, up to 32 bytes */ - if (end < privkey+2 || privkey[0] != 0x04 || privkey[1] > 0x20 || end < privkey+2+privkey[1]) { + if (end - privkey < 2 || privkey[0] != 0x04u) { return 0; } - memcpy(out32 + 32 - privkey[1], privkey + 2, privkey[1]); + size_t oslen = privkey[1]; + privkey += 2; + if (oslen > 32 || end - privkey < oslen) { + return 0; + } + memcpy(out32 + (32 - oslen), privkey, oslen); if (!secp256k1_ec_seckey_verify(ctx, out32)) { memset(out32, 0, 32); return 0; @@ -219,10 +225,10 @@ bool CKey::Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const std::vector> vout(64); if ((nChild >> 31) == 0) { CPubKey pubkey = GetPubKey(); - assert(pubkey.begin() + 33 == pubkey.end()); + assert(pubkey.size() == 33); BIP32Hash(cc, nChild, *pubkey.begin(), pubkey.begin()+1, vout.data()); } else { - assert(begin() + 32 == end()); + assert(size() == 32); BIP32Hash(cc, nChild, 0, begin(), vout.data()); } memcpy(ccChild.begin(), vout.data()+32, 32); -- cgit v1.2.3 From e181dbe7482424d2658df08e7a0c3cd832839edf Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 6 Jun 2017 17:44:17 +1200 Subject: Add comments --- src/key.cpp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) (limited to 'src/key.cpp') diff --git a/src/key.cpp b/src/key.cpp index f581bcb0c8..31d485722a 100644 --- a/src/key.cpp +++ b/src/key.cpp @@ -17,6 +17,22 @@ static secp256k1_context* secp256k1_context_sign = NULL; /** These functions are taken from the libsecp256k1 distribution and are very ugly. */ + +/** + * This parses a format loosely based on a DER encoding of the ECPrivateKey type from + * section C.4 of SEC 1 , with the following caveats: + * + * * The octet-length of the SEQUENCE must be encoded as 1 or 2 octets. It is not + * required to be encoded as one octet if it is less than 256, as DER would require. + * * The octet-length of the SEQUENCE must not be greater than the remaining + * length of the key encoding, but need not match it (i.e. the encoding may contain + * junk after the encoded SEQUENCE). + * * The privateKey OCTET STRING is zero-filled on the left to 32 octets. + * * Anything after the encoding of the privateKey OCTET STRING is ignored, whether + * or not it is validly encoded DER. + * + * out32 must point to an output buffer of length at least 32 bytes. + */ static int ec_privkey_import_der(const secp256k1_context* ctx, unsigned char *out32, const unsigned char *privkey, size_t privkeylen) { const unsigned char *end = privkey + privkeylen; size_t lenb = 0; @@ -66,6 +82,13 @@ static int ec_privkey_import_der(const secp256k1_context* ctx, unsigned char *ou return 1; } +/** + * This serializes to a DER encoding of the ECPrivateKey type from section C.4 of SEC 1 + * . The optional parameters and publicKey fields are + * included. + * + * key32 must point to a 32-byte raw private key. + */ static int ec_privkey_export_der(const secp256k1_context *ctx, unsigned char *privkey, size_t *privkeylen, const unsigned char *key32, int compressed) { secp256k1_pubkey pubkey; size_t pubkeylen = 0; -- cgit v1.2.3 From 17fa3913ef7ba5f569ebc3695fab15b10dd914f0 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 6 Jun 2017 19:21:34 +1200 Subject: Specify ECDSA constant sizes as constants --- src/key.cpp | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) (limited to 'src/key.cpp') diff --git a/src/key.cpp b/src/key.cpp index 31d485722a..3b679414d1 100644 --- a/src/key.cpp +++ b/src/key.cpp @@ -87,9 +87,13 @@ static int ec_privkey_import_der(const secp256k1_context* ctx, unsigned char *ou * . The optional parameters and publicKey fields are * included. * + * privkey must point to an output buffer of length at least PRIVATE_KEY_SIZE bytes. + * privkeylen must initially be set to the size of the privkey buffer. Upon return it + * will be set to the number of bytes used in the buffer. * key32 must point to a 32-byte raw private key. */ static int ec_privkey_export_der(const secp256k1_context *ctx, unsigned char *privkey, size_t *privkeylen, const unsigned char *key32, int compressed) { + assert(*privkeylen >= PRIVATE_KEY_SIZE); secp256k1_pubkey pubkey; size_t pubkeylen = 0; if (!secp256k1_ec_pubkey_create(ctx, &pubkey, key32)) { @@ -115,10 +119,11 @@ static int ec_privkey_export_der(const secp256k1_context *ctx, unsigned char *pr memcpy(ptr, begin, sizeof(begin)); ptr += sizeof(begin); memcpy(ptr, key32, 32); ptr += 32; memcpy(ptr, middle, sizeof(middle)); ptr += sizeof(middle); - pubkeylen = 33; + pubkeylen = COMPRESSED_PUBLIC_KEY_SIZE; secp256k1_ec_pubkey_serialize(ctx, ptr, &pubkeylen, &pubkey, SECP256K1_EC_COMPRESSED); ptr += pubkeylen; *privkeylen = ptr - privkey; + assert(*privkeylen == COMPRESSED_PRIVATE_KEY_SIZE); } else { static const unsigned char begin[] = { 0x30,0x82,0x01,0x13,0x02,0x01,0x01,0x04,0x20 @@ -140,10 +145,11 @@ static int ec_privkey_export_der(const secp256k1_context *ctx, unsigned char *pr memcpy(ptr, begin, sizeof(begin)); ptr += sizeof(begin); memcpy(ptr, key32, 32); ptr += 32; memcpy(ptr, middle, sizeof(middle)); ptr += sizeof(middle); - pubkeylen = 65; + pubkeylen = PUBLIC_KEY_SIZE; secp256k1_ec_pubkey_serialize(ctx, ptr, &pubkeylen, &pubkey, SECP256K1_EC_UNCOMPRESSED); ptr += pubkeylen; *privkeylen = ptr - privkey; + assert(*privkeylen == PRIVATE_KEY_SIZE); } return 1; } @@ -165,8 +171,8 @@ CPrivKey CKey::GetPrivKey() const { CPrivKey privkey; int ret; size_t privkeylen; - privkey.resize(279); - privkeylen = 279; + privkey.resize(PRIVATE_KEY_SIZE); + privkeylen = PRIVATE_KEY_SIZE; ret = ec_privkey_export_der(secp256k1_context_sign, (unsigned char*) privkey.data(), &privkeylen, begin(), fCompressed ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED); assert(ret); privkey.resize(privkeylen); @@ -176,7 +182,7 @@ CPrivKey CKey::GetPrivKey() const { CPubKey CKey::GetPubKey() const { assert(fValid); secp256k1_pubkey pubkey; - size_t clen = 65; + size_t clen = PUBLIC_KEY_SIZE; CPubKey result; int ret = secp256k1_ec_pubkey_create(secp256k1_context_sign, &pubkey, begin()); assert(ret); @@ -189,8 +195,8 @@ CPubKey CKey::GetPubKey() const { bool CKey::Sign(const uint256 &hash, std::vector& vchSig, uint32_t test_case) const { if (!fValid) return false; - vchSig.resize(72); - size_t nSigLen = 72; + vchSig.resize(SIGNATURE_SIZE); + size_t nSigLen = SIGNATURE_SIZE; unsigned char extra_entropy[32] = {0}; WriteLE32(extra_entropy, test_case); secp256k1_ecdsa_signature sig; @@ -218,7 +224,7 @@ bool CKey::VerifyPubKey(const CPubKey& pubkey) const { bool CKey::SignCompact(const uint256 &hash, std::vector& vchSig) const { if (!fValid) return false; - vchSig.resize(65); + vchSig.resize(COMPACT_SIGNATURE_SIZE); int rec = -1; secp256k1_ecdsa_recoverable_signature sig; int ret = secp256k1_ecdsa_sign_recoverable(secp256k1_context_sign, &sig, hash.begin(), begin(), secp256k1_nonce_function_rfc6979, NULL); @@ -248,7 +254,7 @@ bool CKey::Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const std::vector> vout(64); if ((nChild >> 31) == 0) { CPubKey pubkey = GetPubKey(); - assert(pubkey.size() == 33); + assert(pubkey.size() == COMPRESSED_PUBLIC_KEY_SIZE); BIP32Hash(cc, nChild, *pubkey.begin(), pubkey.begin()+1, vout.data()); } else { assert(size() == 32); -- cgit v1.2.3 From 48abe78e51e9a51fba8b93ff7faa32a14a2aa50c Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 6 Jun 2017 20:28:37 +1200 Subject: Remove redundant `= 0` initialisations --- src/key.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'src/key.cpp') diff --git a/src/key.cpp b/src/key.cpp index 3b679414d1..3a93187d76 100644 --- a/src/key.cpp +++ b/src/key.cpp @@ -35,8 +35,6 @@ static secp256k1_context* secp256k1_context_sign = NULL; */ static int ec_privkey_import_der(const secp256k1_context* ctx, unsigned char *out32, const unsigned char *privkey, size_t privkeylen) { const unsigned char *end = privkey + privkeylen; - size_t lenb = 0; - size_t len = 0; memset(out32, 0, 32); /* sequence header */ if (end - privkey < 1 || *privkey != 0x30u) { @@ -47,7 +45,7 @@ static int ec_privkey_import_der(const secp256k1_context* ctx, unsigned char *ou if (end - privkey < 1 || !(*privkey & 0x80u)) { return 0; } - lenb = *privkey & ~0x80u; privkey++; + size_t lenb = *privkey & ~0x80u; privkey++; if (lenb < 1 || lenb > 2) { return 0; } @@ -55,7 +53,7 @@ static int ec_privkey_import_der(const secp256k1_context* ctx, unsigned char *ou return 0; } /* sequence length */ - len = privkey[lenb-1] | (lenb > 1 ? privkey[lenb-2] << 8 : 0u); + size_t len = privkey[lenb-1] | (lenb > 1 ? privkey[lenb-2] << 8 : 0u); privkey += lenb; if (end - privkey < len) { return 0; -- cgit v1.2.3 From 1ce9f0a952a3d5d9442ad8251da898d96209c16c Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 8 Jun 2017 16:07:49 +1200 Subject: Ensure that ECDSA constant sizes are correctly-sized --- src/key.cpp | 3 +++ 1 file changed, 3 insertions(+) (limited to 'src/key.cpp') diff --git a/src/key.cpp b/src/key.cpp index 3a93187d76..42301e81a0 100644 --- a/src/key.cpp +++ b/src/key.cpp @@ -92,6 +92,9 @@ static int ec_privkey_import_der(const secp256k1_context* ctx, unsigned char *ou */ static int ec_privkey_export_der(const secp256k1_context *ctx, unsigned char *privkey, size_t *privkeylen, const unsigned char *key32, int compressed) { assert(*privkeylen >= PRIVATE_KEY_SIZE); + static_assert( + PRIVATE_KEY_SIZE >= COMPRESSED_PRIVATE_KEY_SIZE, + "COMPRESSED_PRIVATE_KEY_SIZE is larger than PRIVATE_KEY_SIZE"); secp256k1_pubkey pubkey; size_t pubkeylen = 0; if (!secp256k1_ec_pubkey_create(ctx, &pubkey, key32)) { -- cgit v1.2.3 From 63179d028347bf3e32c7ea61386df4c44307b4a7 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 4 Oct 2017 14:41:40 +0100 Subject: Scope the ECDSA constant sizes to CPubKey / CKey classes --- src/key.cpp | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) (limited to 'src/key.cpp') diff --git a/src/key.cpp b/src/key.cpp index 42301e81a0..0ddf4e1fc4 100644 --- a/src/key.cpp +++ b/src/key.cpp @@ -85,16 +85,13 @@ static int ec_privkey_import_der(const secp256k1_context* ctx, unsigned char *ou * . The optional parameters and publicKey fields are * included. * - * privkey must point to an output buffer of length at least PRIVATE_KEY_SIZE bytes. + * privkey must point to an output buffer of length at least CKey::PRIVATE_KEY_SIZE bytes. * privkeylen must initially be set to the size of the privkey buffer. Upon return it * will be set to the number of bytes used in the buffer. * key32 must point to a 32-byte raw private key. */ static int ec_privkey_export_der(const secp256k1_context *ctx, unsigned char *privkey, size_t *privkeylen, const unsigned char *key32, int compressed) { - assert(*privkeylen >= PRIVATE_KEY_SIZE); - static_assert( - PRIVATE_KEY_SIZE >= COMPRESSED_PRIVATE_KEY_SIZE, - "COMPRESSED_PRIVATE_KEY_SIZE is larger than PRIVATE_KEY_SIZE"); + assert(*privkeylen >= CKey::PRIVATE_KEY_SIZE); secp256k1_pubkey pubkey; size_t pubkeylen = 0; if (!secp256k1_ec_pubkey_create(ctx, &pubkey, key32)) { @@ -120,11 +117,11 @@ static int ec_privkey_export_der(const secp256k1_context *ctx, unsigned char *pr memcpy(ptr, begin, sizeof(begin)); ptr += sizeof(begin); memcpy(ptr, key32, 32); ptr += 32; memcpy(ptr, middle, sizeof(middle)); ptr += sizeof(middle); - pubkeylen = COMPRESSED_PUBLIC_KEY_SIZE; + pubkeylen = CPubKey::COMPRESSED_PUBLIC_KEY_SIZE; secp256k1_ec_pubkey_serialize(ctx, ptr, &pubkeylen, &pubkey, SECP256K1_EC_COMPRESSED); ptr += pubkeylen; *privkeylen = ptr - privkey; - assert(*privkeylen == COMPRESSED_PRIVATE_KEY_SIZE); + assert(*privkeylen == CKey::COMPRESSED_PRIVATE_KEY_SIZE); } else { static const unsigned char begin[] = { 0x30,0x82,0x01,0x13,0x02,0x01,0x01,0x04,0x20 @@ -146,11 +143,11 @@ static int ec_privkey_export_der(const secp256k1_context *ctx, unsigned char *pr memcpy(ptr, begin, sizeof(begin)); ptr += sizeof(begin); memcpy(ptr, key32, 32); ptr += 32; memcpy(ptr, middle, sizeof(middle)); ptr += sizeof(middle); - pubkeylen = PUBLIC_KEY_SIZE; + pubkeylen = CPubKey::PUBLIC_KEY_SIZE; secp256k1_ec_pubkey_serialize(ctx, ptr, &pubkeylen, &pubkey, SECP256K1_EC_UNCOMPRESSED); ptr += pubkeylen; *privkeylen = ptr - privkey; - assert(*privkeylen == PRIVATE_KEY_SIZE); + assert(*privkeylen == CKey::PRIVATE_KEY_SIZE); } return 1; } @@ -183,7 +180,7 @@ CPrivKey CKey::GetPrivKey() const { CPubKey CKey::GetPubKey() const { assert(fValid); secp256k1_pubkey pubkey; - size_t clen = PUBLIC_KEY_SIZE; + size_t clen = CPubKey::PUBLIC_KEY_SIZE; CPubKey result; int ret = secp256k1_ec_pubkey_create(secp256k1_context_sign, &pubkey, begin()); assert(ret); @@ -196,8 +193,8 @@ CPubKey CKey::GetPubKey() const { bool CKey::Sign(const uint256 &hash, std::vector& vchSig, uint32_t test_case) const { if (!fValid) return false; - vchSig.resize(SIGNATURE_SIZE); - size_t nSigLen = SIGNATURE_SIZE; + vchSig.resize(CPubKey::SIGNATURE_SIZE); + size_t nSigLen = CPubKey::SIGNATURE_SIZE; unsigned char extra_entropy[32] = {0}; WriteLE32(extra_entropy, test_case); secp256k1_ecdsa_signature sig; @@ -225,7 +222,7 @@ bool CKey::VerifyPubKey(const CPubKey& pubkey) const { bool CKey::SignCompact(const uint256 &hash, std::vector& vchSig) const { if (!fValid) return false; - vchSig.resize(COMPACT_SIGNATURE_SIZE); + vchSig.resize(CPubKey::COMPACT_SIGNATURE_SIZE); int rec = -1; secp256k1_ecdsa_recoverable_signature sig; int ret = secp256k1_ecdsa_sign_recoverable(secp256k1_context_sign, &sig, hash.begin(), begin(), secp256k1_nonce_function_rfc6979, NULL); @@ -255,7 +252,7 @@ bool CKey::Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const std::vector> vout(64); if ((nChild >> 31) == 0) { CPubKey pubkey = GetPubKey(); - assert(pubkey.size() == COMPRESSED_PUBLIC_KEY_SIZE); + assert(pubkey.size() == CPubKey::COMPRESSED_PUBLIC_KEY_SIZE); BIP32Hash(cc, nChild, *pubkey.begin(), pubkey.begin()+1, vout.data()); } else { assert(size() == 32); -- cgit v1.2.3