Skip to content
Permalink
Browse files

Check that certificates are valid for the domain we're trying to cont…

…act.

This depends on x509_check_host, which was added in openssl 1.0.2.  If we want
to support older versions of openssl we need to parse certs ourself, like svn
does, which is a bunch of moderately tricky security sensitive code that I'd
really rather avoid.  We normally build agaist boringssl, which has this
function, but we also prepare a tarball build that intendes to link against
system openssl.  So anyone using that build process will need to upgrade to
1.0.2.
  • Loading branch information...
jeffkaufman committed Feb 3, 2016
1 parent 5ac28b3 commit 4af5e6565ad3310a05a28c49212c5c87a9a83dec
@@ -82,6 +82,9 @@ apr_status_t serf_ssl_set_certificates_directory(serf_ssl_context_t *ssl_ctx,
const char* path);
apr_status_t serf_ssl_set_certificates_file(serf_ssl_context_t *ssl_ctx,
const char* file);
int serf_ssl_check_host(const serf_ssl_certificate_t *cert,
const char* hostname);

} // extern "C"

namespace net_instaweb {
@@ -237,18 +240,19 @@ int64 SerfFetch::TimeDuration() const {

#if SERF_HTTPS_FETCHING
// static
apr_status_t SerfFetch::SSLCertError(void *data, int failures,
apr_status_t SerfFetch::SSLCertValidate(void *data, int failures,
const serf_ssl_certificate_t *cert) {
return static_cast<SerfFetch*>(data)->HandleSSLCertErrors(failures, 0);
return static_cast<SerfFetch*>(data)->HandleSSLCertValidation(
failures, 0, cert);
}

// static
apr_status_t SerfFetch::SSLCertChainError(
apr_status_t SerfFetch::SSLCertChainValidate(
void *data, int failures, int error_depth,
const serf_ssl_certificate_t * const *certs,
apr_size_t certs_count) {
return static_cast<SerfFetch*>(data)->HandleSSLCertErrors(failures,
error_depth);
return static_cast<SerfFetch*>(data)->HandleSSLCertValidation(
failures, error_depth, NULL);
}
#endif

@@ -293,12 +297,11 @@ apr_status_t SerfFetch::ConnectionSetup(
}
}

serf_ssl_server_cert_callback_set(fetch->ssl_context_, SSLCertError,
fetch);
serf_ssl_server_cert_callback_set(
fetch->ssl_context_, SSLCertValidate, fetch);

serf_ssl_server_cert_chain_callback_set(fetch->ssl_context_,
SSLCertError, SSLCertChainError,
fetch);
serf_ssl_server_cert_chain_callback_set(
fetch->ssl_context_, SSLCertValidate, SSLCertChainValidate, fetch);

status = serf_ssl_set_hostname(fetch->ssl_context_, fetch->sni_host_);
if (status != APR_SUCCESS) {
@@ -382,14 +385,15 @@ bool SerfFetch::IsStatusOk(apr_status_t status) {
}

#if SERF_HTTPS_FETCHING
apr_status_t SerfFetch::HandleSSLCertErrors(int errors, int failure_depth) {
apr_status_t SerfFetch::HandleSSLCertValidation(
int errors, int failure_depth, const serf_ssl_certificate_t *cert) {
// TODO(jmarantz): is there value in logging the errors and failure_depth
// formals here?

// Note that HandleSSLCertErrors can be called multiple times for
// a single request. As far as I can tell, there is value in
// recording only one of these. For now, I have set up the logic
// so only the last error will be printed lazilly, in ReadHeaders.
// Note that HandleSSLCertValidation can be called multiple times for a single
// request. As far as I can tell, there is value in recording only one of
// these. For now, I have set up the logic so only the last error will be
// printed lazilly, in ReadHeaders.
if (((errors & SERF_SSL_CERT_SELF_SIGNED) != 0) &&
!fetcher_->allow_self_signed()) {
ssl_error_message_ = "SSL certificate is self-signed";
@@ -406,9 +410,35 @@ apr_status_t SerfFetch::HandleSSLCertErrors(int errors, int failure_depth) {
ssl_error_message_ = "SSL certificate has an unknown error";
}

if (ssl_error_message_ == NULL && async_fetch_ != NULL) {
if (// If cert is null that means we're being called via SSLCertChainError.
// We only need to check the host name matches when being called via
// SSLCertError, in which case cert won't be null.
cert != NULL &&
// No point in checking the host if we're allowing self-signed or a made
// up CA, since people can forge whatever they want and often don't
// bother to make the name match.
!fetcher_->allow_self_signed() &&
!fetcher_->allow_unknown_certificate_authority()) {

DCHECK(serf_ssl_cert_depth(cert) == 0) <<
"Serf should be filtering out intermediate certs before hitting us.";

// You would think we could do whatever serf_get.c does, but it turns
// out that does no checking at all. There's x509_check_host, added in
// 1.0.2, but when svn uses serf it rolls its own check because it wants
// to support older versions. We generally build with boringssl, which
// forked from 1.0.2 and has always had this function, but when we build
// with openssl we now require 1.0.2.
if (serf_ssl_check_host(cert, sni_host_) != 1) {
ssl_error_message_ = "Failed to match host.";
}
}
}

// Immediately call the fetch callback on a cert error. Note that
// HandleSSLCertErrors is called multiple times when there is an error,
// so check async_fetch before CallCallback.
// HandleSSLCertValidation is called multiple times when there is an error, so
// check async_fetch before CallCallback.
if ((ssl_error_message_ != NULL) && (async_fetch_ != NULL)) {
fetcher_->cert_errors_->Add(1);
CallCallback(false); // sets async_fetch_ to null.
@@ -330,10 +330,10 @@ class SerfFetch : public PoolElement<SerfFetch> {
// Note this must be ifdef'd because calling serf_bucket_ssl_decrypt_create
// requires ssl_buckets.c in the link. ssl_buckets.c requires openssl.
#if SERF_HTTPS_FETCHING
static apr_status_t SSLCertError(void *data, int failures,
static apr_status_t SSLCertValidate(void *data, int failures,
const serf_ssl_certificate_t *cert);

static apr_status_t SSLCertChainError(
static apr_status_t SSLCertChainValidate(
void *data, int failures, int error_depth,
const serf_ssl_certificate_t * const *certs,
apr_size_t certs_count);
@@ -363,9 +363,13 @@ class SerfFetch : public PoolElement<SerfFetch> {
// non-null for errors as a signal to ReadHeaders that we should not let
// any output thorugh.
//
// Interpretation of two of the error conditions is configuraable:
// Interpretation of two of the error conditions is configurable:
// 'allow_unknown_certificate_authority' and 'allow_self_signed'.
apr_status_t HandleSSLCertErrors(int errors, int failure_depth);
//
// If there is a cert that should be checked for a hostname match, that should
// go in cert. Otherwise cert should be null.
apr_status_t HandleSSLCertValidation(
int errors, int failure_depth, const serf_ssl_certificate_t *cert);
#endif

apr_status_t HandleResponse(serf_bucket_t* response);
@@ -652,6 +652,13 @@ TEST_F(SerfUrlAsyncFetcherTest, TestHttpsWithExplicitHost) {
ExpectHttpsSucceeds(index);
}

TEST_F(SerfUrlAsyncFetcherTest, TestHttpsFailsWithIncorrectHost) {
serf_url_async_fetcher_->SetHttpsOptions("enable");
int index = AddTestUrl("https://www.google.com", "");
request_headers(index)->Add(HttpAttributes::kHost, "www.example.com");
TestHttpsFails(index, index);
}

TEST_F(SerfUrlAsyncFetcherTest, TestHttpsWithExplicitHostPort) {
// Similar to above, but just throw in an explicit port number;
// if it doesn't get properly dropped from the SNI Apache will
@@ -1680,6 +1680,21 @@ int serf_ssl_cert_depth(const serf_ssl_certificate_t *cert)
return cert->depth;
}

/*
* PageSpeed addition to allow verification of certificate hostnames.
*
* Hostname must be null-terminated.
* For return values, see X509_check_host.
*/
int serf_ssl_check_host(const serf_ssl_certificate_t *cert,
const char* hostname)
{
return X509_check_host(cert->ssl_cert,
hostname,
strlen(hostname),
0 /* we don't need to set any flags */,
NULL /* we don't need the SAN or CN extracted*/);
}

apr_hash_t *serf_ssl_cert_issuer(
const serf_ssl_certificate_t *cert,

0 comments on commit 4af5e65

Please sign in to comment.
You can’t perform that action at this time.