Reinstate support for constructed strings in crypto/asn1

samuel40791765 · samuel40791765 · commit 369254aa16bc · 2025-04-03T01:37:16.000Z
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
@@ -2404,10 +2404,10 @@ TEST(ASN1Test, ZeroTag) {
   ExpectParse(d2i_X509_ALGOR,
               {0x30, 0x10, 0x06, 0x0c, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04,
                0x01, 0x84, 0xb7, 0x09, 0x01, 0x20, 0x00},
-              false);
+              true);
 
-  ExpectParse(d2i_ASN1_TYPE, {0x20, 0x00}, false);
-  ExpectParse(d2i_ASN1_TYPE, {0x20, 0x00}, false);
+  ExpectParse(d2i_ASN1_TYPE, {0x20, 0x00}, true);
+  ExpectParse(d2i_ASN1_TYPE, {0x20, 0x00}, true);
 }
 
 TEST(ASN1Test, IndefiniteLength) {
@@ -2418,19 +2418,21 @@ TEST(ASN1Test, IndefiniteLength) {
               {0x31, 0x80, 0x02, 0x01, 0x01, 0x02, 0x01, 0x02, 0x00, 0x00},
               true);
 
-  // The ones below use constructed form and should fail for now. This is
-  // indicated with (0x20 | 0x??) in the first byte.
-  ExpectParse(d2i_ASN1_INTEGER,
-              {0x22, 0x80, 0x02, 0x01, 0x12, 0x02, 0x01, 0x34, 0x00, 0x00},
-              false);
-  ExpectParse(
-      d2i_ASN1_OCTET_STRING,
-      {0x24, 0x80, 0x04, 0x02, 0x12, 0x34, 0x04, 0x02, 0x56, 0x78, 0x00, 0x00},
-      false);
+  // The ones below use constructed form. This is indicated with (0x20 | 0x??)
+  // in the first byte.
   ExpectParse(
       d2i_ASN1_BIT_STRING,
       {0x23, 0x80, 0x03, 0x02, 0x00, 0xFF, 0x03, 0x02, 0x00, 0xAA, 0x00, 0x00},
-      false);
+      true);
+  ExpectParse(
+      d2i_ASN1_OCTET_STRING,
+      {0x24, 0x80, 0x04, 0x02, 0x12, 0x34, 0x04, 0x02, 0x56, 0x78, 0x00, 0x00},
+      true);
+  // |ASN1_INTEGER|s only have primitive encoding. They do not have constructed
+  // forms.
+  ExpectParse(d2i_ASN1_INTEGER,
+            {0x22, 0x80, 0x02, 0x01, 0x12, 0x02, 0x01, 0x34, 0x00, 0x00},
+            false);
 }
 
 // Exhaustively test POSIX time conversions for every day across the millenium.
diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c
@@ -75,6 +75,11 @@
 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_collect(BUF_MEM *buf, const unsigned char **in, long len,
+                        char inf, int tag, int aclass, int depth);
+
+static int collect_data(BUF_MEM *buf, const unsigned char **p, long plen);
+
 static int asn1_check_tlen(long *olen, int *otag, unsigned char *oclass,
                            char *inf, char *cst, const unsigned char **in,
                            long len, int exptag, int expclass, char opt);
@@ -668,6 +673,7 @@ static int asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in,
   long plen;
   char cst, inf;
   const unsigned char *p;
