Skip to content

Commit

Permalink
Merge #10657: Utils: Improvements to ECDSA key-handling code
Browse files Browse the repository at this point in the history 
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 zcash/zcash#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`

Reviewers: deadalnix, Fabien, jasonbcox, #bitcoin_abc

Reviewed By: deadalnix, Fabien, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3581
  • Loading branch information
@laanwj @markblundeberg
laanwj authored and markblundeberg committed Jul 31, 2019
1 parent bb8fad2 commit 7309653
Show file tree
Hide file tree
Showing 6 changed files with 133 additions and 67 deletions.
14 changes: 14 additions & 0 deletions contrib/debian/copyright
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ Copyright: 2010-2011, Jonas Smedegaard <dr@jones.dk>
2017, freetrader <freetrader@tuta.io>
License: GPL-2+

Files: src/secp256k1/build-aux/m4/ax_jni_include_dir.m4
Copyright: 2008 Don Anderson <dda@sleepycat.com>
License: GNU-All-permissive-License

Files: src/secp256k1/build-aux/m4/ax_prog_cc_for_build.m4
Copyright: 2008 Paolo Bonzini <bonzini@gnu.org>
License: GNU-All-permissive-License

Files: src/qt/res/icons/add.png
src/qt/res/icons/address-book.png
src/qt/res/icons/chevron.png
Expand Down Expand Up @@ -111,6 +119,12 @@ License: Expat
TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

License: GNU-All-permissive-License
Copying and distribution of this file, with or without modification, are
permitted in any medium without royalty provided the copyright notice
and this notice are preserved. This file is offered as-is, without any
warranty.

License: GPL-2+
This program is free software; you can redistribute it and/or modify it
under the terms of the GNU General Public License as published by the
Expand Down
81 changes: 58 additions & 23 deletions src/key.cpp
View file
Original file line number Diff line number Diff line change
@@ -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.

Expand All @@ -20,57 +21,89 @@ static secp256k1_context *secp256k1_context_sign = nullptr;
* 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 <http://www.secg.org/sec1-v2.pdf>, 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, uint8_t *out32,
const uint8_t *privkey, size_t privkeylen) {
const uint8_t *end = privkey + privkeylen;
int lenb = 0;
int 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;
ptrdiff_t 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);
ptrdiff_t 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]);
ptrdiff_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;
}
return 1;
}

