From: Christos Tsantilas Date: Wed, 21 Mar 2012 17:06:32 +0000 (+0200) Subject: certificateMatchesProperties segfault X-Git-Tag: BumpSslServerFirst.take08~23 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f4e4d4d6f1540163acf52590868f1dc77b1d3410;p=thirdparty%2Fsquid.git certificateMatchesProperties segfault It is reported at least one case where squid crashed with segfault because the signing certificate was NULL. This patch: - Add assertion checks inside buildSslCertGenerationParams and Ssl::certificateMatchesProperties functions (a)to avoid segfaults - In the case the signing certificate is not given in http_port configuration or the given certificate filename was not valid, squid does not start. - Creates the http_port_list::configureSslServerContext method and move here the cache_cf.cc code which was responsible to initialize ssl contexts and sslBump feature. --- diff --git a/src/ProtoPort.cc b/src/ProtoPort.cc index 9fba353226..a520d9b568 100644 --- a/src/ProtoPort.cc +++ b/src/ProtoPort.cc @@ -4,6 +4,9 @@ #if HAVE_LIMITS #include #endif +#if USE_SSL +#include "ssl/support.h" +#endif http_port_list::http_port_list(const char *aProtocol) #if USE_SSL @@ -37,3 +40,41 @@ http_port_list::~http_port_list() safe_free(sslContextSessionId); #endif } + +#if USE_SSL +void http_port_list::configureSslServerContext() +{ + staticSslContext.reset( + sslCreateServerContext(cert, key, + version, cipher, options, sslflags, clientca, + cafile, capath, crlfile, dhfile, + sslContextSessionId)); + + if (!staticSslContext) { + char buf[128]; + fatalf("%s_port %s initialization error", protocol, s.ToURL(buf, sizeof(buf))); + } + + if (!sslBump) + return; + + if (cert) + Ssl::readCertChainAndPrivateKeyFromFiles(signingCert, signPkey, certsToChain, cert, key); + + if (!signingCert) { + char buf[128]; + fatalf("No valid signing SSL certificate configured for %s_port %s", protocol, s.ToURL(buf, sizeof(buf))); + } + + if (!signPkey) + debugs(3, DBG_IMPORTANT, "No SSL private key configured for " << protocol << "_port " << s); + + Ssl::generateUntrustedCert(untrustedSigningCert, untrustedSignPkey, + signingCert, signPkey); + + if (!untrustedSigningCert) { + char buf[128]; + fatalf("Unable to generate signing SSL certificate for untrusted sites for %s_port %s", protocol, s.ToURL(buf, sizeof(buf))); + } +} +#endif diff --git a/src/ProtoPort.h b/src/ProtoPort.h index 30405b43e9..6e3cad24b1 100644 --- a/src/ProtoPort.h +++ b/src/ProtoPort.h @@ -15,6 +15,9 @@ struct http_port_list { http_port_list(const char *aProtocol); ~http_port_list(); +#if USE_SSL + void configureSslServerContext(); +#endif http_port_list *next; diff --git a/src/cache_cf.cc b/src/cache_cf.cc index cf687ed0c5..49f09077d8 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -906,24 +906,11 @@ configDoConfigure(void) http_port_list *s; for (s = Config.Sockaddr.http; s != NULL; s = (http_port_list *) s->next) { - if (!s->cert) + if (!s->sslBump) continue; debugs(3, 1, "Initializing http_port " << s->s << " SSL context"); - - s->staticSslContext.reset( - sslCreateServerContext(s->cert, s->key, - s->version, s->cipher, s->options, s->sslflags, s->clientca, - s->cafile, s->capath, s->crlfile, s->dhfile, - s->sslContextSessionId)); - - Ssl::readCertChainAndPrivateKeyFromFiles(s->signingCert, s->signPkey, s->certsToChain, s->cert, s->key); - - if (!s->signPkey) - debugs(3, DBG_IMPORTANT, "No SSL private key configured for http_port " << s->s); - - Ssl::generateUntrustedCert(s->untrustedSigningCert, s->untrustedSignPkey, - s->signingCert, s->signPkey); + s->configureSslServerContext(); } } @@ -933,21 +920,7 @@ configDoConfigure(void) for (s = Config.Sockaddr.https; s != NULL; s = s->next) { debugs(3, 1, "Initializing https_port " << s->s << " SSL context"); - - s->staticSslContext.reset( - sslCreateServerContext(s->cert, s->key, - s->version, s->cipher, s->options, s->sslflags, s->clientca, - s->cafile, s->capath, s->crlfile, s->dhfile, - s->sslContextSessionId)); - - if (s->cert && s->sslBump) { - Ssl::readCertChainAndPrivateKeyFromFiles(s->signingCert, s->signPkey, s->certsToChain, s->cert, s->key); - if (!s->signPkey) - debugs(3, DBG_IMPORTANT, "No SSL private key configured for https_port " << s->s); - - Ssl::generateUntrustedCert(s->untrustedSigningCert, s->untrustedSignPkey, - s->signingCert, s->signPkey); - } + s->configureSslServerContext(); } } diff --git a/src/client_side.cc b/src/client_side.cc index f29464ec1a..9381f09b5c 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -3741,12 +3741,13 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer assert(certProperties.signAlgorithm != Ssl::algSignEnd); if (certProperties.signAlgorithm == Ssl::algSignUntrusted) { + assert(port->untrustedSigningCert.get()); certProperties.signWithX509.resetAndLock(port->untrustedSigningCert.get()); certProperties.signWithPkey.resetAndLock(port->untrustedSignPkey.get()); } else { - if (port->signingCert.get()) - certProperties.signWithX509.resetAndLock(port->signingCert.get()); + assert(port->signingCert.get()); + certProperties.signWithX509.resetAndLock(port->signingCert.get()); if (port->signPkey.get()) certProperties.signWithPkey.resetAndLock(port->signPkey.get()); diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index ec4b14d6f6..55fcabe6f4 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -554,6 +554,7 @@ bool Ssl::certificateMatchesProperties(X509 *cert, CertificateProperties const & // For non self-signed certificates we have to check if the signing certificate changed if (properties.signAlgorithm != Ssl::algSignSelf) { + assert(properties.signWithX509.get()); if (X509_check_issued(properties.signWithX509.get(), cert) != X509_V_OK) return false; } diff --git a/src/ssl/support.cc b/src/ssl/support.cc index ec7f9af58b..c40ad61b9f 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -707,6 +707,11 @@ sslCreateServerContext(const char *certfile, const char *keyfile, int version, c if (!CAfile) CAfile = clientCA; + if (!certfile) { + debugs(83, DBG_CRITICAL, "ERROR: No certificate file"); + return NULL; + } + switch (version) { case 2: @@ -741,8 +746,8 @@ sslCreateServerContext(const char *certfile, const char *keyfile, int version, c if (sslContext == NULL) { ssl_error = ERR_get_error(); - fatalf("Failed to allocate SSL context: %s\n", - ERR_error_string(ssl_error, NULL)); + debugs(83, DBG_CRITICAL, "ERROR: Failed to allocate SSL context: " << ERR_error_string(ssl_error, NULL)); + return NULL; } SSL_CTX_set_options(sslContext, ssl_parse_options(options)); @@ -766,8 +771,9 @@ sslCreateServerContext(const char *certfile, const char *keyfile, int version, c if (!SSL_CTX_set_cipher_list(sslContext, cipher)) { ssl_error = ERR_get_error(); - fatalf("Failed to set SSL cipher suite '%s': %s\n", - cipher, ERR_error_string(ssl_error, NULL)); + debugs(83, DBG_CRITICAL, "ERROR: Failed to set SSL cipher suite '" << cipher << "': " << ERR_error_string(ssl_error, NULL)); + SSL_CTX_free(sslContext); + return NULL; } }