Reject bad ASN.1 templates with implicitly-tagged CHOICEs. This imports 1ecc76f6746cefd502c7e9000bdfa4e5d7911386 and 41d62636fd996c031c0c7cef746476278583dc9e from upstream. These would have rejected the mistake in OpenSSL's EDIPartyName sturcture. Change-Id: I4eb218f9372bea0f7ff302321b9dc1992ef0c13a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44424 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com>
diff --git a/.clang-format b/.clang-format index eee2a9c..6de1483e 100644 --- a/.clang-format +++ b/.clang-format
@@ -35,6 +35,19 @@ - "DECLARE_PEM_write_const" - "DECLARE_PEM_write_fp" - "DECLARE_PEM_write_fp_const" + - "IMPLEMENT_ASN1_ALLOC_FUNCTIONS" + - "IMPLEMENT_ASN1_ALLOC_FUNCTIONS_fname" + - "IMPLEMENT_ASN1_ALLOC_FUNCTIONS_pfname" + - "IMPLEMENT_ASN1_DUP_FUNCTION" + - "IMPLEMENT_ASN1_ENCODE_FUNCTIONS_const_fname" + - "IMPLEMENT_ASN1_ENCODE_FUNCTIONS_fname" + - "IMPLEMENT_ASN1_FUNCTIONS" + - "IMPLEMENT_ASN1_FUNCTIONS_const" + - "IMPLEMENT_ASN1_FUNCTIONS_const_fname" + - "IMPLEMENT_ASN1_FUNCTIONS_ENCODE_name" + - "IMPLEMENT_ASN1_FUNCTIONS_fname" + - "IMPLEMENT_ASN1_FUNCTIONS_name" + - "IMPLEMENT_STATIC_ASN1_ALLOC_FUNCTIONS" - "IMPLEMENT_PEM_read" - "IMPLEMENT_PEM_read_bio" - "IMPLEMENT_PEM_read_fp"
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 54d6ee8..68a062b 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc
@@ -184,3 +184,40 @@ static const uint8_t kFalse[] = {0x01, 0x01, 0x00}; TestSerialize(0x00, i2d_ASN1_BOOLEAN, kFalse); } + +struct IMPLICIT_CHOICE { + ASN1_STRING *string; +}; + +// clang-format off +DECLARE_ASN1_FUNCTIONS(IMPLICIT_CHOICE) + +ASN1_SEQUENCE(IMPLICIT_CHOICE) = { + ASN1_IMP(IMPLICIT_CHOICE, string, DIRECTORYSTRING, 0) +} ASN1_SEQUENCE_END(IMPLICIT_CHOICE) + +IMPLEMENT_ASN1_FUNCTIONS(IMPLICIT_CHOICE) +// clang-format on + +// Test that the ASN.1 templates reject types with implicitly-tagged CHOICE +// types. +TEST(ASN1Test, ImplicitChoice) { + // Serializing a type with an implicitly tagged CHOICE should fail. + std::unique_ptr<IMPLICIT_CHOICE, decltype(&IMPLICIT_CHOICE_free)> obj( + IMPLICIT_CHOICE_new(), IMPLICIT_CHOICE_free); + EXPECT_EQ(-1, i2d_IMPLICIT_CHOICE(obj.get(), nullptr)); + + // An implicitly-tagged CHOICE is an error. Depending on the implementation, + // it may be misinterpreted as without the tag, or as clobbering the CHOICE + // tag. Test both inputs and ensure they fail. + + // SEQUENCE { UTF8String {} } + static const uint8_t kInput1[] = {0x30, 0x02, 0x0c, 0x00}; + const uint8_t *ptr = kInput1; + EXPECT_EQ(nullptr, d2i_IMPLICIT_CHOICE(nullptr, &ptr, sizeof(kInput1))); + + // SEQUENCE { [0 PRIMITIVE] {} } + static const uint8_t kInput2[] = {0x30, 0x02, 0x80, 0x00}; + ptr = kInput2; + EXPECT_EQ(nullptr, d2i_IMPLICIT_CHOICE(nullptr, &ptr, sizeof(kInput2))); +}
diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c index 531bc66..99a9714 100644 --- a/crypto/asn1/tasn_dec.c +++ b/crypto/asn1/tasn_dec.c
@@ -223,6 +223,15 @@ break; case ASN1_ITYPE_MSTRING: + /* + * It never makes sense for multi-strings to have implicit tagging, so + * if tag != -1, then this looks like an error in the template. + */ + if (tag != -1) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_TEMPLATE); + goto err; + } + p = *in; /* Just read in tag and class */ ret = asn1_check_tlen(NULL, &otag, &oclass, NULL, NULL, @@ -256,6 +265,15 @@ return ef->asn1_ex_d2i(pval, in, len, it, tag, aclass, opt, ctx); case ASN1_ITYPE_CHOICE: + /* + * It never makes sense for CHOICE types to have implicit tagging, so if + * tag != -1, then this looks like an error in the template. + */ + if (tag != -1) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_TEMPLATE); + goto err; + } + if (asn1_cb && !asn1_cb(ASN1_OP_D2I_PRE, pval, it, NULL)) goto auxerr;
diff --git a/crypto/asn1/tasn_enc.c b/crypto/asn1/tasn_enc.c index d0aa0c5..1323439 100644 --- a/crypto/asn1/tasn_enc.c +++ b/crypto/asn1/tasn_enc.c
@@ -145,9 +145,25 @@ break; case ASN1_ITYPE_MSTRING: + /* + * It never makes sense for multi-strings to have implicit tagging, so + * if tag != -1, then this looks like an error in the template. + */ + if (tag != -1) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_TEMPLATE); + return -1; + } return asn1_i2d_ex_primitive(pval, out, it, -1, aclass); case ASN1_ITYPE_CHOICE: + /* + * It never makes sense for CHOICE types to have implicit tagging, so if + * tag != -1, then this looks like an error in the template. + */ + if (tag != -1) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_TEMPLATE); + return -1; + } if (asn1_cb && !asn1_cb(ASN1_OP_I2D_PRE, pval, it, NULL)) return 0; i = asn1_get_choice_selector(pval, it);
diff --git a/crypto/err/asn1.errordata b/crypto/err/asn1.errordata index 271561b..5674bda 100644 --- a/crypto/err/asn1.errordata +++ b/crypto/err/asn1.errordata
@@ -2,6 +2,7 @@ ASN1,101,AUX_ERROR ASN1,102,BAD_GET_ASN1_OBJECT_CALL ASN1,103,BAD_OBJECT_HEADER +ASN1,193,BAD_TEMPLATE ASN1,104,BMPSTRING_IS_WRONG_LENGTH ASN1,105,BN_LIB ASN1,106,BOOLEAN_IS_WRONG_LENGTH
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h index 70a23c5..9269553 100644 --- a/include/openssl/asn1.h +++ b/include/openssl/asn1.h
@@ -1012,5 +1012,6 @@ #define ASN1_R_WRONG_TAG 190 #define ASN1_R_WRONG_TYPE 191 #define ASN1_R_NESTED_TOO_DEEP 192 +#define ASN1_R_BAD_TEMPLATE 193 #endif