commit c8006be227207ca7416d45924cf778de4a48e487
author Adam Langley <agl@google.com> Wed Dec 07 16:45:24 2016 -0800
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Thu Dec 08 16:20:49 2016 +0000
tree c4a040c18035f0d153a488fab6ecb7091ead08ab
parent 9d125dcdec52e8968c377c51cfbb4f2d4ad4af7b

Fix X509_parse_from_buffer when failing to parse.

d2i_X509 will free an existing |X509*| on parse failure. Thus
|X509_parse_from_buffer| would double-free the result on error.

Change-Id: If2bca2f1e1895bc426079f6ade4b82008707888d
Reviewed-on: https://boringssl-review.googlesource.com/12635
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>

crypto/x509/x509_test.cc

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 3e2d3f7..0c25754 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -940,6 +940,50 @@
   return true;
 }
 
+static bool TestFailedParseFromBuffer() {
+  static const uint8_t kNonsense[] = {1, 2, 3, 4, 5};
+
+  bssl::UniquePtr<CRYPTO_BUFFER> buf(
+      CRYPTO_BUFFER_new(kNonsense, sizeof(kNonsense), nullptr));
+  if (!buf) {
+    return false;
+  }
+
+  bssl::UniquePtr<X509> cert(X509_parse_from_buffer(buf.get()));
+  if (cert) {
+    fprintf(stderr, "Nonsense somehow parsed.\n");
+    return false;
+  }
+  ERR_clear_error();
+
+  // Test a buffer with trailing data.
+  size_t data_len;
+  bssl::UniquePtr<uint8_t> data;
+  if (!PEMToDER(&data, &data_len, kRootCAPEM)) {
+    return false;
+  }
+
+  std::unique_ptr<uint8_t[]> data_with_trailing_byte(new uint8_t[data_len + 1]);
+  memcpy(data_with_trailing_byte.get(), data.get(), data_len);
+  data_with_trailing_byte[data_len] = 0;
+
+  bssl::UniquePtr<CRYPTO_BUFFER> buf_with_trailing_byte(
+      CRYPTO_BUFFER_new(data_with_trailing_byte.get(), data_len + 1, nullptr));
+  if (!buf_with_trailing_byte) {
+    return false;
+  }
+
+  bssl::UniquePtr<X509> root(
+      X509_parse_from_buffer(buf_with_trailing_byte.get()));
+  if (root) {
+    fprintf(stderr, "Parsed buffer with trailing byte.\n");
+    return false;
+  }
+  ERR_clear_error();
+
+  return true;
+}
+
 int main() {
   CRYPTO_library_init();
 
@@ -951,7 +995,8 @@
       !TestFromBuffer() ||
       !TestFromBufferTrailingData() ||
       !TestFromBufferModified() ||
-      !TestFromBufferReused()) {
+      !TestFromBufferReused() ||
+      !TestFailedParseFromBuffer()) {
     return 1;
   }
 

crypto/x509/x_x509.c

diff --git a/crypto/x509/x_x509.c b/crypto/x509/x_x509.c
index b2fb845..d3cd5b0 100644
--- a/crypto/x509/x_x509.c
+++ b/crypto/x509/x_x509.c
@@ -162,8 +162,8 @@
   X509 *x509p = x509;
   X509 *ret = d2i_X509(&x509p, &inp, CRYPTO_BUFFER_len(buf));
   if (ret == NULL ||
-      (inp - CRYPTO_BUFFER_data(buf)) != (ptrdiff_t) CRYPTO_BUFFER_len(buf)) {
-    X509_free(x509);
+      inp - CRYPTO_BUFFER_data(buf) != (ptrdiff_t)CRYPTO_BUFFER_len(buf)) {
+    X509_free(x509p);
     return NULL;
   }
   assert(x509p == x509);