/**
* This serializes to a DER encoding of the ECPrivateKey type from section C.4
* of SEC 1 <http://www.secg.org/sec1-v2.pdf>. The optional parameters and
* publicKey fields are included.
*
* 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, uint8_t *privkey,
size_t *privkeylen, const uint8_t *key32,
int compressed) {
assert(*privkeylen >= CKey::PRIVATE_KEY_SIZE);
secp256k1_pubkey pubkey;
size_t pubkeylen = 0;
if (!secp256k1_ec_pubkey_create(ctx, &pubkey, key32)) {
Expand Down Expand Up @@ -102,11 +135,12 @@ static int ec_privkey_export_der(const secp256k1_context *ctx, uint8_t *privkey,
ptr += 32;
memcpy(ptr, middle, sizeof(middle));
ptr += sizeof(middle);
pubkeylen = 33;
pubkeylen = CPubKey::COMPRESSED_PUBLIC_KEY_SIZE;
secp256k1_ec_pubkey_serialize(ctx, ptr, &pubkeylen, &pubkey,
SECP256K1_EC_COMPRESSED);
ptr += pubkeylen;
*privkeylen = ptr - privkey;
assert(*privkeylen == CKey::COMPRESSED_PRIVATE_KEY_SIZE);
} else {
static const uint8_t begin[] = {0x30, 0x82, 0x01, 0x13, 0x02,
0x01, 0x01, 0x04, 0x20};
Expand Down Expand Up @@ -134,11 +168,12 @@ static int ec_privkey_export_der(const secp256k1_context *ctx, uint8_t *privkey,
ptr += 32;
memcpy(ptr, middle, sizeof(middle));
ptr += sizeof(middle);
pubkeylen = 65;
pubkeylen = CPubKey::PUBLIC_KEY_SIZE;
secp256k1_ec_pubkey_serialize(ctx, ptr, &pubkeylen, &pubkey,
SECP256K1_EC_UNCOMPRESSED);
ptr += pubkeylen;
*privkeylen = ptr - privkey;
assert(*privkeylen == CKey::PRIVATE_KEY_SIZE);
}
return 1;
}
Expand All @@ -160,8 +195,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, (uint8_t *)privkey.data(), &privkeylen, begin(),
fCompressed ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED);
Expand All @@ -173,7 +208,7 @@ CPrivKey CKey::GetPrivKey() const {
CPubKey CKey::GetPubKey() const {
assert(fValid);
secp256k1_pubkey pubkey;
size_t clen = 65;
size_t clen = CPubKey::PUBLIC_KEY_SIZE;
CPubKey result;
int ret =
secp256k1_ec_pubkey_create(secp256k1_context_sign, &pubkey, begin());
Expand All @@ -191,8 +226,8 @@ bool CKey::SignECDSA(const uint256 &hash, std::vector<uint8_t> &vchSig,
if (!fValid) {
return false;
}
vchSig.resize(72);
size_t nSigLen = 72;
vchSig.resize(CPubKey::SIGNATURE_SIZE);
size_t nSigLen = CPubKey::SIGNATURE_SIZE;
uint8_t extra_entropy[32] = {0};
WriteLE32(extra_entropy, test_case);
secp256k1_ecdsa_signature sig;
Expand Down Expand Up @@ -244,7 +279,7 @@ bool CKey::SignCompact(const uint256 &hash,
if (!fValid) {
return false;
}
vchSig.resize(65);
vchSig.resize(CPubKey::COMPACT_SIGNATURE_SIZE);
int rec = -1;
secp256k1_ecdsa_recoverable_signature sig;
int ret = secp256k1_ecdsa_sign_recoverable(
Expand Down Expand Up @@ -281,10 +316,10 @@ bool CKey::Derive(CKey &keyChild, ChainCode &ccChild, unsigned int nChild,
std::vector<uint8_t, secure_allocator<uint8_t>> vout(64);
if ((nChild >> 31) == 0) {
CPubKey pubkey = GetPubKey();
assert(pubkey.begin() + 33 == pubkey.end());
assert(pubkey.size() == CPubKey::COMPRESSED_PUBLIC_KEY_SIZE);
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);
Expand Down
29 changes: 17 additions & 12 deletions src/key.h
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Copyright (c) 2009-2010 Satoshi Nakamoto
// 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.

Expand All @@ -14,25 +15,29 @@
#include <stdexcept>
#include <vector>

/**
* secp256k1:
* const unsigned int PRIVATE_KEY_SIZE = 279;
* const unsigned int PUBLIC_KEY_SIZE = 65;
* const unsigned int SIGNATURE_SIZE = 72;
*
* see www.keylength.com
* script supports up to 75 for single byte push
*/

/**
* secure_allocator is defined in allocators.h
* CPrivKey is a serialized private key, with all parameters included (279
* bytes)
* CPrivKey is a serialized private key, with all parameters included
* (PRIVATE_KEY_SIZE bytes)
*/
typedef std::vector<uint8_t, secure_allocator<uint8_t>> CPrivKey;

