Fix some memory leaks in policy_cache_new. If a certificate has policy constraints, but the certificate policies extension is either missing or unsuitable (in a way not caught by the parser), the policy constraints object is leaked. As part of this, add some basic tests for policy constraints. Change-Id: I4a2c618019d1f92b0f3b9ad4cf6e29d4926e3095 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55752 Reviewed-by: Bob Beck <bbe@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/test/make_policy_certs.go b/crypto/x509/test/make_policy_certs.go index a4da0a2..73fcae3 100644 --- a/crypto/x509/test/make_policy_certs.go +++ b/crypto/x509/test/make_policy_certs.go
@@ -25,6 +25,9 @@ "math/big" "os" "time" + + "golang.org/x/crypto/cryptobyte" + cbasn1 "golang.org/x/crypto/cryptobyte/asn1" ) var ( @@ -34,6 +37,9 @@ // https://www.rfc-editor.org/rfc/rfc5280.html#section-4.2.1.4 certificatePoliciesOID = asn1.ObjectIdentifier([]int{2, 5, 29, 32}) + + // https://www.rfc-editor.org/rfc/rfc5280.html#section-4.2.1.11 + policyConstraintsOID = asn1.ObjectIdentifier([]int{2, 5, 29, 36}) ) var leafKey, intermediateKey, rootKey *ecdsa.PrivateKey @@ -147,6 +153,24 @@ intermediateDuplicate.template.PolicyIdentifiers = []asn1.ObjectIdentifier{testOID1, testOID2, testOID2} mustGenerateCertificate("policy_intermediate_duplicate.pem", &intermediateDuplicate, &root) + // A version of the intermediate that sets requireExplicitPolicy without + // skipping certificates. + b := cryptobyte.NewBuilder(nil) + b.AddASN1(cbasn1.SEQUENCE, func(seq *cryptobyte.Builder) { + seq.AddASN1Int64WithTag(0, cbasn1.Tag(0).ContextSpecific()) + }) + intermediateRequire := intermediate + intermediateRequire.template.ExtraExtensions = []pkix.Extension{{Id: policyConstraintsOID, Value: b.BytesOrPanic()}} + mustGenerateCertificate("policy_intermediate_require.pem", &intermediateRequire, &root) + + // Same as above, but there are no policies on the intermediate. + intermediateRequire.template.PolicyIdentifiers = nil + mustGenerateCertificate("policy_intermediate_require_no_policies.pem", &intermediateRequire, &root) + + // Same as above, but the policy list has duplicates. + intermediateRequire.template.PolicyIdentifiers = []asn1.ObjectIdentifier{testOID1, testOID2, testOID2} + mustGenerateCertificate("policy_intermediate_require_duplicate.pem", &intermediateRequire, &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_require.pem b/crypto/x509/test/policy_intermediate_require.pem new file mode 100644 index 0000000..c141bfc --- /dev/null +++ b/crypto/x509/test/policy_intermediate_require.pem
@@ -0,0 +1,12 @@ +-----BEGIN CERTIFICATE----- +MIIBuTCCAV+gAwIBAgIBAjAKBggqhkjOPQQDAjAWMRQwEgYDVQQDEwtQb2xpY3kg +Um9vdDAgFw0wMDAxMDEwMDAwMDBaGA8yMTAwMDEwMTAwMDAwMFowHjEcMBoGA1UE +AxMTUG9saWN5IEludGVybWVkaWF0ZTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IA +BOI6fKiM3jFLkLyAn88cvlw4SwxuygRjopP3FFBKHyUQvh3VVvfqSpSCSmp50Qia +jQ6Dg7CTpVZVVH+bguT7JTCjgZMwgZAwDgYDVR0PAQH/BAQDAgIEMBMGA1UdJQQM +MAoGCCsGAQUFBwMBMA8GA1UdEwEB/wQFMAMBAf8wHQYDVR0OBBYEFJDS9/4O7qhr +CIRhwsXrPVBagG2uMCsGA1UdIAQkMCIwDwYNKoZIhvcSBAGEtwkCATAPBg0qhkiG +9xIEAYS3CQICMAwGA1UdJAQFMAOAAQAwCgYIKoZIzj0EAwIDSAAwRQIhALuEN9FY +tVyp8yVAjc/XDZMY86/p/9nA0t1x4fYJz6T4AiACPS/Lx3PD4FfypUZGlY3uv6TJ +Dv3tfaa/0GLD98VNIw== +-----END CERTIFICATE-----
diff --git a/crypto/x509/test/policy_intermediate_require_duplicate.pem b/crypto/x509/test/policy_intermediate_require_duplicate.pem new file mode 100644 index 0000000..b52a8d1 --- /dev/null +++ b/crypto/x509/test/policy_intermediate_require_duplicate.pem
@@ -0,0 +1,12 @@ +-----BEGIN CERTIFICATE----- +MIIByjCCAXCgAwIBAgIBAjAKBggqhkjOPQQDAjAWMRQwEgYDVQQDEwtQb2xpY3kg +Um9vdDAgFw0wMDAxMDEwMDAwMDBaGA8yMTAwMDEwMTAwMDAwMFowHjEcMBoGA1UE +AxMTUG9saWN5IEludGVybWVkaWF0ZTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IA +BOI6fKiM3jFLkLyAn88cvlw4SwxuygRjopP3FFBKHyUQvh3VVvfqSpSCSmp50Qia +jQ6Dg7CTpVZVVH+bguT7JTCjgaQwgaEwDgYDVR0PAQH/BAQDAgIEMBMGA1UdJQQM +MAoGCCsGAQUFBwMBMA8GA1UdEwEB/wQFMAMBAf8wHQYDVR0OBBYEFJDS9/4O7qhr +CIRhwsXrPVBagG2uMDwGA1UdIAQ1MDMwDwYNKoZIhvcSBAGEtwkCATAPBg0qhkiG +9xIEAYS3CQICMA8GDSqGSIb3EgQBhLcJAgIwDAYDVR0kBAUwA4ABADAKBggqhkjO +PQQDAgNIADBFAiEAlowPT0PE6vY2KKCdgh/7ji7x6NBD73OeFe07lk9aBUACIH6R +ann4GsMCEqkptXfBZ00SIAHJIioAEW+E/8wXU8TX +-----END CERTIFICATE-----
diff --git a/crypto/x509/test/policy_intermediate_require_no_policies.pem b/crypto/x509/test/policy_intermediate_require_no_policies.pem new file mode 100644 index 0000000..a78878e --- /dev/null +++ b/crypto/x509/test/policy_intermediate_require_no_policies.pem
@@ -0,0 +1,11 @@ +-----BEGIN CERTIFICATE----- +MIIBijCCATCgAwIBAgIBAjAKBggqhkjOPQQDAjAWMRQwEgYDVQQDEwtQb2xpY3kg +Um9vdDAgFw0wMDAxMDEwMDAwMDBaGA8yMTAwMDEwMTAwMDAwMFowHjEcMBoGA1UE +AxMTUG9saWN5IEludGVybWVkaWF0ZTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IA +BOI6fKiM3jFLkLyAn88cvlw4SwxuygRjopP3FFBKHyUQvh3VVvfqSpSCSmp50Qia +jQ6Dg7CTpVZVVH+bguT7JTCjZTBjMA4GA1UdDwEB/wQEAwICBDATBgNVHSUEDDAK +BggrBgEFBQcDATAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBSQ0vf+Du6oawiE +YcLF6z1QWoBtrjAMBgNVHSQEBTADgAEAMAoGCCqGSM49BAMCA0gAMEUCIF2jnBVg +7hNV09+9ZS7bGaf5GcRifHOkAUC6M596ECXCAiEApuyn9lyP15qD/mHtMz8eYrKn +8HygEypeq3ClqqKx2c4= +-----END CERTIFICATE-----
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 5dca616..248544a 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc
@@ -5119,6 +5119,19 @@ GetTestData("crypto/x509/test/policy_intermediate_duplicate.pem") .c_str())); ASSERT_TRUE(intermediate_duplicate); + bssl::UniquePtr<X509> intermediate_require(CertFromPEM( + GetTestData("crypto/x509/test/policy_intermediate_require.pem") + .c_str())); + ASSERT_TRUE(intermediate_require); + bssl::UniquePtr<X509> intermediate_require_duplicate(CertFromPEM( + GetTestData("crypto/x509/test/policy_intermediate_require_duplicate.pem") + .c_str())); + ASSERT_TRUE(intermediate_require_duplicate); + bssl::UniquePtr<X509> intermediate_require_no_policies(CertFromPEM( + GetTestData( + "crypto/x509/test/policy_intermediate_require_no_policies.pem") + .c_str())); + ASSERT_TRUE(intermediate_require_no_policies); bssl::UniquePtr<X509> leaf( CertFromPEM(GetTestData("crypto/x509/test/policy_leaf.pem").c_str())); ASSERT_TRUE(leaf); @@ -5145,6 +5158,9 @@ ASSERT_TRUE(X509_VERIFY_PARAM_add0_policy(param, copy.get())); copy.release(); // |X509_VERIFY_PARAM_add0_policy| takes ownership on // success. + // TODO(davidben): |X509_VERIFY_PARAM_add0_policy| does not set this flag, + // while |X509_VERIFY_PARAM_set1_policies| does. Is this a bug? + X509_VERIFY_PARAM_set_flags(param, X509_V_FLAG_POLICY_CHECK); } }; @@ -5204,4 +5220,49 @@ [&](X509_VERIFY_PARAM *param) { set_policies(param, {oid1.get()}); })); + + // Without |X509_V_FLAG_EXPLICIT_POLICY|, the policy tree is built and + // intersected with user-specified policies, but it is not required to result + // in any valid policies. + EXPECT_EQ(X509_V_OK, + Verify(leaf.get(), {root.get()}, {intermediate.get()}, /*crls=*/{}, + /*flags=*/0, [&](X509_VERIFY_PARAM *param) { + set_policies(param, {oid1.get()}); + })); + EXPECT_EQ(X509_V_OK, + Verify(leaf.get(), {root.get()}, {intermediate.get()}, /*crls=*/{}, + /*flags=*/0, [&](X509_VERIFY_PARAM *param) { + set_policies(param, {oid3.get()}); + })); + + // However, a CA with policy constraints can require an explicit policy. + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, + {intermediate_require.get()}, /*crls=*/{}, + /*flags=*/0, [&](X509_VERIFY_PARAM *param) { + set_policies(param, {oid1.get()}); + })); + EXPECT_EQ(X509_V_ERR_NO_EXPLICIT_POLICY, + Verify(leaf.get(), {root.get()}, {intermediate_require.get()}, + /*crls=*/{}, + /*flags=*/0, [&](X509_VERIFY_PARAM *param) { + set_policies(param, {oid3.get()}); + })); + + // An intermediate that requires an explicit policy, but then specifies no + // policies should fail verification as a result. + EXPECT_EQ(X509_V_ERR_NO_EXPLICIT_POLICY, + Verify(leaf.get(), {root.get()}, + {intermediate_require_no_policies.get()}, /*crls=*/{}, + /*flags=*/0, [&](X509_VERIFY_PARAM *param) { + set_policies(param, {oid1.get()}); + })); + + // A constrained intermediate's policy extension has a duplicate policy, which + // is invalid. Historically this, and the above case, leaked memory. + EXPECT_EQ(X509_V_ERR_INVALID_POLICY_EXTENSION, + Verify(leaf.get(), {root.get()}, + {intermediate_require_duplicate.get()}, /*crls=*/{}, + /*flags=*/0, [&](X509_VERIFY_PARAM *param) { + set_policies(param, {oid1.get()}); + })); }
diff --git a/crypto/x509v3/pcy_cache.c b/crypto/x509v3/pcy_cache.c index 74f95f9..3a351dc 100644 --- a/crypto/x509v3/pcy_cache.c +++ b/crypto/x509v3/pcy_cache.c
@@ -123,16 +123,15 @@ return ret; } -static int policy_cache_new(X509 *x) { +static void policy_cache_new(X509 *x) { X509_POLICY_CACHE *cache; ASN1_INTEGER *ext_any = NULL; POLICY_CONSTRAINTS *ext_pcons = NULL; CERTIFICATEPOLICIES *ext_cpols = NULL; POLICY_MAPPINGS *ext_pmaps = NULL; - int i; cache = OPENSSL_malloc(sizeof(X509_POLICY_CACHE)); if (!cache) { - return 0; + return; } cache->anyPolicy = NULL; cache->data = NULL; @@ -144,10 +143,10 @@ // Handle requireExplicitPolicy *first*. Need to process this even if we // don't have any policies. - ext_pcons = X509_get_ext_d2i(x, NID_policy_constraints, &i, NULL); - + int critical; + ext_pcons = X509_get_ext_d2i(x, NID_policy_constraints, &critical, NULL); if (!ext_pcons) { - if (i != -1) { + if (critical != -1) { goto bad_cache; } } else { @@ -164,45 +163,42 @@ } } - // Process CertificatePolicies - - ext_cpols = X509_get_ext_d2i(x, NID_certificate_policies, &i, NULL); + ext_cpols = X509_get_ext_d2i(x, NID_certificate_policies, &critical, NULL); // If no CertificatePolicies extension or problem decoding then there is // no point continuing because the valid policies will be NULL. if (!ext_cpols) { // If not absent some problem with extension - if (i != -1) { + if (critical != -1) { goto bad_cache; } - return 1; + goto done; } - i = policy_cache_create(x, ext_cpols, i); - - // NB: ext_cpols freed by policy_cache_set_policies - - if (i <= 0) { - return i; + // This call frees |ext_cpols|. + if (policy_cache_create(x, ext_cpols, critical) <= 0) { + // |policy_cache_create| already sets |EXFLAG_INVALID_POLICY|. + // + // TODO(davidben): While it does, it's missing some spots. Align this and + // |policy_cache_set_mapping|. + goto done; } - ext_pmaps = X509_get_ext_d2i(x, NID_policy_mappings, &i, NULL); - + ext_pmaps = X509_get_ext_d2i(x, NID_policy_mappings, &critical, NULL); if (!ext_pmaps) { // If not absent some problem with extension - if (i != -1) { + if (critical != -1) { goto bad_cache; } } else { - i = policy_cache_set_mapping(x, ext_pmaps); - if (i <= 0) { + // This call frees |ext_pmaps|. + if (policy_cache_set_mapping(x, ext_pmaps) <= 0) { goto bad_cache; } } - ext_any = X509_get_ext_d2i(x, NID_inhibit_any_policy, &i, NULL); - + ext_any = X509_get_ext_d2i(x, NID_inhibit_any_policy, &critical, NULL); if (!ext_any) { - if (i != -1) { + if (critical != -1) { goto bad_cache; } } else if (!policy_cache_set_int(&cache->any_skip, ext_any)) { @@ -214,15 +210,9 @@ x->ex_flags |= EXFLAG_INVALID_POLICY; } - if (ext_pcons) { - POLICY_CONSTRAINTS_free(ext_pcons); - } - - if (ext_any) { - ASN1_INTEGER_free(ext_any); - } - - return 1; +done: + POLICY_CONSTRAINTS_free(ext_pcons); + ASN1_INTEGER_free(ext_any); } void policy_cache_free(X509_POLICY_CACHE *cache) {
diff --git a/go.mod b/go.mod index 5d8b372..5fd27f4 100644 --- a/go.mod +++ b/go.mod
@@ -3,11 +3,11 @@ go 1.19 require ( - golang.org/x/crypto v0.0.0-20210513164829-c07d793c2f9a - golang.org/x/net v0.0.0-20210614182718-04defd469f4e + golang.org/x/crypto v0.4.0 + golang.org/x/net v0.3.0 ) require ( - golang.org/x/sys v0.0.0-20210423082822-04245dca01da // indirect - golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1 // indirect + golang.org/x/sys v0.3.0 // indirect + golang.org/x/term v0.3.0 // indirect )
diff --git a/go.sum b/go.sum index e08ca5f..26fc6b6 100644 --- a/go.sum +++ b/go.sum
@@ -1,9 +1,17 @@ golang.org/x/crypto v0.0.0-20210513164829-c07d793c2f9a h1:kr2P4QFmQr29mSLA43kwrOcgcReGTfbE9N577tCTuBc= golang.org/x/crypto v0.0.0-20210513164829-c07d793c2f9a/go.mod h1:P+XmwS30IXTQdn5tA2iutPOUgjI07+tq3H3K9MVA1s8= +golang.org/x/crypto v0.4.0 h1:UVQgzMY87xqpKNgb+kDsll2Igd33HszWHFLmpaRMq/8= +golang.org/x/crypto v0.4.0/go.mod h1:3quD/ATkf6oY+rnes5c3ExXTbLc8mueNue5/DoinL80= golang.org/x/net v0.0.0-20210614182718-04defd469f4e h1:XpT3nA5TvE525Ne3hInMh6+GETgn27Zfm9dxsThnX2Q= golang.org/x/net v0.0.0-20210614182718-04defd469f4e/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= +golang.org/x/net v0.3.0 h1:VWL6FNY2bEEmsGVKabSlHu5Irp34xmMRoqb/9lF9lxk= +golang.org/x/net v0.3.0/go.mod h1:MBQ8lrhLObU/6UmLb4fmbmk5OcyYmqtbGd/9yIeKjEE= golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210423082822-04245dca01da h1:b3NXsE2LusjYGGjL5bxEVZZORm/YEFFrWFjR8eFrw/c= golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.3.0 h1:w8ZOecv6NaNa/zC8944JTU3vz4u6Lagfk4RPQxv92NQ= +golang.org/x/sys v0.3.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1 h1:v+OssWQX+hTHEmOBgwxdZxK4zHq3yOs8F9J7mk0PY8E= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= +golang.org/x/term v0.3.0 h1:qoo4akIqOcDME5bhc/NgxUdovd6BSS2uMsVjB56q1xI= +golang.org/x/term v0.3.0/go.mod h1:q750SLmJuPmVoN1blW3UFBPREJfb1KmY3vwxfr+nFDA=
diff --git a/sources.cmake b/sources.cmake index 46788fa..4dcbbc1 100644 --- a/sources.cmake +++ b/sources.cmake
@@ -114,6 +114,9 @@ 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_require.pem + crypto/x509/test/policy_intermediate_require_duplicate.pem + crypto/x509/test/policy_intermediate_require_no_policies.pem crypto/x509/test/policy_intermediate.pem crypto/x509/test/policy_leaf_duplicate.pem crypto/x509/test/policy_leaf_invalid.pem