Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

Commit

Permalink
add check for null dereference in SSL serf fetcher
Browse files Browse the repository at this point in the history
SSL_set_tlsext_host_name did an unchecked dereference of context->ssl
which could be null. Check for null and log on this case.
  • Loading branch information
crowell committed Jul 27, 2015
1 parent a49af6b commit b4f4ae9
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 7 deletions.
46 changes: 46 additions & 0 deletions .gitignore
@@ -0,0 +1,46 @@
Makefile
*.mk
*.Makefile
out/*
testing/*
build/android/*
build/mac/*
build/linux/*
build/internal/*
build/win/*
build/ios/*
build/gyp_helper.pyc
build/landmine_utils.pyc
build/temp_gyp/*
gclient.timestamp
third_party/apr/src/*
third_party/aprutil/src/*
third_party/boringssl/*
third_party/chromium/src/base/*
third_party/chromium/src/build/*
third_party/chromium/src/net/base/*
third_party/chromium/src/url/*
third_party/chromium_deps/*
third_party/closure_library/*
third_party/domain_registry_provider/*
third_party/gflags/src/*
third_party/giflib/*
third_party/google-sparsehash/src/*
third_party/httpd/src/*
third_party/httpd24/src/*
third_party/icu/*
third_party/jsoncpp/src/*
third_party/libjpeg_turbo/src/*
third_party/libjpeg_turbo/yasm/*
third_party/libwebp/*
third_party/mod_spdy/*
third_party/modp_b64/*
third_party/optipng/*
third_party/pagespeed*
third_party/protobuf/*
third_party/re2/src/*
third_party/serf/src/*
third_party/zlib/*
tools/*
third_party/domain_registry_provider
third_party/domain_registry_provider/*
7 changes: 6 additions & 1 deletion net/instaweb/system/serf_url_async_fetcher.cc
Expand Up @@ -330,7 +330,12 @@ class SerfFetch : public PoolElement<SerfFetch> {
SSLCertError, SSLCertChainError,
fetch);

serf_ssl_set_hostname(fetch->ssl_context_, fetch->sni_host_);
status = serf_ssl_set_hostname(fetch->ssl_context_, fetch->sni_host_);
if (status != APR_SUCCESS) {
LOG(INFO) << "Unable to set hostname from serf fetcher. Connection "
"setup failed";
return status;
}
*write_bkt = serf_bucket_ssl_encrypt_create(*write_bkt,
fetch->ssl_context_,
fetch->bucket_alloc_);
Expand Down
4 changes: 3 additions & 1 deletion third_party/serf/instaweb_ssl_buckets.c
Expand Up @@ -1212,7 +1212,9 @@ apr_status_t serf_ssl_set_hostname(serf_ssl_context_t *context,
const char * hostname)
{
#ifdef SSL_set_tlsext_host_name
if (SSL_set_tlsext_host_name(context->ssl, hostname) != 1) {
if (!context->ssl) {
return APR_EGENERAL;
} else if (SSL_set_tlsext_host_name(context->ssl, hostname) != 1) {
ERR_clear_error();
}
#endif
Expand Down
54 changes: 49 additions & 5 deletions third_party/serf/serf.diff
Expand Up @@ -267,8 +267,8 @@
+ return serf_request_bucket_request_create_for_host(
+ request, method, uri, body, allocator, NULL);
+}
--- serf/src/buckets/ssl_buckets.c 2012-08-29 13:52:29.443913000 -0400
+++ src/third_party/serf/instaweb_ssl_buckets.c 2013-05-14 09:56:37.292605000 -0400
--- serf/src/buckets/ssl_buckets.c 2012-08-29 13:29:42.000000000 -0400
+++ src/third_party/serf/instaweb_ssl_buckets.c 2015-07-27 10:56:12.000000000 -0400
@@ -34,6 +34,11 @@
* Originally developed by Aaron Bannert and Justin Erenkrantz, eBuilt.
*/
Expand All @@ -281,7 +281,40 @@
#include <apr_pools.h>
#include <apr_network_io.h>
#include <apr_portable.h>
@@ -1160,7 +1165,9 @@
@@ -873,7 +878,9 @@
#if APR_HAS_THREADS
int i, numlocks;
#endif
+#ifndef OPENSSL_IS_BORINGSSL
CRYPTO_malloc_init();
+#endif
ERR_load_crypto_strings();
SSL_load_error_strings();
SSL_library_init();
@@ -971,8 +978,13 @@
else {
int err = ERR_get_error();
ERR_clear_error();
+#ifndef OPENSSL_IS_BORINGSSL
if (ERR_GET_LIB(err) == ERR_LIB_PKCS12 &&
ERR_GET_REASON(err) == PKCS12_R_MAC_VERIFY_FAILURE) {
+#else
+ if (ERR_GET_LIB(err) == ERR_LIB_PKCS8 &&
+ ERR_GET_REASON(err) == PKCS8_R_INCORRECT_PASSWORD) {
+#endif
if (ctx->cert_pw_callback) {
const char *password;

@@ -1102,6 +1114,8 @@
ssl_ctx->allocator = allocator;

ssl_ctx->ctx = SSL_CTX_new(SSLv23_client_method());
+ /* Use the best possible protocol version, but disable the broken SSLv2/3 */
+ SSL_CTX_set_options(ssl_ctx->ctx, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);

SSL_CTX_set_client_cert_cb(ssl_ctx->ctx, ssl_need_client_cert);
ssl_ctx->cached_cert = 0;
@@ -1160,7 +1174,9 @@

/* SSL_free implicitly frees the underlying BIO. */
SSL_free(ssl_ctx->ssl);
Expand All @@ -291,7 +324,18 @@

p = ssl_ctx->pool;

@@ -1209,6 +1216,28 @@
@@ -1193,7 +1209,9 @@
const char * hostname)
{
#ifdef SSL_set_tlsext_host_name
- if (SSL_set_tlsext_host_name(context->ssl, hostname) != 1) {
+ if (!context->ssl) {
+ return APR_EGENERAL;
+ } else if (SSL_set_tlsext_host_name(context->ssl, hostname) != 1) {
ERR_clear_error();
}
#endif
@@ -1209,6 +1227,28 @@
return result ? APR_SUCCESS : APR_EGENERAL;
}

Expand Down Expand Up @@ -320,7 +364,7 @@
apr_status_t serf_ssl_load_cert_file(
serf_ssl_certificate_t **cert,
const char *file_path,
@@ -1527,6 +1556,7 @@
@@ -1527,6 +1567,7 @@

if (!--ctx->ssl_ctx->refcount) {
ssl_free_context(ctx->ssl_ctx);
Expand Down

0 comments on commit b4f4ae9

Please sign in to comment.