From 2a722260329a62e0682410060f84cf144960afe4 Mon Sep 17 00:00:00 2001
From: Will Childs-Klein <childw@amazon.com>
Date: Mon, 2 Dec 2024 09:05:35 -0600
Subject: [PATCH] Allow constructed strings in BER parsing (#2015)

This change relaxes a prior restriction forbidding constructed strings
when parsing ASN.1 BER. We modify relevant tests accordingly and fix two
small errors in PEM fixtures.

`kConstructedBitString` contained a bit string with invalid padding. We
fixed this by zeroing out the final padding byte of each constructed bit
string component. `kConstructedOctetString`' constructed string
components were typed as integers (type `0x02`) instead of octet strings
(type `0x04`). For that, we simply change the types to `0x04`.
---
 crypto/asn1/tasn_dec.c   |  6 ------
 crypto/x509/x509_test.cc | 15 +++++++--------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c
index 97214b5986..329422cbd8 100644
--- a/crypto/asn1/tasn_dec.c
+++ b/crypto/asn1/tasn_dec.c
@@ -681,12 +681,6 @@ static int asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in,
     cont = *in;
     len = p - cont + plen;
     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;
   } else {
     cont = p;
     len = plen;
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 0b5501f3c7..99e8a155f4 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -5178,8 +5178,8 @@ DTAwMDEwMTAwMDAwMFoYDzIxMDAwMTAxMDAwMDAwWjAPMQ0wCwYDVQQDEwRUZXN0
 MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE5itp4r9ln5e+Lx4NlIpM1Zdrt6ke
 DUb73ampHp3culoB59aXqAoY+cPEox5W4nyDSNsWGhz1HX7xlC1Lz3IiwaMQMA4w
 DAYDVR0TBAUwAwEB/zAKBggqhkjOPQQDAiNOAyQAMEYCIQCp0iIX5s30KXjihR4g
-KnJpd3seqGlVRqCVgrD0KGYDJgA1QAIhAKkx0vR82QU0NtHDD11KX/LuQF2T+2nX
-oeKp5LKAbMVi
+KnJpd3seqGlVRqCVgrD0KAADJgA1QAIhAKkx0vR82QU0NtHDD11KX/LuQF2T+2nX
+oeKp5LKAbMUA
 -----END CERTIFICATE-----
 )";
 
@@ -5191,8 +5191,8 @@ MIIBJDCByqADAgECAgIE0jAKBggqhkjOPQQDAjAPMQ0wCwYDVQQDEwRUZXN0MCAX
 DTAwMDEwMTAwMDAwMFoYDzIxMDAwMTAxMDAwMDAwWjAPMQ0wCwYDVQQDEwRUZXN0
 MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE5itp4r9ln5e+Lx4NlIpM1Zdrt6ke
 DUb73ampHp3culoB59aXqAoY+cPEox5W4nyDSNsWGhz1HX7xlC1Lz3IiwaMUMBIw
-EAYDVR0TJAkEAzADAQQCAf8wCgYIKoZIzj0EAwIDSQAwRgIhAKnSIhfmzfQpeOKF
-HiAqcml3ex6oaVVGoJWCsPQoZjVAAiEAqTHS9HzZBTQ20cMPXUpf8u5AXZP7adeh
+EAYDVR0TJAkEAzADAQQCAf8wCgYIKoZIzj0EAwIDSQAwRgQhAKnSIhfmzfQpeOKF
+HiAqcml3ex6oaVVGoJWCsPQoZjVABCEAqTHS9HzZBTQ20cMPXUpf8u5AXZP7adeh
 4qnksoBsxWI=
 -----END CERTIFICATE-----
 )";
@@ -5243,9 +5243,9 @@ 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.
+  EXPECT_TRUE(CertFromPEM(kConstructedBitString));
+  EXPECT_TRUE(CertFromPEM(kConstructedOctetString));
   // Indefinite lengths are forbidden in DER.
   EXPECT_FALSE(CertFromPEM(kIndefiniteLength));
   // Padding bits in BIT STRINGs must be zero in BER.
@@ -7539,7 +7539,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