From 3cb057f8429c812b5dbfcd43299658463162b740 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Thu, 1 Nov 2018 12:15:28 +0100 Subject: [PATCH 1/4] Fix possible integer overflow in DER parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we’re in the last loop iteration, then `lenleft == 1` and it could be the case that `ret == MAX_SIZE`, and so `ret + lenleft` will overflow to 0 and the sanity check will not catch it. Then we will return `(int) MAX_SIZE`, which should be avoided because this value is implementation-defined. (However, this is harmless because `(int) MAX_SIZE == -1` on all supported platforms.) --- src/ecdsa_impl.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h index c340004..4f62198 100644 --- a/src/ecdsa_impl.h +++ b/src/ecdsa_impl.h @@ -66,7 +66,7 @@ static int secp256k1_der_read_len(const unsigned char **sigp, const unsigned cha return -1; } /* X.690-207 8.1.3.5 long form length octets */ - lenleft = b1 & 0x7F; + lenleft = b1 & 0x7F; /* lenleft is at least 1 */ if (lenleft > sigend - *sigp) { return -1; } @@ -82,13 +82,13 @@ static int secp256k1_der_read_len(const unsigned char **sigp, const unsigned cha } while (lenleft > 0) { ret = (ret << 8) | **sigp; - if (ret + lenleft > (size_t)(sigend - *sigp)) { - /* Result exceeds the length of the passed array. */ - return -1; - } (*sigp)++; lenleft--; } + if (ret > (size_t)(sigend - *sigp)) { + /* Result exceeds the length of the passed array. */ + return -1; + } if (ret < 128) { /* Not the shortest possible length encoding. */ return -1; From 01ee1b3b3c665bf22e06b92afa832ccc54de5119 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 7 Nov 2018 16:07:33 +0100 Subject: [PATCH 2/4] Parse DER-enconded length into a size_t instead of an int This avoids a possibly implementation-defined signed (int) to unsigned (size_t) conversion portably. --- src/ecdsa_impl.h | 55 +++++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h index 4f62198..2a9b94a 100644 --- a/src/ecdsa_impl.h +++ b/src/ecdsa_impl.h @@ -46,68 +46,73 @@ static const secp256k1_fe secp256k1_ecdsa_const_p_minus_order = SECP256K1_FE_CON 0, 0, 0, 1, 0x45512319UL, 0x50B75FC4UL, 0x402DA172UL, 0x2FC9BAEEUL ); -static int secp256k1_der_read_len(const unsigned char **sigp, const unsigned char *sigend) { - int lenleft, b1; - size_t ret = 0; +static int secp256k1_der_read_len(size_t *len, const unsigned char **sigp, const unsigned char *sigend) { + size_t lenleft; + unsigned char b1; + VERIFY_CHECK(len != NULL); + *len = 0; if (*sigp >= sigend) { - return -1; + return 0; } b1 = *((*sigp)++); if (b1 == 0xFF) { /* X.690-0207 8.1.3.5.c the value 0xFF shall not be used. */ - return -1; + return 0; } if ((b1 & 0x80) == 0) { /* X.690-0207 8.1.3.4 short form length octets */ - return b1; + *len = b1; + return 1; } if (b1 == 0x80) { /* Indefinite length is not allowed in DER. */ - return -1; + return 0; } /* X.690-207 8.1.3.5 long form length octets */ lenleft = b1 & 0x7F; /* lenleft is at least 1 */ - if (lenleft > sigend - *sigp) { - return -1; + if (lenleft > (size_t)(sigend - *sigp)) { + return 0; } if (**sigp == 0) { /* Not the shortest possible length encoding. */ - return -1; + return 0; } - if ((size_t)lenleft > sizeof(size_t)) { + if (lenleft > sizeof(size_t)) { /* The resulting length would exceed the range of a size_t, so * certainly longer than the passed array size. */ - return -1; + return 0; } while (lenleft > 0) { - ret = (ret << 8) | **sigp; + *len = (*len << 8) | **sigp; (*sigp)++; lenleft--; } - if (ret > (size_t)(sigend - *sigp)) { + if (*len > (size_t)(sigend - *sigp)) { /* Result exceeds the length of the passed array. */ - return -1; + return 0; } - if (ret < 128) { + if (*len < 128) { /* Not the shortest possible length encoding. */ - return -1; + return 0; } - return ret; + return 1; } static int secp256k1_der_parse_integer(secp256k1_scalar *r, const unsigned char **sig, const unsigned char *sigend) { int overflow = 0; unsigned char ra[32] = {0}; - int rlen; + size_t rlen; if (*sig == sigend || **sig != 0x02) { /* Not a primitive integer (X.690-0207 8.3.1). */ return 0; } (*sig)++; - rlen = secp256k1_der_read_len(sig, sigend); - if (rlen <= 0 || (*sig) + rlen > sigend) { + if (secp256k1_der_read_len(&rlen, sig, sigend) == 0) { + return 0; + } + if (rlen == 0 || *sig + rlen > sigend) { /* Exceeds bounds or not at least length 1 (X.690-0207 8.3.1). */ return 0; } @@ -144,13 +149,15 @@ static int secp256k1_der_parse_integer(secp256k1_scalar *r, const unsigned char static int secp256k1_ecdsa_sig_parse(secp256k1_scalar *rr, secp256k1_scalar *rs, const unsigned char *sig, size_t size) { const unsigned char *sigend = sig + size; - int rlen; + size_t rlen; if (sig == sigend || *(sig++) != 0x30) { /* The encoding doesn't start with a constructed sequence (X.690-0207 8.9.1). */ return 0; } - rlen = secp256k1_der_read_len(&sig, sigend); - if (rlen < 0 || sig + rlen > sigend) { + if (secp256k1_der_read_len(&rlen, &sig, sigend) == 0) { + return 0; + } + if (sig + rlen > sigend) { /* Tuple exceeds bounds */ return 0; } From ec8f20babd8595f119e642d3833ee90bacc12873 Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 7 Nov 2018 16:17:57 +0100 Subject: [PATCH 3/4] Avoid out-of-bound pointers and integer overflows in size comparisons This changes pointer calculations in size comparions to a form that ensures that no out-of-bound pointers are computed, because even their computation yields undefined behavior. Also, this changes size comparions to a form that ensures that neither the left-hand side nor the right-hand side can overflow. --- contrib/lax_der_parsing.c | 6 +++--- src/ecdsa_impl.h | 8 ++------ src/hash_impl.h | 3 ++- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/contrib/lax_der_parsing.c b/contrib/lax_der_parsing.c index 5b141a9..e177a05 100644 --- a/contrib/lax_der_parsing.c +++ b/contrib/lax_der_parsing.c @@ -32,7 +32,7 @@ int ecdsa_signature_parse_der_lax(const secp256k1_context* ctx, secp256k1_ecdsa_ lenbyte = input[pos++]; if (lenbyte & 0x80) { lenbyte -= 0x80; - if (pos + lenbyte > inputlen) { + if (lenbyte > inputlen - pos) { return 0; } pos += lenbyte; @@ -51,7 +51,7 @@ int ecdsa_signature_parse_der_lax(const secp256k1_context* ctx, secp256k1_ecdsa_ lenbyte = input[pos++]; if (lenbyte & 0x80) { lenbyte -= 0x80; - if (pos + lenbyte > inputlen) { + if (lenbyte > inputlen - pos) { return 0; } while (lenbyte > 0 && input[pos] == 0) { @@ -89,7 +89,7 @@ int ecdsa_signature_parse_der_lax(const secp256k1_context* ctx, secp256k1_ecdsa_ lenbyte = input[pos++]; if (lenbyte & 0x80) { lenbyte -= 0x80; - if (pos + lenbyte > inputlen) { + if (lenbyte > inputlen - pos) { return 0; } while (lenbyte > 0 && input[pos] == 0) { diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h index 2a9b94a..b37aff4 100644 --- a/src/ecdsa_impl.h +++ b/src/ecdsa_impl.h @@ -157,12 +157,8 @@ static int secp256k1_ecdsa_sig_parse(secp256k1_scalar *rr, secp256k1_scalar *rs, if (secp256k1_der_read_len(&rlen, &sig, sigend) == 0) { return 0; } - if (sig + rlen > sigend) { - /* Tuple exceeds bounds */ - return 0; - } - if (sig + rlen != sigend) { - /* Garbage after tuple. */ + if (rlen != (size_t)(sigend - sig)) { + /* Tuple exceeds bounds or garage after tuple. */ return 0; } diff --git a/src/hash_impl.h b/src/hash_impl.h index 009f26b..782f972 100644 --- a/src/hash_impl.h +++ b/src/hash_impl.h @@ -131,7 +131,8 @@ static void secp256k1_sha256_transform(uint32_t* s, const uint32_t* chunk) { static void secp256k1_sha256_write(secp256k1_sha256 *hash, const unsigned char *data, size_t len) { size_t bufsize = hash->bytes & 0x3F; hash->bytes += len; - while (bufsize + len >= 64) { + VERIFY_CHECK(hash->bytes >= len); + while (len >= 64 - bufsize) { /* Fill the buffer, and process it. */ size_t chunk_len = 64 - bufsize; memcpy(((unsigned char*)hash->buf) + bufsize, data, chunk_len); From 14c7dbd4440f582e260da977cabb19d2cee506fa Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Wed, 7 Nov 2018 16:13:27 +0100 Subject: [PATCH 4/4] Simplify control flow in DER parsing --- src/ecdsa_impl.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/ecdsa_impl.h b/src/ecdsa_impl.h index b37aff4..eb099c8 100644 --- a/src/ecdsa_impl.h +++ b/src/ecdsa_impl.h @@ -128,8 +128,11 @@ static int secp256k1_der_parse_integer(secp256k1_scalar *r, const unsigned char /* Negative. */ overflow = 1; } - while (rlen > 0 && **sig == 0) { - /* Skip leading zero bytes */ + /* There is at most one leading zero byte: + * if there were two leading zero bytes, we would have failed and returned 0 + * because of excessive 0x00 padding already. */ + if (rlen > 0 && **sig == 0) { + /* Skip leading zero byte */ rlen--; (*sig)++; }