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..6de1483 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