+  BUF_MEM buf = {0, NULL, 0 };
   const unsigned char *cont = NULL;
   long len;
   if (!pval) {
@@ -738,11 +744,31 @@ static int asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in,
       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
-    // parser. BER types should use |CBS_asn1_ber_to_der|.
-    OPENSSL_PUT_ERROR(ASN1, ASN1_R_TYPE_NOT_PRIMITIVE);
-    return 0;
+    if (utype == V_ASN1_NULL || utype == V_ASN1_BOOLEAN
+                || utype == V_ASN1_OBJECT || utype == V_ASN1_INTEGER
+                || utype == V_ASN1_ENUMERATED) {
+      // These types only have primitive encodings.
+      OPENSSL_PUT_ERROR(ASN1, ASN1_R_TYPE_NOT_PRIMITIVE);
+      return 0;
+    }
+
+    // Free any returned 'buf' content.
+    // free_cont = 1;
+    // Should really check the internal tags are correct but some things
+    // may get this wrong. The relevant specs say that constructed string
+    // types should be OCTET STRINGs internally irrespective of the type.
+    // So instead just check for UNIVERSAL class and ignore the tag.
+    if (!asn1_collect(&buf, &p, plen, inf, -1, V_ASN1_UNIVERSAL, 0)) {
+      goto err;
+    }
+    len = buf.length;
+    // Append a final null to string.
+    if (!BUF_MEM_grow_clean(&buf, len + 1)) {
+      OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE);
+      goto err;
+    }
+    buf.data[len] = 0;
+    cont = (const unsigned char *)buf.data;
   } else {
     cont = p;
     len = plen;
@@ -757,6 +783,7 @@ static int asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in,
   *in = p;
   ret = 1;
 err:
+  OPENSSL_free(buf.data);
   return ret;
 }
 
@@ -960,6 +987,83 @@ static int asn1_find_end(const unsigned char **in, long len, char inf) {
   return 1;
 }
 
