crypto/x509: Fix handling of pathLenConstraint on self-issued intermediates.
The test case that fails before this CL is:
```
EXPECT_EQ(X509_V_ERR_PATH_LENGTH_EXCEEDED,
VerifyChain({{/*self_issued=*/false, /*pathlen=/0},
{/*self_issued=*/false, /*pathlen=/0},
{/*self_issued=*/true, /*pathlen=/0},
{/*self_issued=*/false, /*pathlen=/1},
{/*self_issued=*/true, /*pathlen=/2}}));
```
as the path length of the migrating-to (and self-issued) intermediate
certificate was ignored.
Bug: 525690632
Change-Id: Id5d74cc3f93156d40db61dc83433e4dc6a6a6964
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95067
Commit-Queue: Rudolf Polzer <rpolzer@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 2401c45..4e04c77 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -16,6 +16,7 @@
#include <algorithm>
#include <functional>
+#include <initializer_list>
#include <iterator>
#include <memory>
#include <string>
@@ -2036,9 +2037,9 @@
return name;
}
-static bssl::UniquePtr<X509> MakeTestCert(std::string_view issuer,
- std::string_view subject,
- EVP_PKEY *key, bool is_ca) {
+static bssl::UniquePtr<X509> MakeTestCert(
+ std::string_view issuer, std::string_view subject, EVP_PKEY *key,
+ bool is_ca, std::optional<int64_t> pathlen = std::nullopt) {
UniquePtr<X509_NAME> issuer_name = MakeTestName(issuer);
UniquePtr<X509_NAME> subject_name = MakeTestName(subject);
UniquePtr<X509> cert(X509_new());
@@ -2060,6 +2061,13 @@
return nullptr;
}
bc->ca = is_ca ? ASN1_BOOLEAN_TRUE : ASN1_BOOLEAN_FALSE;
+ if (pathlen.has_value()) {
+ bc->pathlen = ASN1_INTEGER_new();
+ if (bc->pathlen == nullptr ||
+ !ASN1_INTEGER_set_int64(bc->pathlen, *pathlen)) {
+ return nullptr;
+ }
+ }
if (!X509_add1_ext_i2d(cert.get(), NID_basic_constraints, bc.get(),
/*crit=*/1, /*flags=*/0)) {
return nullptr;
@@ -10440,5 +10448,133 @@
EXPECT_EQ(actual, expected);
}
+struct CertChainItem {
+ bool self_issued;
+ std::optional<int64_t> pathlen;
+};
+
+static std::optional<int> VerifyChain(
+ std::initializer_list<CertChainItem> items) {
+ std::vector<bssl::UniquePtr<X509>> intermediates;
+
+ std::vector<UniquePtr<EVP_PKEY>> keys;
+ for (size_t k = 0; k < items.size(); ++k) {
+ UniquePtr<EVP_PKEY> key(EVP_PKEY_generate_from_alg(EVP_pkey_ec_p256()));
+ if (key == nullptr) {
+ return std::nullopt;
+ }
+ keys.push_back(std::move(key));
+ }
+
+ bssl::UniquePtr<X509> leaf = nullptr;
+
+ size_t name_idx = 0;
+ size_t key_idx = 0;
+ for (const auto &item : items) {
+ std::string subject_name = std::to_string(name_idx);
+ if (!item.self_issued) {
+ ++name_idx;
+ }
+ std::string issuer_name = std::to_string(name_idx);
+
+ size_t subject_key_idx = key_idx;
+ if (key_idx < items.size() - 1) {
+ ++key_idx;
+ }
+ size_t issuer_key_idx = key_idx;
+
+ EVP_PKEY *subject_key = keys[subject_key_idx].get();
+ EVP_PKEY *issuer_key = keys[issuer_key_idx].get();
+
+ bssl::UniquePtr<X509> cert(MakeTestCert(
+ issuer_name, subject_name, subject_key, /*is_ca=*/true, item.pathlen));
+ uint8_t skid = static_cast<uint8_t>(subject_key_idx);
+ uint8_t akid = static_cast<uint8_t>(issuer_key_idx);
+ if (cert == nullptr || !AddSubjectKeyIdentifier(cert.get(), {&skid, 1}) ||
+ !AddAuthorityKeyIdentifier(cert.get(), {&akid, 1}) ||
+ !X509_sign(cert.get(), issuer_key, EVP_sha256())) {
+ return std::nullopt;
+ }
+ if (leaf == nullptr) {
+ // The first cert in the list is the leaf.
+ leaf = std::move(cert);
+ } else {
+ // All else gets into the chain for now.
+ intermediates.emplace_back(std::move(cert));
+ }
+ }
+
+ // The last cert shall be considered the root.
+ bssl::UniquePtr<X509> root = std::move(intermediates.back());
+ intermediates.pop_back();
+
+ std::vector<X509 *> intermediate_ptrs = {};
+ for (const auto &intermediate : intermediates) {
+ intermediate_ptrs.push_back(intermediate.get());
+ }
+ return Verify(leaf.get(), {root.get()}, intermediate_ptrs, {}, /*flags=*/0);
+}
+
+TEST(X509Test, PathLenNormalUnconstrained) {
+ EXPECT_EQ(X509_V_OK,
+ VerifyChain({{/*self_issued=*/false, /*pathlen=*/std::nullopt},
+ {/*self_issued=*/false, /*pathlen=*/std::nullopt},
+ {/*self_issued=*/false, /*pathlen=*/std::nullopt},
+ {/*self_issued=*/false, /*pathlen=*/std::nullopt},
+ {/*self_issued=*/true, /*pathlen=*/std::nullopt}}));
+}
+
+TEST(X509Test, PathLenNormal) {
+ EXPECT_EQ(X509_V_OK, VerifyChain({{/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/1},
+ {/*self_issued=*/false, /*pathlen=*/2},
+ {/*self_issued=*/true, /*pathlen=*/3}}));
+ EXPECT_EQ(X509_V_ERR_PATH_LENGTH_EXCEEDED,
+ VerifyChain({{/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/2},
+ {/*self_issued=*/true, /*pathlen=*/3}}));
+ EXPECT_EQ(X509_V_ERR_PATH_LENGTH_EXCEEDED,
+ VerifyChain({{/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/1},
+ {/*self_issued=*/false, /*pathlen=*/1},
+ {/*self_issued=*/true, /*pathlen=*/3}}));
+ EXPECT_EQ(X509_V_OK, // Path length on trust anchor is ignored.
+ VerifyChain({{/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/1},
+ {/*self_issued=*/false, /*pathlen=*/2},
+ {/*self_issued=*/true, /*pathlen=*/2}}));
+}
+
+TEST(X509Test, PathLenSelfIssuedNotCountedButStillVerified) {
+ EXPECT_EQ(X509_V_OK, VerifyChain({{/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/true, /*pathlen=*/1},
+ {/*self_issued=*/false, /*pathlen=*/1},
+ {/*self_issued=*/true, /*pathlen=*/2}}));
+ EXPECT_EQ(X509_V_ERR_PATH_LENGTH_EXCEEDED,
+ VerifyChain({{/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/true, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/1},
+ {/*self_issued=*/true, /*pathlen=*/2}}));
+ EXPECT_EQ(X509_V_ERR_PATH_LENGTH_EXCEEDED,
+ VerifyChain({{/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/true, /*pathlen=*/1},
+ {/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/true, /*pathlen=*/2}}));
+ EXPECT_EQ(X509_V_OK, // Path length on trust anchor is ignored.
+ VerifyChain({{/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/false, /*pathlen=*/0},
+ {/*self_issued=*/true, /*pathlen=*/1},
+ {/*self_issued=*/false, /*pathlen=*/1},
+ {/*self_issued=*/true, /*pathlen=*/1}}));
+}
+
} // namespace
BSSL_NAMESPACE_END
diff --git a/crypto/x509/x509_vfy.cc b/crypto/x509/x509_vfy.cc
index 5405038..16625ba 100644
--- a/crypto/x509/x509_vfy.cc
+++ b/crypto/x509/x509_vfy.cc
@@ -499,9 +499,16 @@
return 0;
}
}
- // Check pathlen if not self issued
- if (i > 1 && !(x->ex_flags & EXFLAG_SI) && x->ex_pathlen != -1 &&
- plen > x->ex_pathlen + 1) {
+ // Check path length constraints. See steps (l) and (m) of RFC 5280,
+ // section 6.1.4. Note the spec is structured differently from this
+ // logic. Section 6.1.4 runs from root to leaf and does not run on
+ // the leaf. `plen` counts the number of times step (l) would have
+ // run. The constraint is violated if some `x->ex_pathlen`, read in
+ // step (m), is too low to be decremented `plen` times.
+ //
+ // Note that path lengths of self-issued certificates still have to be
+ // considered - they are just not counted as part of the path length!
+ if (i > 1 && x->ex_pathlen != -1 && plen > x->ex_pathlen + 1) {
ctx->error = X509_V_ERR_PATH_LENGTH_EXCEEDED;
ctx->error_depth = i;
ctx->current_cert = x;
@@ -509,8 +516,11 @@
return 0;
}
}
- // Increment path length if not self issued
- if (!(x->ex_flags & EXFLAG_SI)) {
+ // Increment path length if not self issued. As only self-issued
+ // _intermediates_ are skipped in (l) of RFC 5280 (simply because it
+ // operates on certificate chain _edges_), always increment for the first
+ // (the leaf) in the chain.
+ if (i == 0 || !(x->ex_flags & EXFLAG_SI)) {
plen++;
}
}