/** An encapsulated secp256k1 private key. */
class CKey {
public:
/**
* secp256k1:
*/
static const unsigned int PRIVATE_KEY_SIZE = 279;
static const unsigned int COMPRESSED_PRIVATE_KEY_SIZE = 214;
/**
* see www.keylength.com
* script supports up to 75 for single byte push
*/
static_assert(
PRIVATE_KEY_SIZE >= COMPRESSED_PRIVATE_KEY_SIZE,
"COMPRESSED_PRIVATE_KEY_SIZE is larger than PRIVATE_KEY_SIZE");

private:
//! Whether this private key is valid. We check for correctness when
//! modifying the key data, so fValid should always correspond to the actual
Expand Down
33 changes: 18 additions & 15 deletions src/pubkey.cpp
View file
Original file line number Diff line number Diff line change
@@ -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.

Expand Down Expand Up @@ -50,7 +51,7 @@ static int ecdsa_signature_parse_der_lax(const secp256k1_context *ctx,
lenbyte = input[pos++];
if (lenbyte & 0x80) {
lenbyte -= 0x80;
if (pos + lenbyte > inputlen) {
if (lenbyte > inputlen - pos) {
return 0;
}
pos += lenbyte;
Expand All @@ -69,14 +70,15 @@ static int ecdsa_signature_parse_der_lax(const secp256k1_context *ctx,
lenbyte = input[pos++];
if (lenbyte & 0x80) {
lenbyte -= 0x80;
if (pos + lenbyte > inputlen) {
if (lenbyte > inputlen - pos) {
return 0;
}
while (lenbyte > 0 && input[pos] == 0) {
pos++;
lenbyte--;
}
if (lenbyte >= sizeof(size_t)) {
static_assert(sizeof(size_t) >= 4, "size_t too small");
if (lenbyte >= 4) {
return 0;
}
rlen = 0;
Expand Down Expand Up @@ -107,14 +109,15 @@ static int ecdsa_signature_parse_der_lax(const secp256k1_context *ctx,
lenbyte = input[pos++];
if (lenbyte & 0x80) {
lenbyte -= 0x80;
if (pos + lenbyte > inputlen) {
if (lenbyte > inputlen - pos) {
return 0;
}
while (lenbyte > 0 && input[pos] == 0) {
pos++;
lenbyte--;
}
if (lenbyte >= sizeof(size_t)) {
static_assert(sizeof(size_t) >= 4, "size_t too small");
if (lenbyte >= 4) {
return 0;
}
slen = 0;
Expand Down Expand Up @@ -214,7 +217,7 @@ bool CPubKey::VerifySchnorr(const uint256 &hash,

bool CPubKey::RecoverCompact(const uint256 &hash,
const std::vector<uint8_t> &vchSig) {
if (vchSig.size() != 65) {
if (vchSig.size() != COMPACT_SIGNATURE_SIZE) {
return false;
}

Expand All @@ -230,8 +233,8 @@ bool CPubKey::RecoverCompact(const uint256 &hash,
hash.begin())) {
return false;
}
uint8_t pub[65];
size_t publen = 65;
uint8_t pub[PUBLIC_KEY_SIZE];
size_t publen = PUBLIC_KEY_SIZE;
secp256k1_ec_pubkey_serialize(
secp256k1_context_verify, pub, &publen, &pubkey,
fComp ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED);
Expand All @@ -257,8 +260,8 @@ bool CPubKey::Decompress() {
&(*this)[0], size())) {
return false;
}
uint8_t pub[65];
size_t publen = 65;
uint8_t pub[PUBLIC_KEY_SIZE];
size_t publen = PUBLIC_KEY_SIZE;
secp256k1_ec_pubkey_serialize(secp256k1_context_verify, pub, &publen,
&pubkey, SECP256K1_EC_UNCOMPRESSED);
Set(pub, pub + publen);
Expand All @@ -269,7 +272,7 @@ bool CPubKey::Derive(CPubKey &pubkeyChild, ChainCode &ccChild,
unsigned int nChild, const ChainCode &cc) const {
assert(IsValid());
assert((nChild >> 31) == 0);
assert(begin() + 33 == end());
assert(size() == COMPRESSED_PUBLIC_KEY_SIZE);
uint8_t out[64];
BIP32Hash(cc, nChild, *begin(), begin() + 1, out);
memcpy(ccChild.begin(), out + 32, 32);
Expand All @@ -282,8 +285,8 @@ bool CPubKey::Derive(CPubKey &pubkeyChild, ChainCode &ccChild,
out)) {
return false;
}
uint8_t pub[33];
size_t publen = 33;
uint8_t pub[COMPRESSED_PUBLIC_KEY_SIZE];
size_t publen = COMPRESSED_PUBLIC_KEY_SIZE;
secp256k1_ec_pubkey_serialize(secp256k1_context_verify, pub, &publen,
&pubkey, SECP256K1_EC_COMPRESSED);
pubkeyChild.Set(pub, pub + publen);
Expand All @@ -298,8 +301,8 @@ void CExtPubKey::Encode(uint8_t code[BIP32_EXTKEY_SIZE]) const {
code[7] = (nChild >> 8) & 0xFF;
code[8] = (nChild >> 0) & 0xFF;
memcpy(code + 9, chaincode.begin(), 32);
assert(pubkey.size() == 33);
memcpy(code + 41, pubkey.begin(), 33);
assert(pubkey.size() == CPubKey::COMPRESSED_PUBLIC_KEY_SIZE);
memcpy(code + 41, pubkey.begin(), CPubKey::COMPRESSED_PUBLIC_KEY_SIZE);
}

void CExtPubKey::Decode(const uint8_t code[BIP32_EXTKEY_SIZE]) {
Expand Down
Loading

0 comments on commit 7309653

Please sign in to comment.