Skip to content

Reinstate support for constructed strings in crypto/asn1 #2310

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 17, 2025
Merged

Reinstate support for constructed strings in crypto/asn1 #2310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22a8204
Reinstate support for constructed strings in crypto/asn1
samuel40791765 Apr 10, 2025
989dcbb
Remove redundant malloc
samuel40791765 Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 15 additions & 13 deletions crypto/asn1/asn1_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2408,10 +2408,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) {
Expand All @@ -2431,19 +2431,21 @@ TEST(ASN1Test, IndefiniteLength) {
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.
Expand Down
112 changes: 107 additions & 5 deletions crypto/asn1/tasn_dec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -667,6 +672,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) {
Expand Down Expand Up @@ -737,11 +743,28 @@ 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;
}

// 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)) {
goto err;
}
buf.data[len] = 0;
cont = (const unsigned char *)buf.data;
} else {
cont = p;
len = plen;
Expand All @@ -756,6 +779,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;
}

Expand Down Expand Up @@ -966,6 +990,84 @@ 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) {
Copy link
Member

@skmcgrail skmcgrail Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like collect_data is never called with buf == NULL, should this return 0 instead?

Copy link
Contributor Author

@samuel40791765 samuel40791765 Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question, I did some digging around. But judging from the comment in asn1_collect which mentions "If no buffer and not indefinite length constructed just pass over the encoded data", it does seem like there's the possibility of buf being NULL and there being operations that need to be done under that circumstance.

I'll leave it as is for the time being, OpenSSL's ASN1 implementation is also like this today.

len = buf->length;
if (!BUF_MEM_grow_clean(buf, len + plen)) {
return 0;
}
OPENSSL_memcpy(buf->data + len, *p, plen);
}
*p += plen;
return 1;
}

// Check for ASN1 EOC and swallow it if found. Returns 1 if found and 0 if none.
static int asn1_check_eoc(const unsigned char **in, long len) {
const unsigned char *p;
Expand Down
81 changes: 46 additions & 35 deletions crypto/bytestring/bytestring_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down
39 changes: 39 additions & 0 deletions crypto/pkcs7/pkcs7_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -2225,4 +2251,17 @@ 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));

// The following is bit wonky with the pkcs7 bytes being read and rewritten,
// but that's how Ruby's underlying PKCS7 test gets the PEM output.
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));
}
9 changes: 5 additions & 4 deletions crypto/x509/x509_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
Loading