Merge bitcoin#578: Avoid implementation-defined and undefined behavio…

…r when dealing with sizes

14c7dbd Simplify control flow in DER parsing (Tim Ruffing)
ec8f20b Avoid out-of-bound pointers and integer overflows in size comparisons (Tim Ruffing)
01ee1b3 Parse DER-enconded length into a size_t instead of an int (Tim Ruffing)
3cb057f Fix possible integer overflow in DER parsing (Tim Ruffing)

Pull request description:

  This is a result of auditing the code for overflow issues at random places. None of this is critical but I think all of it should be fixed.

  I know this touches "red" code. I double-checked and triple-checked this but I can understand if some of the changes are not desirable because they change well-tested code.

  Best reviewed in individual commits.

ACKs for commit 14c7db:

Tree-SHA512: 312dd3f961739752e1a861e75bd755920f634f87ee9668793e102c224434e8d21367452e114de729322c71a89f4fa82126aa5d32742f2bbbc091777c99515e10

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) {

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;
-    if (lenleft > sigend - *sigp) {
-        return -1;
+    lenleft = b1 & 0x7F; /* lenleft is at least 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;
-        if (ret + lenleft > (size_t)(sigend - *sigp)) {
-            /* Result exceeds the length of the passed array. */
-            return -1;
-        }
+        *len = (*len << 8) | **sigp;
         (*sigp)++;
         lenleft--;
     }
-    if (ret < 128) {
+    if (*len > (size_t)(sigend - *sigp)) {
+        /* Result exceeds the length of the passed array. */
+        return 0;
+    }
+    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;
     }
@@ -123,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)++;
     }
@@ -144,18 +152,16 @@ 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) {
-        /* Tuple exceeds bounds */
+    if (secp256k1_der_read_len(&rlen, &sig, sigend) == 0) {
         return 0;
     }
-    if (sig + rlen != sigend) {
-        /* Garbage after tuple. */
+    if (rlen != (size_t)(sigend - sig)) {
+        /* Tuple exceeds bounds or garage after tuple. */
         return 0;
     }
 

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);