Remove support for indefinite lengths in crypto/asn1.
This simplifies the ASN1_get_object calling convention and removes
another significant source of tasn_dec.c complexity. This change does
not affect our PKCS#7 and PKCS#12 parsers.
Update-Note: Invalid certificates (and the few external structures using
asn1t.h) with BER indefinite lengths will now be rejected.
Bug: 354
Change-Id: I723036798fc3254d0a289c77b105fcbdcda309b2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50287
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c
index 1954bea..fbf4d68 100644
--- a/crypto/asn1/asn1_lib.c
+++ b/crypto/asn1/asn1_lib.c
@@ -104,8 +104,7 @@
OPENSSL_DECLARE_ERROR_REASON(ASN1, UNKNOWN_TAG)
OPENSSL_DECLARE_ERROR_REASON(ASN1, UNSUPPORTED_TYPE)
-static int asn1_get_length(const unsigned char **pp, int *inf, long *rl,
- long max);
+static int asn1_get_length(const unsigned char **pp, long *rl, long max);
static void asn1_put_length(unsigned char **pp, int length);
int ASN1_get_object(const unsigned char **pp, long *plength, int *ptag,
@@ -114,7 +113,7 @@
int i, ret;
long l;
const unsigned char *p = *pp;
- int tag, xclass, inf;
+ int tag, xclass;
long max = omax;
if (!max)
@@ -153,54 +152,43 @@
*ptag = tag;
*pclass = xclass;
- if (!asn1_get_length(&p, &inf, plength, max))
+ if (!asn1_get_length(&p, plength, max))
goto err;
- if (inf && !(ret & V_ASN1_CONSTRUCTED))
- goto err;
-
-#if 0
- fprintf(stderr, "p=%d + *plength=%ld > omax=%ld + *pp=%d (%d > %d)\n",
- (int)p, *plength, omax, (int)*pp, (int)(p + *plength),
- (int)(omax + *pp));
-
-#endif
if (*plength > (omax - (p - *pp))) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_TOO_LONG);
return 0x80;
}
*pp = p;
- return (ret | inf);
+ return ret;
err:
OPENSSL_PUT_ERROR(ASN1, ASN1_R_HEADER_TOO_LONG);
return 0x80;
}
-static int asn1_get_length(const unsigned char **pp, int *inf, long *rl,
- long max)
+static int asn1_get_length(const unsigned char **pp, long *rl, long max)
{
const unsigned char *p = *pp;
unsigned long ret = 0;
unsigned long i;
- if (max-- < 1)
+ if (max-- < 1) {
return 0;
+ }
if (*p == 0x80) {
- *inf = 1;
- ret = 0;
- p++;
+ /* We do not support BER indefinite-length encoding. */
+ return 0;
+ }
+ i = *p & 0x7f;
+ if (*(p++) & 0x80) {
+ if (i > sizeof(ret) || max < (long)i)
+ return 0;
+ while (i-- > 0) {
+ ret <<= 8L;
+ ret |= *(p++);
+ }
} else {
- *inf = 0;
- i = *p & 0x7f;
- if (*(p++) & 0x80) {
- if (i > sizeof(ret) || max < (long)i)
- return 0;
- while (i-- > 0) {
- ret <<= 8L;
- ret |= *(p++);
- }
- } else
- ret = i;
+ ret = i;
}
/*
* Bound the length to comfortably fit in an int. Lengths in this module
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 96f9f9c..4d8ed1b 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -1678,6 +1678,11 @@
int tag_class;
EXPECT_EQ(0x80, ASN1_get_object(&ptr, &length, &tag, &tag_class,
sizeof(kTruncated)));
+
+ static const uint8_t kIndefinite[] = {0x30, 0x80, 0x00, 0x00};
+ ptr = kIndefinite;
+ EXPECT_EQ(0x80, ASN1_get_object(&ptr, &length, &tag, &tag_class,
+ sizeof(kIndefinite)));
}
// The ASN.1 macros do not work on Windows shared library builds, where usage of
diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c
index c45be7a..beb9a0b 100644
--- a/crypto/asn1/tasn_dec.c
+++ b/crypto/asn1/tasn_dec.c
@@ -75,11 +75,9 @@
#define ASN1_MAX_CONSTRUCTED_NEST 30
static int asn1_check_eoc(const unsigned char **in, long len);
-static int asn1_find_end(const unsigned char **in, long len, char inf);
static int asn1_check_tlen(long *olen, int *otag, unsigned char *oclass,
- char *inf, char *cst,
- const unsigned char **in, long len,
+ char *cst, const unsigned char **in, long len,
int exptag, int expclass, char opt, ASN1_TLC *ctx);
static int asn1_template_ex_d2i(ASN1_VALUE **pval,
@@ -166,7 +164,7 @@
const ASN1_EXTERN_FUNCS *ef;
const unsigned char *p = NULL, *q;
unsigned char oclass;
- char seq_eoc, seq_nolen, cst, isopt;
+ char cst, isopt;
int i;
int otag;
int ret = 0;
@@ -222,7 +220,7 @@
p = *in;
/* Just read in tag and class */
- ret = asn1_check_tlen(NULL, &otag, &oclass, NULL, NULL,
+ ret = asn1_check_tlen(NULL, &otag, &oclass, NULL,
&p, len, -1, 0, 1, ctx);
if (!ret) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR);
@@ -328,15 +326,13 @@
aclass = V_ASN1_UNIVERSAL;
}
/* Get SEQUENCE length and update len, p */
- ret = asn1_check_tlen(&len, NULL, NULL, &seq_eoc, &cst,
+ ret = asn1_check_tlen(&len, NULL, NULL, &cst,
&p, len, tag, aclass, opt, ctx);
if (!ret) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR);
goto err;
} else if (ret == -1)
return -1;
- /* If indefinite we don't do a length check */
- seq_nolen = seq_eoc;
if (!cst) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_SEQUENCE_NOT_CONSTRUCTED);
goto err;
@@ -377,15 +373,12 @@
if (!len)
break;
q = p;
+ /* TODO(https://crbug.com/boringssl/455): Although we've removed
+ * indefinite-length support, this check is not quite a no-op.
+ * Reject [UNIVERSAL 0] in the tag parsers themselves. */
if (asn1_check_eoc(&p, len)) {
- if (!seq_eoc) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_UNEXPECTED_EOC);
- goto err;
- }
- len -= p - q;
- seq_eoc = 0;
- q = p;
- break;
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_UNEXPECTED_EOC);
+ goto err;
}
/*
* This determines the OPTIONAL flag value. The field cannot be
@@ -417,13 +410,8 @@
len -= p - q;
}
- /* Check for EOC if expecting one */
- if (seq_eoc && !asn1_check_eoc(&p, len)) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_MISSING_EOC);
- goto err;
- }
/* Check all data read */
- if (!seq_nolen && len) {
+ if (len) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_SEQUENCE_LENGTH_MISMATCH);
goto err;
}
@@ -494,7 +482,6 @@
int ret;
long len;
const unsigned char *p, *q;
- char exp_eoc;
if (!val)
return 0;
flags = tt->flags;
@@ -509,7 +496,7 @@
* Need to work out amount of data available to the inner content and
* where it starts: so read in EXPLICIT header to get the info.
*/
- ret = asn1_check_tlen(&len, NULL, NULL, &exp_eoc, &cst,
+ ret = asn1_check_tlen(&len, NULL, NULL, &cst,
&p, inlen, tt->tag, aclass, opt, ctx);
q = p;
if (!ret) {
@@ -529,20 +516,10 @@
}
/* We read the field in OK so update length */
len -= p - q;
- if (exp_eoc) {
- /* If NDEF we must have an EOC here */
- if (!asn1_check_eoc(&p, len)) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_MISSING_EOC);
- goto err;
- }
- } else {
- /*
- * Otherwise we must hit the EXPLICIT tag end or its an error
- */
- if (len) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_EXPLICIT_LENGTH_MISMATCH);
- goto err;
- }
+ /* Check for trailing data. */
+ if (len) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_EXPLICIT_LENGTH_MISMATCH);
+ goto err;
}
} else
return asn1_template_noexp_d2i(val, in, inlen, tt, opt, ctx, depth);
@@ -573,7 +550,6 @@
if (flags & ASN1_TFLG_SK_MASK) {
/* SET OF, SEQUENCE OF */
int sktag, skaclass;
- char sk_eoc;
/* First work out expected inner tag value */
if (flags & ASN1_TFLG_IMPTAG) {
sktag = tt->tag;
@@ -586,7 +562,7 @@
sktag = V_ASN1_SEQUENCE;
}
/* Get the tag */
- ret = asn1_check_tlen(&len, NULL, NULL, &sk_eoc, NULL,
+ ret = asn1_check_tlen(&len, NULL, NULL, NULL,
&p, len, sktag, skaclass, opt, ctx);
if (!ret) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR);
@@ -616,15 +592,12 @@
while (len > 0) {
ASN1_VALUE *skfield;
const unsigned char *q = p;
- /* See if EOC found */
+ /* TODO(https://crbug.com/boringssl/455): Although we've removed
+ * indefinite-length support, this check is not quite a no-op.
+ * Reject [UNIVERSAL 0] in the tag parsers themselves. */
if (asn1_check_eoc(&p, len)) {
- if (!sk_eoc) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_UNEXPECTED_EOC);
- goto err;
- }
- len -= p - q;
- sk_eoc = 0;
- break;
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_UNEXPECTED_EOC);
+ goto err;
}
skfield = NULL;
if (!asn1_item_ex_d2i(&skfield, &p, len, ASN1_ITEM_ptr(tt->item),
@@ -639,10 +612,6 @@
goto err;
}
}
- if (sk_eoc) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_MISSING_EOC);
- goto err;
- }
} else if (flags & ASN1_TFLG_IMPTAG) {
/* IMPLICIT tagging */
ret = asn1_item_ex_d2i(val, &p, len, ASN1_ITEM_ptr(tt->item), tt->tag,
@@ -679,7 +648,7 @@
{
int ret = 0, utype;
long plen;
- char cst, inf;
+ char cst;
const unsigned char *p;
const unsigned char *cont = NULL;
long len;
@@ -706,7 +675,7 @@
return 0;
}
p = *in;
- ret = asn1_check_tlen(NULL, &utype, &oclass, NULL, NULL,
+ ret = asn1_check_tlen(NULL, &utype, &oclass, NULL,
&p, inlen, -1, 0, 0, ctx);
if (!ret) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR);
@@ -721,7 +690,7 @@
}
p = *in;
/* Check header */
- ret = asn1_check_tlen(&plen, NULL, NULL, &inf, &cst,
+ ret = asn1_check_tlen(&plen, NULL, NULL, &cst,
&p, inlen, tag, aclass, opt, ctx);
if (!ret) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR);
@@ -746,15 +715,8 @@
}
cont = *in;
- /* If indefinite length constructed find the real end */
- if (inf) {
- if (!asn1_find_end(&p, plen, inf))
- goto err;
- len = p - cont;
- } else {
- len = p - cont + plen;
- p += plen;
- }
+ len = p - cont + plen;
+ p += plen;
} else if (cst) {
/* This parser historically supported BER constructed strings. We no
* longer do and will gradually tighten this parser into a DER
@@ -906,59 +868,6 @@
return ret;
}
-/*
- * This function finds the end of an ASN1 structure when passed its maximum
- * length, whether it is indefinite length and a pointer to the content. This
- * is more efficient than calling asn1_collect because it does not recurse on
- * each indefinite length header.
- */
-
-static int asn1_find_end(const unsigned char **in, long len, char inf)
-{
- int expected_eoc;
- long plen;
- const unsigned char *p = *in, *q;
- /* If not indefinite length constructed just add length */
- if (inf == 0) {
- *in += len;
- return 1;
- }
- expected_eoc = 1;
- /*
- * Indefinite length constructed form. Find the end when enough EOCs are
- * found. If more indefinite length constructed headers are encountered
- * increment the expected eoc count otherwise just skip to the end of the
- * data.
- */
- while (len > 0) {
- if (asn1_check_eoc(&p, len)) {
- expected_eoc--;
- if (expected_eoc == 0)
- break;
- len -= 2;
- continue;
- }
- q = p;
- /* Just read in a header: only care about the length */
- if (!asn1_check_tlen(&plen, NULL, NULL, &inf, NULL, &p, len,
- -1, 0, 0, NULL)) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR);
- return 0;
- }
- if (inf)
- expected_eoc++;
- else
- p += plen;
- len -= p - q;
- }
- if (expected_eoc) {
- OPENSSL_PUT_ERROR(ASN1, ASN1_R_MISSING_EOC);
- return 0;
- }
- *in = p;
- return 1;
-}
-
/* Check for ASN1 EOC and swallow it if found */
static int asn1_check_eoc(const unsigned char **in, long len)
@@ -975,15 +884,12 @@
}
/*
- * Check an ASN1 tag and length: a bit like ASN1_get_object but it sets the
- * length for indefinite length constructed form, we don't know the exact
- * length but we can set an upper bound to the amount of data available minus
- * the header length just read.
+ * Check an ASN1 tag and length: a bit like ASN1_get_object but it handles
+ * the ASN1_TLC cache and checks the expected tag.
*/
static int asn1_check_tlen(long *olen, int *otag, unsigned char *oclass,
- char *inf, char *cst,
- const unsigned char **in, long len,
+ char *cst, const unsigned char **in, long len,
int exptag, int expclass, char opt, ASN1_TLC *ctx)
{
int i;
@@ -1009,10 +915,13 @@
ctx->hdrlen = p - q;
ctx->valid = 1;
/*
- * If definite length, and no error, length + header can't exceed
- * total amount of data available.
+ * If no error, length + header can't exceed total amount of data
+ * available.
+ *
+ * TODO(davidben): Is this check necessary? |ASN1_get_object|
+ * should already guarantee this.
*/
- if (!(i & 0x81) && ((plen + ctx->hdrlen) > len)) {
+ if (!(i & 0x80) && ((plen + ctx->hdrlen) > len)) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_TOO_LONG);
asn1_tlc_clear(ctx);
return 0;
@@ -1043,12 +952,6 @@
asn1_tlc_clear(ctx);
}
- if (i & 1)
- plen = len - (p - q);
-
- if (inf)
- *inf = i & 1;
-
if (cst)
*cst = i & V_ASN1_CONSTRUCTED;
diff --git a/crypto/x509/asn1_gen.c b/crypto/x509/asn1_gen.c
index f1a20e0..cd8185b 100644
--- a/crypto/x509/asn1_gen.c
+++ b/crypto/x509/asn1_gen.c
@@ -215,13 +215,7 @@
* For IMPLICIT tagging the length should match the original length
* and constructed flag should be consistent.
*/
- if (r & 0x1) {
- /* Indefinite length constructed */
- hdr_constructed = 2;
- hdr_len = 0;
- } else
- /* Just retain constructed flag */
- hdr_constructed = r & V_ASN1_CONSTRUCTED;
+ hdr_constructed = r & V_ASN1_CONSTRUCTED;
/*
* Work out new length with IMPLICIT tag: ignore constructed because
* it will mess up if indefinite length
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 477e5a1..46b7b3f 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -3493,8 +3493,24 @@
-----END CERTIFICATE-----
)";
+// kIndefiniteLength is an X.509 certificate where the outermost SEQUENCE uses
+// BER indefinite-length encoding.
+static const char kIndefiniteLength[] = R"(
+-----BEGIN CERTIFICATE-----
+MIAwgcagAwIBAgICBNIwCgYIKoZIzj0EAwIwDzENMAsGA1UEAxMEVGVzdDAgFw0w
+MDAxMDEwMDAwMDBaGA8yMTAwMDEwMTAwMDAwMFowDzENMAsGA1UEAxMEVGVzdDBZ
+MBMGByqGSM49AgEGCCqGSM49AwEHA0IABOYraeK/ZZ+Xvi8eDZSKTNWXa7epHg1G
++92pqR6d3LpaAefWl6gKGPnDxKMeVuJ8g0jbFhoc9R1+8ZQtS89yIsGjEDAOMAwG
+A1UdEwQFMAMBAf8wCgYIKoZIzj0EAwIDSQAwRgIhAKnSIhfmzfQpeOKFHiAqcml3
+ex6oaVVGoJWCsPQoZjVAAiEAqTHS9HzZBTQ20cMPXUpf8u5AXZP7adeh4qnksoBs
+xWIAAA==
+-----END CERTIFICATE-----
+)";
+
TEST(X509Test, BER) {
// Constructed strings are forbidden in DER.
EXPECT_FALSE(CertFromPEM(kConstructedBitString));
EXPECT_FALSE(CertFromPEM(kConstructedOctetString));
+ // Indefinite lengths are forbidden in DER.
+ EXPECT_FALSE(CertFromPEM(kIndefiniteLength));
}
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 0cf0604..a186701 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -1720,25 +1720,20 @@
// Low-level encoding functions.
-// ASN1_get_object parses a BER element from up to |max_len| bytes at |*inp|.
+// ASN1_get_object parses a BER element from up to |max_len| bytes at |*inp|. It
+// returns |V_ASN1_CONSTRUCTED| if it successfully parsed a constructed element,
+// zero if it successfully parsed a primitive element, and 0x80 on error. On
+// success, it additionally advances |*inp| to the element body, sets
+// |*out_length|, |*out_tag|, and |*out_class| to the element's length, tag
+// number, and tag class, respectively,
//
-// If the return value is 0x80, the function failed to parse an element.
-// The contents of |*inp|, |*out_length|, |*out_tag|, and |*out_class| are
-// undefined.
+// Unlike OpenSSL, this function does not support indefinite-length elements.
//
-// Otherwise, the function successfully parsed a element. The return value has
-// bit 0x01 set if the length is indefinite, and |V_ASN1_CONSTRUCTED| set if the
-// element was constructed. The function additionally advances |*inp| to the
-// element body and sets |*out_length|, |*out_tag|, and |*out_class| to the
-// element's length, tag number, and tag class, respectively. If the length is
-// indefinite, |*out_length| will be zero and the caller is responsible for
-// finding the end-of-contents.
+// This function is difficult to use correctly. Use |CBS_get_asn1| and related
+// functions from bytestring.h.
//
-// This function is very difficult to use correctly. Use |CBS_get_asn1| and
-// related functions from bytestring.h.
-//
-// TODO(https://crbug.com/boringssl/354): Remove support for indefinite lengths
-// and non-minimal lengths.
+// TODO(https://crbug.com/boringssl/354): Remove support for non-minimal
+// lengths.
OPENSSL_EXPORT int ASN1_get_object(const unsigned char **inp, long *out_length,
int *out_tag, int *out_class, long max_len);
