commit e1e78130f51288d65be0b43eebbe13385068980f
author Adam Langley <agl@google.com> Tue Jan 31 15:24:31 2017 -0800
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Wed Feb 01 00:24:24 2017 +0000
tree b84d171a9d308a2a172927b3a9d656aab148da88
parent 7cd0a978cc3cbfe771b6fdecdcb83ceaf09cf4a6

Keep a reference to |X509|s appended to a chain.

The recent CRYPTO_BUFFER changes meant that |X509| objects passed to
SSL_CTX_add_extra_chain_cert would be |free|ed immediately. However,
some third-party code (at least serf and curl) continue to use the
|X509| even after handing over ownership.

In order to unblock things, keep the past |X509| around for a while to
paper over the issues with those libraries while we try and upstream
changes.

Change-Id: I832b458af9b265749fed964658c5c34c84d518df
Reviewed-on: https://boringssl-review.googlesource.com/13480
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

ssl/internal.h

diff --git a/ssl/internal.h b/ssl/internal.h
index a408b0f..5364f99 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1245,6 +1245,11 @@
    * a non-owning pointer to the certificate chain. */
   X509 *x509_leaf;
 
+  /* x509_stash contains the last |X509| object append to the chain. This is a
+   * workaround for some third-party code that continue to use an |X509| object
+   * even after passing ownership with an “add0” function. */
+  X509 *x509_stash;
+
   /* key_method, if non-NULL, is a set of callbacks to call for private key
    * operations. */
   const SSL_PRIVATE_KEY_METHOD *key_method;

ssl/ssl_cert.c

diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index ed6ba0d..1ebe4b6 100644
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -227,6 +227,9 @@
   ssl_cert_flush_cached_x509_leaf(cert);
   ssl_cert_flush_cached_x509_chain(cert);
 
+  X509_free(cert->x509_stash);
+  cert->x509_stash = NULL;
+
   sk_CRYPTO_BUFFER_pop_free(cert->chain, CRYPTO_BUFFER_free);
   cert->chain = NULL;
   EVP_PKEY_free(cert->privatekey);
@@ -378,7 +381,8 @@
     return 0;
   }
 
-  X509_free(x509);
+  X509_free(cert->x509_stash);
+  cert->x509_stash = x509;
   ssl_cert_flush_cached_x509_chain(cert);
   return 1;
 }

ssl/ssl_test.cc

diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 22a9b7b..99e9af2 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -3142,6 +3142,21 @@
   return true;
 }
 
+TEST(SSLTest, AddChainCertHack) {
+  // Ensure that we don't accidently break the hack that we have in place to
+  // keep curl and serf happy when they use an |X509| even after transfering
+  // ownership.
+
+  bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
+  ASSERT_TRUE(ctx);
+  X509 *cert = GetTestCertificate().release();
+  ASSERT_TRUE(cert);
+  SSL_CTX_add0_chain_cert(ctx.get(), cert);
+
+  // This should not trigger a use-after-free.
+  X509_cmp(cert, cert);
+}
+
 // TODO(davidben): Convert this file to GTest properly.
 TEST(SSLTest, AllTests) {
   if (!TestCipherRules() ||