Fixed buffer overflow, and also NULL pointer dereference, in ECDSA signature handling.

diff --git a/src/ec/ecdsa_atr.c b/src/ec/ecdsa_atr.c
index 91a1d0c..3a11226 100644 (file)
--- a/src/ec/ecdsa_atr.c
+++ b/src/ec/ecdsa_atr.c
@@ -33,6 +33,11 @@ br_ecdsa_asn1_to_raw(void *sig, size_t sig_len)
         * deviations to DER with regards to minimality of encoding of
         * lengths and integer values. These deviations are still
         * unambiguous.
+        *
+        * Signature format is a SEQUENCE of two INTEGER values. We
+        * support only integers of less than 127 bytes each (signed
+        * encoding) so the resulting raw signature will have length
+        * at most 254 bytes.
         */
 
        unsigned char *buf, *r, *s;
@@ -43,9 +48,20 @@ br_ecdsa_asn1_to_raw(void *sig, size_t sig_len)
        if (sig_len < 8) {
                return 0;
        }
+
+       /*
+        * First byte is SEQUENCE tag.
+        */
        if (buf[0] != 0x30) {
                return 0;
        }
+
+       /*
+        * The SEQUENCE length will be encoded over one or two bytes. We
+        * limit the total SEQUENCE contents to 255 bytes, because it
+        * makes things simpler; this is enough for subgroup orders up
+        * to 999 bits.
+        */
        zlen = buf[1];
        if (zlen > 0x80) {
                if (zlen != 0x81) {
@@ -62,6 +78,10 @@ br_ecdsa_asn1_to_raw(void *sig, size_t sig_len)
                }
                off = 2;
        }
+
+       /*
+        * First INTEGER (r).
+        */
        if (buf[off ++] != 0x02) {
                return 0;
        }
@@ -71,6 +91,10 @@ br_ecdsa_asn1_to_raw(void *sig, size_t sig_len)
        }
        r = buf + off;
        off += rlen;
+
+       /*
+        * Second INTEGER (s).
+        */
        if (off + 2 > sig_len) {
                return 0;
        }
@@ -83,6 +107,9 @@ br_ecdsa_asn1_to_raw(void *sig, size_t sig_len)
        }
        s = buf + off;
 
+       /*
+        * Removing leading zeros from r and s.
+        */
        while (rlen > 0 && *r == 0) {
                rlen --;
                r ++;
@@ -92,6 +119,11 @@ br_ecdsa_asn1_to_raw(void *sig, size_t sig_len)
                s ++;
        }
 
+       /*
+        * Compute common length for the two integers, then copy integers
+        * into the temporary buffer, and finally copy it back over the
+        * signature buffer.
+        */
        zlen = rlen > slen ? rlen : slen;
        sig_len = zlen << 1;
        memset(tmp, 0, sig_len);

