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

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

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).
 	 */

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

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).
 	 */

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