X509_VERIFY_PARAM_inherit/_set1: refuse if either params are poisoned.

Before, these functions would unconditionally copy the src's poison flag
to the dst, potentially resulting in an unpoisoned context even if the
reason for poisoning (e.g. a field with invalid value) remains.

After this change, copying always fails if any of the two params is
poisoned.

The most relevant code path for this is when `X509_STORE_CTX_init` is
called with a poisoned `X509_STORE`; it happily copied the poisoned
context and then cleared the poison flag while inheriting from defaults.

Yes, instead the calls in `X509_STORE_CTX_init` could be reversed to
first `inherit` from the defaults and then to `set1` from the user
provided context, which then would yield the correct copied poison flag;
however it seems prudent to instead harden the public APIs, as there
could be more issues of this kind.

Not considering a vulnerability as the poison flag can only ever be set
on a call to us if a caller used an API wrong by ignoring its return
value. Of course, the whole purpose of the flag is to detect and fail
such callers so no damage happens.

Change-Id: Iaed97dfa21863c882a0d46b1b8439ec06a6a6964
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95527
Commit-Queue: Rudolf Polzer <rpolzer@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com>

crypto/x509/x509_test.cc

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index d3ab671..cc79b09 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -8434,6 +8434,36 @@
     // The new value is used.
     EXPECT_EQ(X509_VERIFY_PARAM_get_depth(dest.get()), 10);
   }
+
+  // |X509_VERIFY_PARAM_inherit| and |X509_VERIFY_PARAM_set1| must fail if the
+  // source parameter is poisoned.
+  {
+    UniquePtr<X509_VERIFY_PARAM> dest(X509_VERIFY_PARAM_new());
+    ASSERT_TRUE(dest);
+    UniquePtr<X509_VERIFY_PARAM> src(X509_VERIFY_PARAM_new());
+    ASSERT_TRUE(src);
+
+    // Poison the source parameter (using an embedded NUL in hostname).
+    ASSERT_FALSE(X509_VERIFY_PARAM_set1_host(src.get(), "a", 2));
+
+    EXPECT_FALSE(X509_VERIFY_PARAM_inherit(dest.get(), src.get()));
+    EXPECT_FALSE(X509_VERIFY_PARAM_set1(dest.get(), src.get()));
+  }
+
+  // |X509_VERIFY_PARAM_inherit| and |X509_VERIFY_PARAM_set1| must fail if the
+  // destination parameter is poisoned.
+  {
+    UniquePtr<X509_VERIFY_PARAM> dest(X509_VERIFY_PARAM_new());
+    ASSERT_TRUE(dest);
+    UniquePtr<X509_VERIFY_PARAM> src(X509_VERIFY_PARAM_new());
+    ASSERT_TRUE(src);
+
+    // Poison the destination parameter (using an embedded NUL in hostname).
+    ASSERT_FALSE(X509_VERIFY_PARAM_set1_host(dest.get(), "a", 2));
+
+    EXPECT_FALSE(X509_VERIFY_PARAM_inherit(dest.get(), src.get()));
+    EXPECT_FALSE(X509_VERIFY_PARAM_set1(dest.get(), src.get()));
+  }
 }
 
 TEST(X509Test, PublicKeyCache) {

crypto/x509/x509_vpm.cc

diff --git a/crypto/x509/x509_vpm.cc b/crypto/x509/x509_vpm.cc
index a0f8a71..7507981 100644
--- a/crypto/x509/x509_vpm.cc
+++ b/crypto/x509/x509_vpm.cc
@@ -117,16 +117,21 @@
   }
 }
 
-// x509_verify_param_copy copies fields from |src| to |dest|. If both |src| and
+// x509_verify_param_merge merges fields from |src| to |dest|. If both |src| and
 // |dest| have some field set, |prefer_src| determines whether |src| or |dest|'s
 // version is used.
-static int x509_verify_param_copy(X509_VERIFY_PARAM *dest,
-                                  const X509_VERIFY_PARAM *src,
-                                  bool prefer_src) {
+static int x509_verify_param_merge(X509_VERIFY_PARAM *dest,
+                                   const X509_VERIFY_PARAM *src,
+                                   bool prefer_src) {
   if (src == nullptr) {
     return 1;
   }
 
+  if (src->poison || dest->poison) {
+    OPENSSL_PUT_ERROR(X509, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
+    return 0;
+  }
+
   copy_int_param(&dest->purpose, &src->purpose, /*default_val=*/0, prefer_src);
   copy_int_param(&dest->trust, &src->trust, /*default_val=*/0, prefer_src);
   copy_int_param(&dest->depth, &src->depth, /*default_val=*/-1, prefer_src);
@@ -175,7 +180,6 @@
     }
   }
 
-  dest->poison = src->poison;
   return 1;
 }
 
@@ -183,14 +187,14 @@
                               const X509_VERIFY_PARAM *src) {
   // Prefer the destination. That is, this function only changes unset
   // parameters in |dest|.
-  return x509_verify_param_copy(dest, src, /*prefer_src=*/false);
+  return x509_verify_param_merge(dest, src, /*prefer_src=*/false);
 }
 
 int X509_VERIFY_PARAM_set1(X509_VERIFY_PARAM *to,
                            const X509_VERIFY_PARAM *from) {
   // Prefer the source. That is, values in |to| are only preserved if they were
   // unset in |from|.
-  return x509_verify_param_copy(to, from, /*prefer_src=*/true);
+  return x509_verify_param_merge(to, from, /*prefer_src=*/true);
 }
 
 static int int_x509_param_set1(char **pdest, size_t *pdestlen, const char *src,