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>

.clang-format

@@ -35,6 +35,19 @@ StatementMacros:
   - "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"

crypto/asn1/asn1_test.cc

@@ -184,3 +184,40 @@ TEST(ASN1Test, SerializeBoolean) {
   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)));
+}

crypto/asn1/tasn_dec.c

@@ -223,6 +223,15 @@ static int asn1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in,
         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 @@ static int asn1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in,
         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;
 

crypto/asn1/tasn_enc.c

@@ -145,9 +145,25 @@ int ASN1_item_ex_i2d(ASN1_VALUE **pval, unsigned char **out,
         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);

crypto/err/asn1.errordata

@@ -2,6 +2,7 @@ ASN1,100,ASN1_LENGTH_MISMATCH
 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

include/openssl/asn1.h

@@ -1012,5 +1012,6 @@ BSSL_NAMESPACE_END
 #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