Skip to content

Commit 1a1eb18

Browse files
Revert "Allow constructed strings in BER parsing (#2015)" (#2278)
This reverts commit 2a72226. ### Issues: Addresses `CryptoAlg-3037` ### Description of changes: We've ran into issues with parsing indefinite BER with PKCS7 and it turns out our support for parsing is not as complete. Instead of parsing BER to an invalid unusable state that's confusing, we should outright disallow parsing of constructed strings in BER until we fix the issue. ### Call-outs: N/A ### Testing: Original tests. Ruby CI is expected to fail. I'll update the patches in another PR. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
1 parent 8a9ebcf commit 1a1eb18

File tree

2 files changed

+14
-7
lines changed

2 files changed

+14
-7
lines changed

crypto/asn1/tasn_dec.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -681,6 +681,12 @@ static int asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in,
681681
cont = *in;
682682
len = p - cont + plen;
683683
p += plen;
684+
} else if (cst) {
685+
// This parser historically supported BER constructed strings. We no
686+
// longer do and will gradually tighten this parser into a DER
687+
// parser. BER types should use |CBS_asn1_ber_to_der|.
688+
OPENSSL_PUT_ERROR(ASN1, ASN1_R_TYPE_NOT_PRIMITIVE);
689+
return 0;
684690
} else {
685691
cont = p;
686692
len = plen;

crypto/x509/x509_test.cc

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5159,8 +5159,8 @@ DTAwMDEwMTAwMDAwMFoYDzIxMDAwMTAxMDAwMDAwWjAPMQ0wCwYDVQQDEwRUZXN0
51595159
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE5itp4r9ln5e+Lx4NlIpM1Zdrt6ke
51605160
DUb73ampHp3culoB59aXqAoY+cPEox5W4nyDSNsWGhz1HX7xlC1Lz3IiwaMQMA4w
51615161
DAYDVR0TBAUwAwEB/zAKBggqhkjOPQQDAiNOAyQAMEYCIQCp0iIX5s30KXjihR4g
5162-
KnJpd3seqGlVRqCVgrD0KAADJgA1QAIhAKkx0vR82QU0NtHDD11KX/LuQF2T+2nX
5163-
oeKp5LKAbMUA
5162+
KnJpd3seqGlVRqCVgrD0KGYDJgA1QAIhAKkx0vR82QU0NtHDD11KX/LuQF2T+2nX
5163+
oeKp5LKAbMVi
51645164
-----END CERTIFICATE-----
51655165
)";
51665166

@@ -5172,8 +5172,8 @@ MIIBJDCByqADAgECAgIE0jAKBggqhkjOPQQDAjAPMQ0wCwYDVQQDEwRUZXN0MCAX
51725172
DTAwMDEwMTAwMDAwMFoYDzIxMDAwMTAxMDAwMDAwWjAPMQ0wCwYDVQQDEwRUZXN0
51735173
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE5itp4r9ln5e+Lx4NlIpM1Zdrt6ke
51745174
DUb73ampHp3culoB59aXqAoY+cPEox5W4nyDSNsWGhz1HX7xlC1Lz3IiwaMUMBIw
5175-
EAYDVR0TJAkEAzADAQQCAf8wCgYIKoZIzj0EAwIDSQAwRgQhAKnSIhfmzfQpeOKF
5176-
HiAqcml3ex6oaVVGoJWCsPQoZjVABCEAqTHS9HzZBTQ20cMPXUpf8u5AXZP7adeh
5175+
EAYDVR0TJAkEAzADAQQCAf8wCgYIKoZIzj0EAwIDSQAwRgIhAKnSIhfmzfQpeOKF
5176+
HiAqcml3ex6oaVVGoJWCsPQoZjVAAiEAqTHS9HzZBTQ20cMPXUpf8u5AXZP7adeh
51775177
4qnksoBsxWI=
51785178
-----END CERTIFICATE-----
51795179
)";
@@ -5224,9 +5224,9 @@ soBsxWI=
52245224
)";
52255225

52265226
TEST(X509Test, BER) {
5227-
// Constructed strings are forbidden in DER, but allowed in BER.
5228-
EXPECT_TRUE(CertFromPEM(kConstructedBitString));
5229-
EXPECT_TRUE(CertFromPEM(kConstructedOctetString));
5227+
// Constructed strings are forbidden in DER.
5228+
EXPECT_FALSE(CertFromPEM(kConstructedBitString));
5229+
EXPECT_FALSE(CertFromPEM(kConstructedOctetString));
52305230
// Indefinite lengths are forbidden in DER.
52315231
EXPECT_FALSE(CertFromPEM(kIndefiniteLength));
52325232
// Padding bits in BIT STRINGs must be zero in BER.
@@ -7520,6 +7520,7 @@ TEST(X509Test, NameAttributeValues) {
75207520
{CBS_ASN1_UNIVERSALSTRING, "not utf-32"},
75217521
{CBS_ASN1_UTCTIME, "not utctime"},
75227522
{CBS_ASN1_GENERALIZEDTIME, "not generalizedtime"},
7523+
{CBS_ASN1_UTF8STRING | CBS_ASN1_CONSTRUCTED, ""},
75237524
{CBS_ASN1_SEQUENCE & ~CBS_ASN1_CONSTRUCTED, ""},
75247525

75257526
// TODO(crbug.com/boringssl/412): The following inputs should parse, but

0 commit comments

Comments
 (0)