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>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index b4cecca..ed4978a 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -487,7 +487,9 @@
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 @@
}
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 @@
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 @@
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) {
diff --git a/crypto/x509/x509_vpm.c b/crypto/x509/x509_vpm.c
index d0f8f79..0b03361 100644
--- a/crypto/x509/x509_vpm.c
+++ b/crypto/x509/x509_vpm.c
@@ -89,6 +89,14 @@
{
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?
