Skip to content

Commit

Permalink
Fix SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR behavior (#1620)
Browse files Browse the repository at this point in the history
### Description of changes: 
This was discovered when taking google/boringssl@5b3dc49 during
 the upstream merge.`ERR_clear_error` is being called more eagerly
with the new change, which led us to discover that
`SSLTest.BuildCertChain` was actually testing against an error code
propagated onto the stack by the previous call to
`SSL_CTX_build_cert_chain`.

Upon further examination, we weren't propagating an error when
calling `SSL_CTX_build_cert_chain` with
`SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR` . The correct
behavior should be to push an error onto the stack regardless.

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 committed Jun 4, 2024
1 parent e44fc2c commit 2431353
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 6 deletions.
2 changes: 2 additions & 0 deletions ssl/ssl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5217,12 +5217,14 @@ TEST(SSLTest, BuildCertChain) {

// Verification will fail because there is no valid root cert available.
EXPECT_FALSE(SSL_CTX_build_cert_chain(ctx.get(), 0));
ERR_clear_error();

// Should return 2 when |SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR| is set.
EXPECT_EQ(
SSL_CTX_build_cert_chain(ctx.get(), SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR),
2);
EXPECT_TRUE(ExpectSingleError(ERR_LIB_SSL, SSL_R_CERTIFICATE_VERIFY_FAILED));
ERR_clear_error();

// Should return 2, but with no error on the stack when
// |SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR| and |SSL_BUILD_CHAIN_FLAG_CLEAR_ERROR|
Expand Down
13 changes: 7 additions & 6 deletions ssl/ssl_x509.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1061,12 +1061,13 @@ static int ssl_build_cert_chain(CERT *cert, X509_STORE *cert_store, int flags) {

bool ignore_error = false;
if (X509_verify_cert(store_ctx.get()) <= 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_CERTIFICATE_VERIFY_FAILED);
ERR_add_error_data(2, "Verify error:",
X509_verify_cert_error_string(
X509_STORE_CTX_get_error(store_ctx.get())));

// Fail if |SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR| is not set.
if(!is_flag_set(flags, SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR)) {
OPENSSL_PUT_ERROR(SSL, SSL_R_CERTIFICATE_VERIFY_FAILED);
ERR_add_error_data(2, "Verify error:",
X509_verify_cert_error_string(
X509_STORE_CTX_get_error(store_ctx.get())));
if (!is_flag_set(flags, SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR)) {
return 0;
}

Expand Down Expand Up @@ -1098,7 +1099,7 @@ static int ssl_build_cert_chain(CERT *cert, X509_STORE *cert_store, int flags) {
// Anything that has passed successfully up to here is valid.
// 2 is used to indicate a verification error has happened, but was ignored
// because |SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR| was set.
if(ignore_error) {
if (ignore_error) {
return 2;
}
return 1;
Expand Down

0 comments on commit 2431353

Please sign in to comment.