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
