Limit ASN.1 constructed types recursive definition depth Constructed types with a recursive definition could eventually exceed the stack given malicious input with excessive recursion. Therefore we limit the stack depth. CVE-2018-0739 Credit to OSSFuzz for finding this issue. (Imported from upstream's 9310d45087ae546e27e61ddf8f6367f29848220d.) BoringSSL does not contain any such structures, but import this anyway with a test. Change-Id: I0e84578ea795134f25dae2ac8b565f3c26ef3204 Reviewed-on: https://boringssl-review.googlesource.com/26844 Commit-Queue: David Benjamin <davidben@google.com> Commit-Queue: Adam Langley <agl@google.com> Reviewed-by: Adam Langley <agl@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 7c114dd..1cca36c 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc
@@ -12,13 +12,18 @@ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ +#include <limits.h> #include <stdio.h> +#include <vector> + #include <gtest/gtest.h> -#include <limits.h> #include <openssl/asn1.h> +#include <openssl/asn1t.h> +#include <openssl/bytestring.h> #include <openssl/err.h> +#include <openssl/mem.h> #include "../test/test_util.h" @@ -88,3 +93,58 @@ } } } + +typedef struct asn1_linked_list_st { + struct asn1_linked_list_st *next; +} ASN1_LINKED_LIST; + +DECLARE_ASN1_ITEM(ASN1_LINKED_LIST) +DECLARE_ASN1_FUNCTIONS(ASN1_LINKED_LIST) + +ASN1_SEQUENCE(ASN1_LINKED_LIST) = { + ASN1_OPT(ASN1_LINKED_LIST, next, ASN1_LINKED_LIST), +} ASN1_SEQUENCE_END(ASN1_LINKED_LIST) + +IMPLEMENT_ASN1_FUNCTIONS(ASN1_LINKED_LIST) + +static bool MakeLinkedList(bssl::UniquePtr<uint8_t> *out, size_t *out_len, + size_t count) { + bssl::ScopedCBB cbb; + std::vector<CBB> cbbs(count); + if (!CBB_init(cbb.get(), 2 * count) || + !CBB_add_asn1(cbb.get(), &cbbs[0], CBS_ASN1_SEQUENCE)) { + return false; + } + for (size_t i = 1; i < count; i++) { + if (!CBB_add_asn1(&cbbs[i - 1], &cbbs[i], CBS_ASN1_SEQUENCE)) { + return false; + } + } + uint8_t *ptr; + if (!CBB_finish(cbb.get(), &ptr, out_len)) { + return false; + } + out->reset(ptr); + return true; +} + +TEST(ASN1Test, Recursive) { + bssl::UniquePtr<uint8_t> data; + size_t len; + + // Sanity-check that MakeLinkedList can be parsed. + ASSERT_TRUE(MakeLinkedList(&data, &len, 5)); + const uint8_t *ptr = data.get(); + ASN1_LINKED_LIST *list = d2i_ASN1_LINKED_LIST(nullptr, &ptr, len); + EXPECT_TRUE(list); + ASN1_LINKED_LIST_free(list); + + // Excessively deep structures are rejected. + ASSERT_TRUE(MakeLinkedList(&data, &len, 100)); + ptr = data.get(); + list = d2i_ASN1_LINKED_LIST(nullptr, &ptr, len); + EXPECT_FALSE(list); + // Note checking the error queue here does not work. The error "stack trace" + // is too deep, so the |ASN1_R_NESTED_TOO_DEEP| entry drops off the queue. + ASN1_LINKED_LIST_free(list); +}
diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c index 2f5f132..32aba0b 100644 --- a/crypto/asn1/tasn_dec.c +++ b/crypto/asn1/tasn_dec.c
@@ -66,6 +66,14 @@ #include "../internal.h" +/* + * Constructed types with a recursive definition (such as can be found in PKCS7) + * could eventually exceed the stack given malicious input with excessive + * recursion. Therefore we limit the stack depth. This is the maximum number of + * recursive invocations of asn1_item_embed_d2i(). + */ +#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); @@ -82,11 +90,11 @@ static int asn1_template_ex_d2i(ASN1_VALUE **pval, const unsigned char **in, long len, const ASN1_TEMPLATE *tt, char opt, - ASN1_TLC *ctx); + ASN1_TLC *ctx, int depth); static int asn1_template_noexp_d2i(ASN1_VALUE **val, const unsigned char **in, long len, const ASN1_TEMPLATE *tt, char opt, - ASN1_TLC *ctx); + ASN1_TLC *ctx, int depth); static int asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in, long len, const ASN1_ITEM *it, @@ -153,9 +161,9 @@ * tag mismatch return -1 to handle OPTIONAL */ -int ASN1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in, long len, - const ASN1_ITEM *it, - int tag, int aclass, char opt, ASN1_TLC *ctx) +static int asn1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in, + long len, const ASN1_ITEM *it, int tag, int aclass, + char opt, ASN1_TLC *ctx, int depth) { const ASN1_TEMPLATE *tt, *errtt = NULL; const ASN1_COMPAT_FUNCS *cf; @@ -188,6 +196,11 @@ len = INT_MAX/2; } + if (++depth > ASN1_MAX_CONSTRUCTED_NEST) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_TOO_DEEP); + goto err; + } + switch (it->itype) { case ASN1_ITYPE_PRIMITIVE: if (it->templates) { @@ -203,7 +216,7 @@ goto err; } return asn1_template_ex_d2i(pval, in, len, - it->templates, opt, ctx); + it->templates, opt, ctx, depth); } return asn1_d2i_ex_primitive(pval, in, len, it, tag, aclass, opt, ctx); @@ -326,7 +339,7 @@ /* * We mark field as OPTIONAL so its absence can be recognised. */ - ret = asn1_template_ex_d2i(pchptr, &p, len, tt, 1, ctx); + ret = asn1_template_ex_d2i(pchptr, &p, len, tt, 1, ctx, depth); /* If field not present, try the next one */ if (ret == -1) continue; @@ -444,7 +457,8 @@ * attempt to read in field, allowing each to be OPTIONAL */ - ret = asn1_template_ex_d2i(pseqval, &p, len, seqtt, isopt, ctx); + ret = asn1_template_ex_d2i(pseqval, &p, len, seqtt, isopt, ctx, + depth); if (!ret) { errtt = seqtt; goto err; @@ -514,6 +528,13 @@ return 0; } +int ASN1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in, long len, + const ASN1_ITEM *it, + int tag, int aclass, char opt, ASN1_TLC *ctx) +{ + return asn1_item_ex_d2i(pval, in, len, it, tag, aclass, opt, ctx, 0); +} + /* * Templates are handled with two separate functions. One handles any * EXPLICIT tag and the other handles the rest. @@ -522,7 +543,7 @@ static int asn1_template_ex_d2i(ASN1_VALUE **val, const unsigned char **in, long inlen, const ASN1_TEMPLATE *tt, char opt, - ASN1_TLC *ctx) + ASN1_TLC *ctx, int depth) { int flags, aclass; int ret; @@ -556,7 +577,7 @@ return 0; } /* We've found the field so it can't be OPTIONAL now */ - ret = asn1_template_noexp_d2i(val, &p, len, tt, 0, ctx); + ret = asn1_template_noexp_d2i(val, &p, len, tt, 0, ctx, depth); if (!ret) { OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR); return 0; @@ -579,7 +600,7 @@ } } } else - return asn1_template_noexp_d2i(val, in, inlen, tt, opt, ctx); + return asn1_template_noexp_d2i(val, in, inlen, tt, opt, ctx, depth); *in = p; return 1; @@ -592,7 +613,7 @@ static int asn1_template_noexp_d2i(ASN1_VALUE **val, const unsigned char **in, long len, const ASN1_TEMPLATE *tt, char opt, - ASN1_TLC *ctx) + ASN1_TLC *ctx, int depth) { int flags, aclass; int ret; @@ -661,8 +682,8 @@ break; } skfield = NULL; - if (!ASN1_item_ex_d2i(&skfield, &p, len, - ASN1_ITEM_ptr(tt->item), -1, 0, 0, ctx)) { + if (!asn1_item_ex_d2i(&skfield, &p, len, ASN1_ITEM_ptr(tt->item), + -1, 0, 0, ctx, depth)) { OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR); goto err; } @@ -679,9 +700,8 @@ } } else if (flags & ASN1_TFLG_IMPTAG) { /* IMPLICIT tagging */ - ret = ASN1_item_ex_d2i(val, &p, len, - ASN1_ITEM_ptr(tt->item), tt->tag, aclass, opt, - ctx); + ret = asn1_item_ex_d2i(val, &p, len, ASN1_ITEM_ptr(tt->item), tt->tag, + aclass, opt, ctx, depth); if (!ret) { OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR); goto err; @@ -689,8 +709,9 @@ return -1; } else { /* Nothing special */ - ret = ASN1_item_ex_d2i(val, &p, len, ASN1_ITEM_ptr(tt->item), - -1, tt->flags & ASN1_TFLG_COMBINE, opt, ctx); + ret = asn1_item_ex_d2i(val, &p, len, ASN1_ITEM_ptr(tt->item), + -1, tt->flags & ASN1_TFLG_COMBINE, opt, ctx, + depth); if (!ret) { OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR); goto err;
diff --git a/crypto/err/asn1.errordata b/crypto/err/asn1.errordata index c304b2c..56cbbe52 100644 --- a/crypto/err/asn1.errordata +++ b/crypto/err/asn1.errordata
@@ -58,6 +58,7 @@ ASN1,157,MSTRING_WRONG_TAG ASN1,158,NESTED_ASN1_ERROR ASN1,159,NESTED_ASN1_STRING +ASN1,192,NESTED_TOO_DEEP ASN1,160,NON_HEX_CHARACTERS ASN1,161,NOT_ASCII_FORMAT ASN1,162,NOT_ENOUGH_DATA
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h index c7ead03..f2e92a7 100644 --- a/include/openssl/asn1.h +++ b/include/openssl/asn1.h
@@ -976,5 +976,6 @@ #define ASN1_R_WRONG_PUBLIC_KEY_TYPE 189 #define ASN1_R_WRONG_TAG 190 #define ASN1_R_WRONG_TYPE 191 +#define ASN1_R_NESTED_TOO_DEEP 192 #endif