Skip to content

Commit

Permalink
[fips-2024-09-27] Align X509 PARTIAL_CHAIN behavior with 1.1.1 (#1929)
Browse files Browse the repository at this point in the history
Some consumers noticed that a behavior difference between AWS-LC and
OpenSSL when the trust store contains certificates that are issued by
other certificates that are also in the trust store. A common example of
this would be the trust store containing both the intermediate and the
root for the cert chain (`leaf -> intermediate -> root`). The default
settings of AWS-LC and OpenSSL require `root` to be self signed for the
chain to be verified.

Many TLS implementations set the `X509_V_FLAG_PARTIAL_CHAIN` flag
however, which allows non self signed certificates to be trusted in the
trust store. When `X509_V_FLAG_PARTIAL_CHAIN` is set, OpenSSL 1.1.1 will
only verify leaf and intermediate, since intermediate is a trusted
certificate. However, AWS-LC will continue building a certificate chain
and include root within the chain of trust. This causes a behavioral
difference with `X509_STORE_CTX_get0_chain`, where AWS-LC will return
all 3 certificates (`leaf -> intermediate -> root`) and OpenSSL 1.1.1
will only return the first two (`leaf -> intermediate `).

Our upstream forked a bit before OpenSSL 1.0.2, so we don't have the new
behavior. This described behavioral difference was introduced in
openssl/openssl@d9b8b89 (along with many others), but the commit
introduces too many backwards incompatible changes for us to take as a
whole.
This subtle difference was due to [OpenSSL 1.1.1 continuously checking
for trust while the chain's being established. The search for the next
valid cert breaks early as soon as a valid chain has been built. Our
current behavior builds the chain with all possible certs first and only
breaks the loop if the final cert in the chain is self-signed. We can
inherit this part of 1.1.1's new behavior to fix this issue.

### Call-outs:
I don't believe this really changes our X509 chain building or
verification by much. We're only adding an additional check for trust
while the chain is being established and the final chain still needs to
go through the same building/verification process that exists in AWS-LC
today.

### Testing:
Specific test for new behavior in `X509_V_FLAG_PARTIAL_CHAIN`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.

(cherry picked from commit 9fbfa70)

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
  • Loading branch information
samuel40791765 authored Oct 22, 2024
1 parent 343968f commit 3d72a6c
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 0 deletions.
46 changes: 46 additions & 0 deletions crypto/x509/x509_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1701,6 +1701,52 @@ TEST(X509Test, TestVerify) {
}
}

TEST(X509Test, PartialChain) {
bssl::UniquePtr<X509> root(CertFromPEM(kRootCAPEM));
bssl::UniquePtr<X509> intermediate(CertFromPEM(kIntermediatePEM));
bssl::UniquePtr<X509> leaf(CertFromPEM(kLeafPEM));
ASSERT_TRUE(root);
ASSERT_TRUE(intermediate);
ASSERT_TRUE(leaf);

// We're intentionally placing the intermediate cert in the trust store here.
// Many TLS implementations set |X509_V_FLAG_PARTIAL_CHAIN|, which allows
// non-self-signed certificates in the trust store to be trusted.
// See https://github.com/openssl/openssl/issues/7871.
bssl::UniquePtr<STACK_OF(X509)> intermediates_stack(CertsToStack({}));
bssl::UniquePtr<STACK_OF(X509)> roots_stack(
CertsToStack({intermediate.get(), root.get()}));

for (bool partial_chain : {true, false}) {
SCOPED_TRACE(partial_chain);
bssl::UniquePtr<X509_STORE_CTX> ctx(X509_STORE_CTX_new());
bssl::UniquePtr<X509_STORE> store(X509_STORE_new());
ASSERT_TRUE(ctx);
ASSERT_TRUE(store);

ASSERT_TRUE(X509_STORE_CTX_init(ctx.get(), store.get(), leaf.get(),
intermediates_stack.get()));
X509_STORE_CTX_set0_trusted_stack(ctx.get(), roots_stack.get());

X509_VERIFY_PARAM *param = X509_STORE_CTX_get0_param(ctx.get());
time_t current_time = time(nullptr);
X509_VERIFY_PARAM_set_time_posix(param, current_time);

if (partial_chain) {
X509_VERIFY_PARAM_set_flags(param, X509_V_FLAG_PARTIAL_CHAIN);
}

EXPECT_EQ(X509_verify_cert(ctx.get()), 1);

STACK_OF(X509) *chain = X509_STORE_CTX_get0_chain(ctx.get());
ASSERT_TRUE(chain);

// |root| will be included in the chain if |X509_V_FLAG_PARTIAL_CHAIN| is
// not set.
EXPECT_EQ(sk_X509_num(chain), partial_chain ? 2u : 3u);
}
}

#if defined(OPENSSL_THREADS)
// Verifying the same |X509| objects on two threads should be safe.
TEST(X509Test, VerifyThreads) {
Expand Down
14 changes: 14 additions & 0 deletions crypto/x509/x509_vfy.c
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,20 @@ int X509_verify_cert(X509_STORE_CTX *ctx) {
ok = 0;
goto end;
}

// OpenSSL 1.1.1 continuously re-checks for trust and breaks the loop
// as soon as trust has been established. 1.0.2 builds the chain with all
// possible certs first and only checks for trust if the final cert in
// the chain is self-signed.
// This caused additional unanticipated certs to be in the established
// certificate chain, particularly when |X509_V_FLAG_PARTIAL_CHAIN| was
// set. We try checking continuously for trust here for better 1.1.1
// compatibility.
trust = check_trust(ctx);
if (trust == X509_TRUST_TRUSTED || trust == X509_TRUST_REJECTED) {
break;
}

num++;
}

Expand Down

0 comments on commit 3d72a6c

Please sign in to comment.