From: Thomas Pornin Date: Mon, 21 Nov 2016 19:11:21 +0000 (+0100) Subject: Fixed buffer overflow, and also NULL pointer dereference, in ECDSA signature handling. X-Git-Tag: v0.4~45 X-Git-Url: https://bearssl.org/gitweb//home/git/?p=BearSSL;a=commitdiff_plain;h=e8ccee8bcdae80cdf74c6d7327f1c7572589fae3;ds=sidebyside 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 --- 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 --- 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 --- 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 --- 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 --- 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; }