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