+// This function collects the asn1 data from a constructed string type into
+// a buffer. The values of 'in' and 'len' should refer to the contents of the
+// constructed type and 'inf' should be set if it is indefinite length.
+
+// This determines how many levels of recursion are permitted in ASN1 string
+// types. If it is not limited stack overflows can occur. If set to zero no
+// recursion is allowed at all. Although zero should be adequate examples
+// exist that require a value of 1. So 5 should be more than enough.
+
+#define ASN1_MAX_STRING_NEST 5
+
+static int asn1_collect(BUF_MEM *buf, const unsigned char **in, long len,
+                        char inf, int tag, int aclass, int depth) {
+  const unsigned char *p, *q;
+  long plen;
+  char cst, ininf;
+  p = *in;
+  inf &= 1;
+  // If no buffer and not indefinite length constructed just pass over the
+  // encoded data
+  if (!buf && !inf) {
+    *in += len;
+    return 1;
+  }
+  while (len > 0) {
+    q = p;
+    // Check for EOC.
+    if (asn1_check_eoc(&p, len)) {
+      // EOC is illegal outside indefinite length constructed form
+      if (!inf) {
+        OPENSSL_PUT_ERROR(ASN1, ASN1_R_UNEXPECTED_EOC);
+        return 0;
+      }
+      inf = 0;
+      break;
+    }
+
+    if (!asn1_check_tlen(&plen, NULL, NULL, &ininf, &cst, &p,
+                         len, tag, aclass, 0)) {
+      OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR);
+      return 0;
+                         }
+
+    // If indefinite length constructed, update max length.
+    if (cst) {
+      if (depth >= ASN1_MAX_STRING_NEST) {
+        OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_STRING);
+        return 0;
+      }
+      if (!asn1_collect(buf, &p, plen, ininf, tag, aclass, depth + 1))
+        return 0;
+    } else if (plen && !collect_data(buf, &p, plen))
+      return 0;
+    len -= p - q;
+  }
+  if (inf) {
+    OPENSSL_PUT_ERROR(ASN1, ASN1_R_MISSING_EOC);
+    return 0;
+  }
+  *in = p;
+  return 1;
+}
+
+static int collect_data(BUF_MEM *buf, const unsigned char **p, long plen) {
+  int len;
+  if (buf) {
+    len = buf->length;
+    if (!BUF_MEM_grow_clean(buf, len + plen)) {
+      OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE);
+      return 0;
+    }
+    OPENSSL_memcpy(buf->data + len, *p, plen);
+  }
+  *p += plen;
+  return 1;
+}
+
 // Check for ASN1 EOC and swallow it if found.
 static int asn1_check_eoc(const unsigned char **in, long len) {
   const unsigned char *p;
diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
@@ -655,6 +655,38 @@ static const uint8_t kWrappedIndefBER[] = {0x30, 0x08, 0x30, 0x04, 0x30,
 static const uint8_t kWrappedIndefDER[] = {0x30, 0x06, 0x30, 0x02,
                                            0x30, 0x00, 0x30, 0x00};
 
+// kConstructedStringBER contains a deeply-nested constructed OCTET STRING.
+// The BER conversion collapses this to one level deep, but not completely.
+static const uint8_t kConstructedStringBER[] = {
+  0xa0, 0x10, 0x24, 0x06, 0x04, 0x01, 0x00, 0x04, 0x01,
+  0x01, 0x24, 0x06, 0x04, 0x01, 0x02, 0x04, 0x01, 0x03,
+};
+static const uint8_t kConstructedStringDER[] = {
+  0xa0, 0x08, 0x04, 0x02, 0x00, 0x01, 0x04, 0x02, 0x02, 0x03,
+};
+
+// kWrappedConstructedStringBER contains a constructed OCTET STRING, wrapped
+// and followed by valid DER. This tests that we correctly identify BER nested
+// inside DER.
+//
+//  SEQUENCE {
+//    SEQUENCE {
+//      [OCTET_STRING CONSTRUCTED] {
+//        OCTET_STRING {}
+//      }
+//    }
+//    SEQUENCE {}
+//  }
+static const uint8_t kWrappedConstructedStringBER[] = {
+  0x30, 0x08, 0x30, 0x04, 0x24, 0x02, 0x04, 0x00, 0x30, 0x00};
+static const uint8_t kWrappedConstructedStringDER[] = {
+  0x30, 0x06, 0x30, 0x02, 0x04, 0x00, 0x30, 0x00};
+
+// kConstructedBitString contains a BER constructed BIT STRING. These are not
+// supported and thus are left unchanged.
+static const uint8_t kConstructedBitStringBER[] = {
+  0x23, 0x0a, 0x03, 0x03, 0x00, 0x12, 0x34, 0x03, 0x03, 0x00, 0x56, 0x78};
+
 TEST(CBSTest, BerConvert) {
   static const uint8_t kSimpleBER[] = {0x01, 0x01, 0x00};
 
@@ -686,38 +718,6 @@ TEST(CBSTest, BerConvert) {
       0x6e, 0x10, 0x9b, 0xb8, 0x02, 0x02, 0x07, 0xd0,
   };
 
-  // kConstructedStringBER contains a deeply-nested constructed OCTET STRING.
-  // The BER conversion collapses this to one level deep, but not completely.
-  static const uint8_t kConstructedStringBER[] = {
-      0xa0, 0x10, 0x24, 0x06, 0x04, 0x01, 0x00, 0x04, 0x01,
-      0x01, 0x24, 0x06, 0x04, 0x01, 0x02, 0x04, 0x01, 0x03,
-  };
-  static const uint8_t kConstructedStringDER[] = {
-      0xa0, 0x08, 0x04, 0x02, 0x00, 0x01, 0x04, 0x02, 0x02, 0x03,
-  };
-
-  // kWrappedConstructedStringBER contains a constructed OCTET STRING, wrapped
-  // and followed by valid DER. This tests that we correctly identify BER nested
-  // inside DER.
-  //
-  //  SEQUENCE {
-  //    SEQUENCE {
-  //      [OCTET_STRING CONSTRUCTED] {
-  //        OCTET_STRING {}
-  //      }
-  //    }
-  //    SEQUENCE {}
-  //  }
-  static const uint8_t kWrappedConstructedStringBER[] = {
-      0x30, 0x08, 0x30, 0x04, 0x24, 0x02, 0x04, 0x00, 0x30, 0x00};
-  static const uint8_t kWrappedConstructedStringDER[] = {
-      0x30, 0x06, 0x30, 0x02, 0x04, 0x00, 0x30, 0x00};
-
-  // kConstructedBitString contains a BER constructed BIT STRING. These are not
-  // supported and thus are left unchanged.
-  static const uint8_t kConstructedBitStringBER[] = {
-      0x23, 0x0a, 0x03, 0x03, 0x00, 0x12, 0x34, 0x03, 0x03, 0x00, 0x56, 0x78};
-
   ExpectBerConvert("kSimpleBER", kSimpleBER, kSimpleBER);
   ExpectBerConvert("kNonMinimalLengthBER", kNonMinimalLengthDER,
                    kNonMinimalLengthBER);
@@ -762,12 +762,23 @@ TEST(CBSTest, IndefiniteBerASN1Macros) {
                                    kWrappedIndefBER + sizeof(kWrappedIndefBER)),
               true);
 
-  // TODO: Change expectation of below to true. Below use BER constructed
-  //       strings and will still fail until we revert a70edd4.
+  // Below use BER constructed strings.
   ExpectParse(d2i_ASN1_OCTET_STRING,
               std::vector<uint8_t>(kOctetStringBER,
                                    kOctetStringBER + sizeof(kOctetStringBER)),
-              false);
+              true);
+  ExpectParse(d2i_ASN1_TYPE,
+              std::vector<uint8_t>(kConstructedStringBER,
+                                   kConstructedStringBER + sizeof(kConstructedStringBER)),
+              true);
+  ExpectParse(d2i_ASN1_SEQUENCE_ANY,
+            std::vector<uint8_t>(kWrappedConstructedStringBER,
+                                 kWrappedConstructedStringBER + sizeof(kWrappedConstructedStringBER)),
+            true);
+  ExpectParse(d2i_ASN1_BIT_STRING,
+            std::vector<uint8_t>(kConstructedBitStringBER,
+                                 kConstructedBitStringBER + sizeof(kConstructedBitStringBER)),
+            true);
 }
 
 struct BERTest {
diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc
@@ -2210,6 +2210,32 @@ CSqGSIb3DQEBAQUABIGAWUNdzvU2iiQOtihBwF0h48Nnw/2qX8uRjg6CVTOMcGji
 BxjUMifEbT//KJwljshl4y3yBLqeVYLOd04k6aKSdjgdZnrnUPI6p5tL5PfJkTAE
 L6qflZ9YCU5erE4T5U98hCQBMh4nOYxgaTjnZzhpkKQuEiKq/755cjzTzlI/eok=
 -----END PKCS7-----
+)";
+
+  // The test has an expected output. Check that we output the same contents
+  // as OpenSSL.
+  static const char kPKCS7RubyOpenSSLOutput[] =
+    R"(-----BEGIN PKCS7-----
+MIIDawYJKoZIhvcNAQcDoIIDXDCCA1gCAQAxggEQMIIBDAIBADB1MHAxEDAOBgNV
+BAoMB2V4YW1wbGUxFzAVBgNVBAMMDlRBUk1BQyBST09UIENBMSIwIAYJKoZIhvcN
+AQkBFhNzb21lb25lQGV4YW1wbGUub3JnMQswCQYDVQQGEwJVUzESMBAGA1UEBwwJ
+VG93biBIYWxsAgFmMA0GCSqGSIb3DQEBAQUABIGAbKV17HvGYRtRRBNz1QLpW763
+UedhVj5KXi70o4BJGM04lItAgt6aFC9SruZjpWr1gCYKCaRSAg273DeGTQwsDoZ8
+6CPXzBpptYLz0MteQXYYWUaPZT+xmvx4NgDyk9P9MoT7JifsPrtXuzqCRFXhGdu8
+d/ru+OWxhHLvKH+bYekwggI9BgkqhkiG9w0BBwEwFAYIKoZIhvcNAwcECBNs2U5m
+Msd/gIICGFOnLq/EAc9Nv+HjKR3ZVPSJMq0TImjGf5Mvc3nDgI572Hdo2aku0YXM
+6WjSWkpYtxpg7Cqxfl6hPSefLPUnBqlIoM2qbrE7MSKEVD6+2bW9GqYPFVg4qQLL
+sOxnxJIMfOvLFfd7guL+iLH424XfiUUxaf8EdZE4u2IEl4REvkS1FoEGwyA4BEGM
+SeVPedQCbZ0qY7Pc2tmZE3XfEUhIsyStG0Nb6i6AKcAFYGapbgE6kAB0gwsYcHlW
+MOvsvdAfcTq6jwtHlO1s68qtvkWquTQ9lpX+fzddUUNxEHSqv5eU3oo6fT3Vj5ZF
+IVlaA5ThZMrI5PgRPuwJM4GL8/VLwY5mbDLFqn/irGeEvP99J3S87ornLLunjpxS
+y1/AymcVep2H32Tj82WS/IRQXBOzz4EnQRJGszKxAV6tY+Zje3sWyTTgObhlsiTQ
+TDgnvtSW8RvVHqKrwgkxxEsRHg7u8UdzZ0jg+O5+3F8B6/NWMyts0OaFqT9wvI8y
+O7VIy3dUtGdz7Hde6Ggp/iTn1LbgdJ3N8Hzxf1j6NMWUKHVsadvwpRJbUeqq9c3+
+QuxsJi8wWemxxQCE+tPyc1dP+ej5/M7bERbSOHMGgX03758IvP7A/fy2DjGPv2+l
+AwlEke0Uze1367QKgxM0nc3SZDlptY7zPIJC5saWXb8Rt2bw2JxEBOTavrp+ZwJ8
+tcH961onq8Tme2ICaCzk
+-----END PKCS7-----
 )";
 
   const bssl::UniquePtr<BIO> bio(
@@ -2225,4 +2251,16 @@ L6qflZ9YCU5erE4T5U98hCQBMh4nOYxgaTjnZzhpkKQuEiKq/755cjzTzlI/eok=
   bssl::UniquePtr<BIO> out(BIO_new(BIO_s_mem()));
   EXPECT_TRUE(PKCS7_verify(pkcs7.get(), nullptr, store.get(), nullptr,
                            out.get(), /*flags*/ PKCS7_NOVERIFY));
+
+  BUF_MEM *buf;
+  BIO_get_mem_ptr(out.get(), &buf);
+  bssl::UniquePtr<BIO> out2(BIO_new_mem_buf(buf->data, buf->length));
+  bssl::UniquePtr<PKCS7> new_pk7(d2i_PKCS7_bio(out2.get(), nullptr));
+  ASSERT_TRUE(new_pk7);
+
+  bssl::UniquePtr<BIO> out3(BIO_new(BIO_s_mem()));
+  ASSERT_TRUE(PEM_write_bio_PKCS7(out3.get(), new_pk7.get()));
+
+  BIO_get_mem_ptr(out3.get(), &buf);
+  EXPECT_EQ(Bytes(buf->data, buf->length), Bytes(kPKCS7RubyOpenSSLOutput));
 }
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
@@ -5224,9 +5224,11 @@ soBsxWI=
 )";
 
 TEST(X509Test, BER) {
-  // Constructed strings are forbidden in DER.
-  EXPECT_FALSE(CertFromPEM(kConstructedBitString));
-  EXPECT_FALSE(CertFromPEM(kConstructedOctetString));
+  // Constructed strings are forbidden in DER, but allowed in BER. AWS-LC has
+  // reinstated support for implicit BER constructed strings  in the ASN1 macros
+  // to align with OpenSSL behavior.
+  EXPECT_TRUE(CertFromPEM(kConstructedBitString));
+  EXPECT_TRUE(CertFromPEM(kConstructedOctetString));
   // Indefinite lengths are forbidden in DER, but allowed in BER. AWS-LC has
   // reinstated indefinite BER support in the ASN1 macros to align with OpenSSL
   // behavior.
@@ -7522,7 +7524,6 @@ TEST(X509Test, NameAttributeValues) {
       {CBS_ASN1_UNIVERSALSTRING, "not utf-32"},
       {CBS_ASN1_UTCTIME, "not utctime"},
       {CBS_ASN1_GENERALIZEDTIME, "not generalizedtime"},
-      {CBS_ASN1_UTF8STRING | CBS_ASN1_CONSTRUCTED, ""},
       {CBS_ASN1_SEQUENCE & ~CBS_ASN1_CONSTRUCTED, ""},
 
       // TODO(crbug.com/boringssl/412): The following inputs should parse, but
