Support the OpenSSL “pass zero for strlen” when setting X.509 hostnames.

BoringSSL does not generally support this quirk but, in this case, we
didn't make it a fatal error and it's instead a silent omission of
hostname checking. This doesn't affect Chrome but, in case something is
using BoringSSL and using this trick, this change makes it safe.

BUG=chromium:824799

Change-Id: If417817b997b9faa9963c09dfc95d06a5d445e0b
Reviewed-on: https://boringssl-review.googlesource.com/26724
Commit-Queue: Adam Langley <alangley@gmail.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

crypto/x509/x509_test.cc

@@ -487,7 +487,9 @@ static int Verify(X509 *leaf, const std::vector<X509 *> &roots,
                    const std::vector<X509 *> &intermediates,
                    const std::vector<X509_CRL *> &crls,
                    unsigned long flags,
-                   bool use_additional_untrusted) {
+                   bool use_additional_untrusted,
+                   const char *hostname,
+                   size_t hostname_len) {
   bssl::UniquePtr<STACK_OF(X509)> roots_stack(CertsToStack(roots));
   bssl::UniquePtr<STACK_OF(X509)> intermediates_stack(
       CertsToStack(intermediates));
@@ -526,6 +528,7 @@ static int Verify(X509 *leaf, const std::vector<X509 *> &roots,
   }
   X509_VERIFY_PARAM_set_time(param, 1474934400 /* Sep 27th, 2016 */);
   X509_VERIFY_PARAM_set_depth(param, 16);
+  X509_VERIFY_PARAM_set1_host(param, hostname, hostname_len);
   if (flags) {
     X509_VERIFY_PARAM_set_flags(param, flags);
   }
@@ -543,8 +546,10 @@ static int Verify(X509 *leaf, const std::vector<X509 *> &roots,
                    const std::vector<X509 *> &intermediates,
                    const std::vector<X509_CRL *> &crls,
                    unsigned long flags = 0) {
-  const int r1 = Verify(leaf, roots, intermediates, crls, flags, false);
-  const int r2 = Verify(leaf, roots, intermediates, crls, flags, true);
+  const int r1 =
+      Verify(leaf, roots, intermediates, crls, flags, false, nullptr, 0);
+  const int r2 =
+      Verify(leaf, roots, intermediates, crls, flags, true, nullptr, 0);
 
   if (r1 != r2) {
     fprintf(stderr,
@@ -612,6 +617,19 @@ TEST(X509Test, TestVerify) {
             Verify(forgery.get(),
                    {intermediate_self_signed.get(), root_cross_signed.get()},
                    {leaf_no_key_usage.get(), intermediate.get()}, empty_crls));
+
+  static const char kHostname[] = "example.com";
+  static const char kWrongHostname[] = "example2.com";
+  ASSERT_EQ(X509_V_OK,
+            Verify(leaf.get(), {root.get()}, {intermediate.get()}, empty_crls,
+                   0, false, kHostname, strlen(kHostname)));
+  // The wrong hostname should trigger a hostname error.
+  ASSERT_EQ(X509_V_ERR_HOSTNAME_MISMATCH,
+            Verify(leaf.get(), {root.get()}, {intermediate.get()}, empty_crls,
+                   0, false, kWrongHostname, strlen(kWrongHostname)));
+  // Passing zero, for this API, is supported for compatibility with OpenSSL.
+  ASSERT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {intermediate.get()},
+                              empty_crls, 0, false, kHostname, 0));
 }
 
 TEST(X509Test, TestCRL) {

crypto/x509/x509_vpm.c

@@ -89,6 +89,14 @@ static int int_x509_param_set_hosts(X509_VERIFY_PARAM_ID *id, int mode,
 {
     char *copy;
 
+    // This is an OpenSSL quirk that BoringSSL typically doesn't support.
+    // However, we didn't make this a fatal error at the time, which was a
+    // mistake. Because of that, given the risk that someone could assume the
+    // OpenSSL semantics from BoringSSL, it's supported in this case.
+    if (name != NULL && namelen == 0) {
+        namelen = strlen(name);
+    }
+
     /*
      * Refuse names with embedded NUL bytes.
      * XXX: Do we need to push an error onto the error stack?