From 837ade76fd2a0ce8a310b0d7c8d7b523fcb23047 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 13 Dec 2022 19:59:48 -0500 Subject: [PATCH] Add tests for rejecting duplicate policy OIDs. Change-Id: I738490d70208885481f638273d663140444c3a76 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55825 Reviewed-by: Bob Beck Commit-Queue: David Benjamin --- crypto/x509/test/make_policy_certs.go | 9 +++++++ .../test/policy_intermediate_duplicate.pem | 12 +++++++++ crypto/x509/test/policy_leaf_duplicate.pem | 12 +++++++++ crypto/x509/x509_test.cc | 25 +++++++++++++++++-- sources.cmake | 2 ++ 5 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 crypto/x509/test/policy_intermediate_duplicate.pem create mode 100644 crypto/x509/test/policy_leaf_duplicate.pem diff --git a/crypto/x509/test/make_policy_certs.go b/crypto/x509/test/make_policy_certs.go index 18a6ffe98..a4da0a230 100644 --- a/crypto/x509/test/make_policy_certs.go +++ b/crypto/x509/test/make_policy_certs.go @@ -138,6 +138,15 @@ func main() { intermediateInvalid.template.ExtraExtensions = []pkix.Extension{{Id: certificatePoliciesOID, Value: []byte("INVALID")}} mustGenerateCertificate("policy_intermediate_invalid.pem", &intermediateInvalid, &root) + // Duplicates are not allowed in certificatePolicies. + leafDuplicate := leaf + leafDuplicate.template.PolicyIdentifiers = []asn1.ObjectIdentifier{testOID1, testOID2, testOID2} + mustGenerateCertificate("policy_leaf_duplicate.pem", &leafDuplicate, &root) + + intermediateDuplicate := intermediate + intermediateDuplicate.template.PolicyIdentifiers = []asn1.ObjectIdentifier{testOID1, testOID2, testOID2} + mustGenerateCertificate("policy_intermediate_duplicate.pem", &intermediateDuplicate, &root) + // TODO(davidben): Generate more certificates to test policy validation more // extensively, including an intermediate with constraints. For now this // just tests the basic case. diff --git a/crypto/x509/test/policy_intermediate_duplicate.pem b/crypto/x509/test/policy_intermediate_duplicate.pem new file mode 100644 index 000000000..af0fe8973 --- /dev/null +++ b/crypto/x509/test/policy_intermediate_duplicate.pem @@ -0,0 +1,12 @@ +-----BEGIN CERTIFICATE----- +MIIBuzCCAWKgAwIBAgIBAjAKBggqhkjOPQQDAjAWMRQwEgYDVQQDEwtQb2xpY3kg +Um9vdDAgFw0wMDAxMDEwMDAwMDBaGA8yMTAwMDEwMTAwMDAwMFowHjEcMBoGA1UE +AxMTUG9saWN5IEludGVybWVkaWF0ZTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IA +BOI6fKiM3jFLkLyAn88cvlw4SwxuygRjopP3FFBKHyUQvh3VVvfqSpSCSmp50Qia +jQ6Dg7CTpVZVVH+bguT7JTCjgZYwgZMwDgYDVR0PAQH/BAQDAgIEMBMGA1UdJQQM +MAoGCCsGAQUFBwMBMA8GA1UdEwEB/wQFMAMBAf8wHQYDVR0OBBYEFJDS9/4O7qhr +CIRhwsXrPVBagG2uMDwGA1UdIAQ1MDMwDwYNKoZIhvcSBAGEtwkCATAPBg0qhkiG +9xIEAYS3CQICMA8GDSqGSIb3EgQBhLcJAgIwCgYIKoZIzj0EAwIDRwAwRAIgCnvy +K47AK/Ve/rzcFSm1fcjFg9UwZoTvOAhZU/xpfLgCIFV4vHl6jsGq9rPs4KblSsIY +VBjAjG2AYkH0Lq+O4LjO +-----END CERTIFICATE----- diff --git a/crypto/x509/test/policy_leaf_duplicate.pem b/crypto/x509/test/policy_leaf_duplicate.pem new file mode 100644 index 000000000..80fce22b1 --- /dev/null +++ b/crypto/x509/test/policy_leaf_duplicate.pem @@ -0,0 +1,12 @@ +-----BEGIN CERTIFICATE----- +MIIBsjCCAVigAwIBAgIBAzAKBggqhkjOPQQDAjAWMRQwEgYDVQQDEwtQb2xpY3kg +Um9vdDAgFw0wMDAxMDEwMDAwMDBaGA8yMTAwMDEwMTAwMDAwMFowGjEYMBYGA1UE +AxMPd3d3LmV4YW1wbGUuY29tMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEkSrY +vFVtkZJmvirfY0JDDYrZQrNJecPLt0ksJux2URL5nAQiQY1SERGnEaiNLpoc0dle +TS8wQT/cjw/wPgoeV6OBkDCBjTAOBgNVHQ8BAf8EBAMCAgQwEwYDVR0lBAwwCgYI +KwYBBQUHAwEwDAYDVR0TAQH/BAIwADAaBgNVHREEEzARgg93d3cuZXhhbXBsZS5j +b20wPAYDVR0gBDUwMzAPBg0qhkiG9xIEAYS3CQIBMA8GDSqGSIb3EgQBhLcJAgIw +DwYNKoZIhvcSBAGEtwkCAjAKBggqhkjOPQQDAgNIADBFAiEA3MEtsp3pypprhmPB +UbMC7FwvK+YZI5qo5dDRGUu0H6QCIEbUDagJc0qNdvZ4H//E/cvqb8dH6UmmIXVX +/WMkIJt2 +-----END CERTIFICATE----- diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index a69b2846c..5dca61650 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -5115,12 +5115,19 @@ TEST(X509Test, Policy) { bssl::UniquePtr intermediate_invalid(CertFromPEM( GetTestData("crypto/x509/test/policy_intermediate_invalid.pem").c_str())); ASSERT_TRUE(intermediate_invalid); + bssl::UniquePtr intermediate_duplicate(CertFromPEM( + GetTestData("crypto/x509/test/policy_intermediate_duplicate.pem") + .c_str())); + ASSERT_TRUE(intermediate_duplicate); bssl::UniquePtr leaf( CertFromPEM(GetTestData("crypto/x509/test/policy_leaf.pem").c_str())); ASSERT_TRUE(leaf); bssl::UniquePtr leaf_invalid(CertFromPEM( GetTestData("crypto/x509/test/policy_leaf_invalid.pem").c_str())); ASSERT_TRUE(leaf_invalid); + bssl::UniquePtr leaf_duplicate(CertFromPEM( + GetTestData("crypto/x509/test/policy_leaf_duplicate.pem").c_str())); + ASSERT_TRUE(leaf_duplicate); // By default, OpenSSL does not check policies, so even syntax errors in the // certificatePolicies extension go unnoticed. (This is probably not @@ -5168,17 +5175,31 @@ TEST(X509Test, Policy) { set_policies(param, {oid1.get(), oid3.get()}); })); - // The policy extension in the intermediate cannot be parsed. + // The policy extension cannot be parsed. EXPECT_EQ(X509_V_ERR_INVALID_POLICY_EXTENSION, Verify(leaf.get(), {root.get()}, {intermediate_invalid.get()}, /*crls=*/{}, X509_V_FLAG_EXPLICIT_POLICY, [&](X509_VERIFY_PARAM *param) { set_policies(param, {oid1.get()}); })); + EXPECT_EQ(X509_V_ERR_INVALID_POLICY_EXTENSION, + Verify(leaf_invalid.get(), {root.get()}, {intermediate.get()}, + /*crls=*/{}, X509_V_FLAG_EXPLICIT_POLICY, + [&](X509_VERIFY_PARAM *param) { + set_policies(param, {oid1.get()}); + })); + + // There is a duplicate policy in the policy extension. + EXPECT_EQ(X509_V_ERR_INVALID_POLICY_EXTENSION, + Verify(leaf.get(), {root.get()}, {intermediate_duplicate.get()}, + /*crls=*/{}, X509_V_FLAG_EXPLICIT_POLICY, + [&](X509_VERIFY_PARAM *param) { + set_policies(param, {oid1.get()}); + })); // The policy extension in the leaf cannot be parsed. EXPECT_EQ(X509_V_ERR_INVALID_POLICY_EXTENSION, - Verify(leaf_invalid.get(), {root.get()}, {intermediate.get()}, + Verify(leaf_duplicate.get(), {root.get()}, {intermediate.get()}, /*crls=*/{}, X509_V_FLAG_EXPLICIT_POLICY, [&](X509_VERIFY_PARAM *param) { set_policies(param, {oid1.get()}); diff --git a/sources.cmake b/sources.cmake index 69a1f6426..46788fac5 100644 --- a/sources.cmake +++ b/sources.cmake @@ -112,8 +112,10 @@ set( crypto/x509/test/many_names2.pem crypto/x509/test/many_names3.pem crypto/x509/test/policy_root.pem + crypto/x509/test/policy_intermediate_duplicate.pem crypto/x509/test/policy_intermediate_invalid.pem crypto/x509/test/policy_intermediate.pem + crypto/x509/test/policy_leaf_duplicate.pem crypto/x509/test/policy_leaf_invalid.pem crypto/x509/test/policy_leaf.pem crypto/x509/test/pss_sha1_explicit.pem