Skip to content

Commit

Permalink
Allow constructed strings in BER parsing (#2015)
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
WillChilds-Klein authored Dec 2, 2024
1 parent 5982853 commit 2a72226
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 14 deletions.
6 changes: 0 additions & 6 deletions crypto/asn1/tasn_dec.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 7 additions & 8 deletions crypto/x509/x509_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5178,8 +5178,8 @@ DTAwMDEwMTAwMDAwMFoYDzIxMDAwMTAxMDAwMDAwWjAPMQ0wCwYDVQQDEwRUZXN0
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE5itp4r9ln5e+Lx4NlIpM1Zdrt6ke
DUb73ampHp3culoB59aXqAoY+cPEox5W4nyDSNsWGhz1HX7xlC1Lz3IiwaMQMA4w
DAYDVR0TBAUwAwEB/zAKBggqhkjOPQQDAiNOAyQAMEYCIQCp0iIX5s30KXjihR4g
KnJpd3seqGlVRqCVgrD0KGYDJgA1QAIhAKkx0vR82QU0NtHDD11KX/LuQF2T+2nX
oeKp5LKAbMVi
KnJpd3seqGlVRqCVgrD0KAADJgA1QAIhAKkx0vR82QU0NtHDD11KX/LuQF2T+2nX
oeKp5LKAbMUA
-----END CERTIFICATE-----
)";

Expand All @@ -5191,8 +5191,8 @@ MIIBJDCByqADAgECAgIE0jAKBggqhkjOPQQDAjAPMQ0wCwYDVQQDEwRUZXN0MCAX
DTAwMDEwMTAwMDAwMFoYDzIxMDAwMTAxMDAwMDAwWjAPMQ0wCwYDVQQDEwRUZXN0
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE5itp4r9ln5e+Lx4NlIpM1Zdrt6ke
DUb73ampHp3culoB59aXqAoY+cPEox5W4nyDSNsWGhz1HX7xlC1Lz3IiwaMUMBIw
EAYDVR0TJAkEAzADAQQCAf8wCgYIKoZIzj0EAwIDSQAwRgIhAKnSIhfmzfQpeOKF
HiAqcml3ex6oaVVGoJWCsPQoZjVAAiEAqTHS9HzZBTQ20cMPXUpf8u5AXZP7adeh
EAYDVR0TJAkEAzADAQQCAf8wCgYIKoZIzj0EAwIDSQAwRgQhAKnSIhfmzfQpeOKF
HiAqcml3ex6oaVVGoJWCsPQoZjVABCEAqTHS9HzZBTQ20cMPXUpf8u5AXZP7adeh
4qnksoBsxWI=
-----END CERTIFICATE-----
)";
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 2a72226

Please sign in to comment.