diff --git a/src/ec/ecdsa_i31_sign_raw.c b/src/ec/ecdsa_i31_sign_raw.c
index 3849545..df20c24 100644 (file)
--- a/src/ec/ecdsa_i31_sign_raw.c
+++ b/src/ec/ecdsa_i31_sign_raw.c
@@ -50,6 +50,13 @@ br_ecdsa_i31_sign_raw(const br_ec_impl *impl,
        uint32_t n0i, ctl;
        br_hmac_drbg_context drbg;
 
+       /*
+        * If the curve is not supported, then exit with an error.
+        */
+       if (((impl->supported_curves >> sk->curve) & 1) == 0) {
+               return 0;
+       }
+
        /*
         * Get the curve parameters (generator and order).
         */

diff --git a/src/ec/ecdsa_i31_vrfy_asn1.c b/src/ec/ecdsa_i31_vrfy_asn1.c
index e98b779..4161aaa 100644 (file)
--- a/src/ec/ecdsa_i31_vrfy_asn1.c
+++ b/src/ec/ecdsa_i31_vrfy_asn1.c
@@ -33,9 +33,13 @@ br_ecdsa_i31_vrfy_asn1(const br_ec_impl *impl,
        const br_ec_public_key *pk,
        const void *sig, size_t sig_len)
 {
-       unsigned char rsig[(FIELD_LEN << 1) + 12];
+       /*
+        * We use a double-sized buffer because a malformed ASN.1 signature
+        * may trigger a size expansion when converting to "raw" format.
+        */
+       unsigned char rsig[(FIELD_LEN << 2) + 24];
 
-       if (sig_len > sizeof rsig) {
+       if (sig_len > ((sizeof rsig) >> 1)) {
                return 0;
        }
        memcpy(rsig, sig, sig_len);

diff --git a/src/ec/ecdsa_i31_vrfy_raw.c b/src/ec/ecdsa_i31_vrfy_raw.c
index 54dcfc2..8af9597 100644 (file)
--- a/src/ec/ecdsa_i31_vrfy_raw.c
+++ b/src/ec/ecdsa_i31_vrfy_raw.c
@@ -47,6 +47,13 @@ br_ecdsa_i31_vrfy_raw(const br_ec_impl *impl,
        size_t nlen, rlen, ulen;
        uint32_t n0i, res;
 
+       /*
+        * If the curve is not supported, then report an error.
+        */
+       if (((impl->supported_curves >> pk->curve) & 1) == 0) {
+               return 0;
+       }
+
        /*
         * Get the curve parameters (generator and order).
         */

diff --git a/src/ec/ecdsa_rta.c b/src/ec/ecdsa_rta.c
index 71f0d39..005c62c 100644 (file)
--- a/src/ec/ecdsa_rta.c
+++ b/src/ec/ecdsa_rta.c
@@ -24,6 +24,13 @@
 
 #include "inner.h"
 
+/*
+ * Compute ASN.1 encoded length for the provided integer. The ASN.1
+ * encoding is signed, so its leading bit must have value 0; it must
+ * also be of minimal length (so leading bytes of value 0 must be
+ * removed, except if that would contradict the rule about the sign
+ * bit).
+ */
 static size_t
 asn1_int_length(const unsigned char *x, size_t xlen)
 {
@@ -44,7 +51,7 @@ br_ecdsa_raw_to_asn1(void *sig, size_t sig_len)
        /*
         * Internal buffer is large enough to accommodate a signature
         * such that r and s fit on 125 bytes each (signed encoding),
-        * meaning a curve order of up to 1000 bits. This is the limit
+        * meaning a curve order of up to 999 bits. This is the limit
         * that ensures "simple" length encodings.
         */
        unsigned char *buf;
@@ -55,12 +62,20 @@ br_ecdsa_raw_to_asn1(void *sig, size_t sig_len)
        if ((sig_len & 1) != 0) {
                return 0;
        }
+
+       /*
+        * Compute lengths for the two integers.
+        */
        hlen = sig_len >> 1;
        rlen = asn1_int_length(buf, hlen);
        slen = asn1_int_length(buf + hlen, hlen);
        if (rlen > 125 || slen > 125) {
                return 0;
        }
+
+       /*
+        * SEQUENCE header.
+        */
        tmp[0] = 0x30;
        zlen = rlen + slen + 4;
        if (zlen >= 0x80) {
@@ -71,6 +86,10 @@ br_ecdsa_raw_to_asn1(void *sig, size_t sig_len)
                tmp[1] = zlen;
                off = 2;
        }
+
+       /*
+        * First INTEGER (r).
+        */
        tmp[off ++] = 0x02;
        tmp[off ++] = rlen;
        if (rlen > hlen) {
@@ -80,6 +99,10 @@ br_ecdsa_raw_to_asn1(void *sig, size_t sig_len)
                memcpy(tmp + off, buf + hlen - rlen, rlen);
        }
        off += rlen;
+
+       /*
+        * Second INTEGER (s).
+        */
        tmp[off ++] = 0x02;
        tmp[off ++] = slen;
        if (slen > hlen) {
@@ -89,6 +112,10 @@ br_ecdsa_raw_to_asn1(void *sig, size_t sig_len)
                memcpy(tmp + off, buf + sig_len - slen, slen);
        }
        off += slen;
+
+       /*
+        * Return ASN.1 signature.
+        */
        memcpy(sig, tmp, off);
        return off;
 }