Reject explicit default X.509 versions and empty extension lists
X.509 version fields are declared as DEFAULT v1 which means, in DER, an
explicitly encoded version is invalid.
X.509 extension fields are declared as [3] EXPLICIT Extensions OPTIONAL
and Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension.
This means in both BER and DER, the extensions list can never be empty.
Instead, a certificate or CRL with no extensions is supposed to be
encoded by omitting the OPTIONAL extensions field.
Also apply the version and extension checks to CRL entries, which we'd
missed earlier. All these checks are done by libpki.
Update-Note: X509 and X509_CRL parsers are now more spec-compliant and
reject some invalid inputs. After cl/809456844, internal tests have
passed with this change. The change was also validated with cl/800872048
and cl/802241042. Let us know if this encounters problems.
Fixed: 42290225, 442221114
Change-Id: I9a10d76ebb258f297079fc4d77b9eaf63f60b6c4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/82087
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index a452cc6..5133d3c 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -3755,6 +3755,23 @@
-----END X509 CRL-----
)";
+// kV1CRLWithEntryExtensionsPEM is a v1 CRL with entry extensions.
+static const char kV1CRLWithEntryExtensionsPEM[] = R"(
+-----BEGIN X509 CRL-----
+MIIB7DCB1TANBgkqhkiG9w0BAQsFADBOMQswCQYDVQQGEwJVUzETMBEGA1UECAwK
+Q2FsaWZvcm5pYTEWMBQGA1UEBwwNTW91bnRhaW4gVmlldzESMBAGA1UECgwJQm9y
+aW5nU1NMFw0xNjA5MjYxNTEyNDRaFw0xNjEwMjYxNTEyNDRaMFYwEwICEAAXDTE2
+MDkyNjE1MTIyNlowEwICD/8XDTE2MDkyNjE1MTIyNlowKgICEAEXDTE2MDkyNjE1
+MTIyNlowFTATBgwqhkiG9xIEAYS3CQAEAwIBAjANBgkqhkiG9w0BAQsFAAOCAQEA
+LnqQPW8l+D5KSXGCm/jn5+n/5oAeEodQQUadBzBZu0N468lwesj2WKe1LcALn2VM
+21eNlJDonzrQ8kG7vn3KZ6W+A/aFqBZa+AxkPp9t8Lox4s6ExGIYXGTI+FOcqKA3
+5iuJSBlOs80GShvtT2kug5uH+vpAPnpFVD/mVYz6hz/yu1L+mpEile4AFrOPTeUI
+6ySjENyIK24OBLdoqWmIE4Ro/LdTFGXVWOFD7UPKJbUh5BRLx+D8Tlv8SqR0+EED
+Jf1k00LEKjpUSQRtnIu6btX8zspWN+WBRAAKZDQ/zJEI0H8pTTYAsw1lgD0MdKQ7
+1SkNL1l3X3BiUS9Q98UV5A==
+-----END X509 CRL-----
+)";
+
// kExplicitDefaultVersionCRLPEM is a v1 CRL with an explicitly-encoded version
// field.
static const char kExplicitDefaultVersionCRLPEM[] = R"(
@@ -3811,11 +3828,8 @@
// Test that the library enforces versions are valid and match the fields
// present.
TEST(X509Test, InvalidVersion) {
- // kExplicitDefaultVersionPEM is invalid but, for now, we accept it. See
- // https://crbug.com/boringssl/364.
- EXPECT_TRUE(CertFromPEM(kExplicitDefaultVersionPEM));
- EXPECT_TRUE(CRLFromPEM(kExplicitDefaultVersionCRLPEM));
-
+ EXPECT_FALSE(CertFromPEM(kExplicitDefaultVersionPEM));
+ EXPECT_FALSE(CRLFromPEM(kExplicitDefaultVersionCRLPEM));
EXPECT_FALSE(CertFromPEM(kNegativeVersionPEM));
EXPECT_FALSE(CertFromPEM(kFutureVersionPEM));
EXPECT_FALSE(CertFromPEM(kOverflowVersionPEM));
@@ -3824,6 +3838,7 @@
EXPECT_FALSE(CertFromPEM(kV1WithIssuerUniqueIDPEM));
EXPECT_FALSE(CertFromPEM(kV1WithSubjectUniqueIDPEM));
EXPECT_FALSE(CRLFromPEM(kV1CRLWithExtensionsPEM));
+ EXPECT_FALSE(CRLFromPEM(kV1CRLWithEntryExtensionsPEM));
EXPECT_FALSE(CRLFromPEM(kV3CRLPEM));
EXPECT_FALSE(CSRFromPEM(kV2CSRPEM));
@@ -3850,6 +3865,45 @@
EXPECT_FALSE(X509_REQ_set_version(req.get(), 9999));
}
+// kCRLEmptyExtension is a CRL with an empty extension list.
+static const char kCRLEmptyExtensionPEM[] = R"(
+-----BEGIN X509 CRL-----
+MIIB3DCBxQIBATANBgkqhkiG9w0BAQsFADBOMQswCQYDVQQGEwJVUzETMBEGA1UE
+CAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNTW91bnRhaW4gVmlldzESMBAGA1UECgwJ
+Qm9yaW5nU1NMFw0xNjA5MjYxNTEyNDRaFw0xNjEwMjYxNTEyNDRaMD8wEwICEAAX
+DTE2MDkyNjE1MTIyNlowEwICD/8XDTE2MDkyNjE1MTIyNlowEwICEAEXDTE2MDky
+NjE1MTIyNlqgAjAAMA0GCSqGSIb3DQEBCwUAA4IBAQAuepA9byX4PkpJcYKb+Ofn
+6f/mgB4Sh1BBRp0HMFm7Q3jryXB6yPZYp7UtwAufZUzbV42UkOifOtDyQbu+fcpn
+pb4D9oWoFlr4DGQ+n23wujHizoTEYhhcZMj4U5yooDfmK4lIGU6zzQZKG+1PaS6D
+m4f6+kA+ekVUP+ZVjPqHP/K7Uv6akSKV7gAWs49N5QjrJKMQ3Igrbg4Et2ipaYgT
+hGj8t1MUZdVY4UPtQ8oltSHkFEvH4PxOW/xKpHT4QQMl/WTTQsQqOlRJBG2ci7pu
+1fzOylY35YFEAApkND/MkQjQfylNNgCzDWWAPQx0pDvVKQ0vWXdfcGJRL1D3xRXk
+-----END X509 CRL-----
+)";
+
+// kCRLEmptyEntryExtension is a CRL with an entry with an empty extension list.
+static const char kCRLEmptyEntryExtensionPEM[] = R"(
+-----BEGIN X509 CRL-----
+MIIB2jCBwwIBATANBgkqhkiG9w0BAQsFADBOMQswCQYDVQQGEwJVUzETMBEGA1UE
+CAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNTW91bnRhaW4gVmlldzESMBAGA1UECgwJ
+Qm9yaW5nU1NMFw0xNjA5MjYxNTEyNDRaFw0xNjEwMjYxNTEyNDRaMEEwEwICEAAX
+DTE2MDkyNjE1MTIyNlowEwICD/8XDTE2MDkyNjE1MTIyNlowFQICEAEXDTE2MDky
+NjE1MTIyNlowADANBgkqhkiG9w0BAQsFAAOCAQEALnqQPW8l+D5KSXGCm/jn5+n/
+5oAeEodQQUadBzBZu0N468lwesj2WKe1LcALn2VM21eNlJDonzrQ8kG7vn3KZ6W+
+A/aFqBZa+AxkPp9t8Lox4s6ExGIYXGTI+FOcqKA35iuJSBlOs80GShvtT2kug5uH
++vpAPnpFVD/mVYz6hz/yu1L+mpEile4AFrOPTeUI6ySjENyIK24OBLdoqWmIE4Ro
+/LdTFGXVWOFD7UPKJbUh5BRLx+D8Tlv8SqR0+EEDJf1k00LEKjpUSQRtnIu6btX8
+zspWN+WBRAAKZDQ/zJEI0H8pTTYAsw1lgD0MdKQ71SkNL1l3X3BiUS9Q98UV5A==
+-----END X509 CRL-----
+)";
+
+
+// Test that the library rejects empty extension lists in CRLs.
+TEST(X509Test, EmptyCRLExtensions) {
+ EXPECT_FALSE(CRLFromPEM(kCRLEmptyExtensionPEM));
+ EXPECT_FALSE(CRLFromPEM(kCRLEmptyEntryExtensionPEM));
+}
+
// Unlike upstream OpenSSL, we require a non-null store in
// |X509_STORE_CTX_init|.
TEST(X509Test, NullStore) {
@@ -8913,13 +8967,10 @@
const char *kPaths[] = {
// Non-canonical encoding of TRUE in the critical bit.
// TODO(crbug.com/442221114): The parser should reject this.
- "crypto/x509/test/unusual_tbs_critical_ber.pem",
+ "crypto/x509/test/unusual_tbs_critical_ber.pem",
// A FALSE critical bit is encoded instead of omitted as DEFAULT.
// TODO(crbug.com/442221114): The parser should reject this.
- "crypto/x509/test/unusual_tbs_critical_false_not_omitted.pem",
- // Empty extension instead of omitting the entire field.
- // TODO(crbug.com/442221114): The parser should reject this.
- "crypto/x509/test/unusual_tbs_empty_extension_not_omitted.pem",
+ "crypto/x509/test/unusual_tbs_critical_false_not_omitted.pem",
// ecdsa-with-SHA256 AlgorithmIdentifier parameters are NULL instead of
// omitted. We accept this due to b/167375496.
"crypto/x509/test/unusual_tbs_null_sigalg_param.pem",
@@ -8932,9 +8983,6 @@
// canonical SET OF order. These are inverted.
// TODO(crbug.com/42290219): The parser should reject this.
"crypto/x509/test/unusual_tbs_wrong_attribute_order.pem",
- // A v1 version is explicit encoded instead of omitted as DEFAULT.
- // TODO(crbug.com/42290225): The parser should reject this.
- "crypto/x509/test/unusual_tbs_v1_not_omitted.pem",
};
for (const char *path : kPaths) {
SCOPED_TRACE(path);
@@ -8942,6 +8990,20 @@
ASSERT_TRUE(cert);
EXPECT_TRUE(X509_verify(cert.get(), key.get()));
}
+
+ // The following inputs were once accepted, and thus preserved in signature
+ // verification, but we no longer parse them at all.
+ const char *kInvalidPaths[] = {
+ // Empty extension instead of omitting the entire field.
+ "crypto/x509/test/unusual_tbs_empty_extension_not_omitted.pem",
+ // A v1 version is explicit encoded instead of omitted as DEFAULT.
+ "crypto/x509/test/unusual_tbs_v1_not_omitted.pem",
+ };
+ for (const char *path : kInvalidPaths) {
+ SCOPED_TRACE(path);
+ bssl::UniquePtr<X509> cert = CertFromPEM(GetTestData(path));
+ EXPECT_FALSE(cert);
+ }
}
TEST(X509Test, TrailingDataX509) {
diff --git a/crypto/x509/x_crl.cc b/crypto/x509/x_crl.cc
index 0c0874e..73a579d 100644
--- a/crypto/x509/x_crl.cc
+++ b/crypto/x509/x_crl.cc
@@ -73,10 +73,25 @@
} ASN1_SEQUENCE_END_enc(X509_CRL_INFO, X509_CRL_INFO)
static int crl_parse_entry_extensions(X509_CRL *crl) {
+ long version = ASN1_INTEGER_get(crl->crl->version);
STACK_OF(X509_REVOKED) *revoked = X509_CRL_get_REVOKED(crl);
for (size_t i = 0; i < sk_X509_REVOKED_num(revoked); i++) {
X509_REVOKED *rev = sk_X509_REVOKED_value(revoked, i);
+ // Per RFC 5280, section 5.1, CRL entry extensions require v2.
+ const STACK_OF(X509_EXTENSION) *exts = rev->extensions;
+ if (version == X509_CRL_VERSION_1 && exts != nullptr) {
+ OPENSSL_PUT_ERROR(X509, X509_R_INVALID_VERSION);
+ return 0;
+ }
+
+ // Extensions is a SEQUENCE SIZE (1..MAX), so it cannot be empty. An empty
+ // extensions list is encoded by omitting the OPTIONAL field.
+ if (exts != nullptr && sk_X509_EXTENSION_num(exts) == 0) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
+ return 0;
+ }
+
int crit;
ASN1_ENUMERATED *reason = reinterpret_cast<ASN1_ENUMERATED *>(
X509_REVOKED_get_ext_d2i(rev, NID_crl_reason, &crit, NULL));
@@ -93,7 +108,6 @@
}
// We do not support any critical CRL entry extensions.
- const STACK_OF(X509_EXTENSION) *exts = rev->extensions;
for (size_t j = 0; j < sk_X509_EXTENSION_num(exts); j++) {
const X509_EXTENSION *ext = sk_X509_EXTENSION_value(exts, j);
if (X509_EXTENSION_get_critical(ext)) {
@@ -126,10 +140,8 @@
long version = X509_CRL_VERSION_1;
if (crl->crl->version != NULL) {
version = ASN1_INTEGER_get(crl->crl->version);
- // TODO(https://crbug.com/boringssl/364): |X509_CRL_VERSION_1|
- // should also be rejected. This means an explicitly-encoded X.509v1
- // version. v1 is DEFAULT, so DER requires it be omitted.
- if (version < X509_CRL_VERSION_1 || version > X509_CRL_VERSION_2) {
+ // Versions v1 and v2. v1 is DEFAULT, so cannot be encoded explicitly.
+ if (version != X509_CRL_VERSION_2) {
OPENSSL_PUT_ERROR(X509, X509_R_INVALID_VERSION);
return 0;
}
@@ -141,6 +153,14 @@
return 0;
}
+ // Extensions is a SEQUENCE SIZE (1..MAX), so it cannot be empty. An empty
+ // extensions list is encoded by omitting the OPTIONAL field.
+ if (crl->crl->extensions != nullptr &&
+ sk_X509_EXTENSION_num(crl->crl->extensions) == 0) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
+ return 0;
+ }
+
if (!X509_CRL_digest(crl, EVP_sha256(), crl->crl_hash, NULL)) {
return 0;
}
diff --git a/crypto/x509/x_x509.cc b/crypto/x509/x_x509.cc
index 26c39fd..5467777 100644
--- a/crypto/x509/x_x509.cc
+++ b/crypto/x509/x_x509.cc
@@ -137,11 +137,9 @@
OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
return nullptr;
}
- // The version must be one of v1(0), v2(1), or v3(2).
- // TODO(https://crbug.com/42290225): Also reject |X509_VERSION_1|. v1 is
- // DEFAULT, so DER requires it be omitted.
- if (version != X509_VERSION_1 && version != X509_VERSION_2 &&
- version != X509_VERSION_3) {
+ // Versions v1, v2, and v3 are defined. v1 is DEFAULT, so cannot be encoded
+ // explicitly.
+ if (version != X509_VERSION_2 && version != X509_VERSION_3) {
OPENSSL_PUT_ERROR(X509, X509_R_INVALID_VERSION);
return nullptr;
}
@@ -189,13 +187,13 @@
OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
return nullptr;
}
- // TODO(crbug.com/42290219): Empty extension lists should be rejected. An
- // empty extensions list is encoded by omitting the field altogether. libpki
- // already rejects this.
const uint8_t *p = CBS_data(&wrapper);
ret->extensions = d2i_X509_EXTENSIONS(nullptr, &p, CBS_len(&wrapper));
if (ret->extensions == nullptr ||
- p != CBS_data(&wrapper) + CBS_len(&wrapper)) {
+ p != CBS_data(&wrapper) + CBS_len(&wrapper) ||
+ // Extensions is a SEQUENCE SIZE (1..MAX), so it cannot be empty. An
+ // empty extensions list is encoded by omitting the OPTIONAL field.
+ sk_X509_EXTENSION_num(ret->extensions) == 0) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
return nullptr;
}
