From 38450a50701177057bd5389c6b8e255fa2f902bf Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Wed, 18 Jul 2012 18:19:04 +0300 Subject: [PATCH] Handle Amos comments on bump-ssl-server-first patch - Use "IF" instead of "IFDEF" in cf.data.pre. The IFDEF already used in the IDEF form. - Add an assertion in setServerBump to assure that even if this method reused in a client request we will not try to change the existing bump mode - Polish the code in doCallouts method: wrap if(!calloutContext->error) { ... } around the whole section of callouts. - The detailCode parameter in ErrorState::detailError(int detailCode) shadows the ErrorState::detailCode parameter. - src/url.cc: Reverting change in urlParseFinish - Allow old "ssl_bump allow/deny acl ..." syntax converting: * "ssl_bump allow" to "ssl_bump client-first" * "ssl-bump deny" to "ssl_bump none" Prints warning messages to urge users update their configuration. Does not allow to mix old and new configurations. - Warnings and comment fixes --- src/cache_cf.cc | 92 ++++++++++++++++++++++++++++++++------ src/cf.data.pre | 10 +++-- src/cf_gen.cc | 8 ++-- src/client_side.cc | 2 +- src/client_side.h | 7 ++- src/client_side_request.cc | 90 +++++++++++++++++++------------------ src/errorpage.cc | 6 --- src/errorpage.h | 2 +- src/url.cc | 1 + 9 files changed, 145 insertions(+), 73 deletions(-) diff --git a/src/cache_cf.cc b/src/cache_cf.cc index 893bd6b52d..0e1c9242d2 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -55,6 +55,7 @@ #include "auth/Config.h" #include "auth/Scheme.h" #endif +#include "base/RunnersRegistry.h" #include "ConfigParser.h" #include "CpuAffinityMap.h" #include "DiskIO/DiskIOModule.h" @@ -3808,14 +3809,14 @@ parsePortCfg(AnyP::PortCfg ** head, const char *optionName) #if USE_SSL if (strcasecmp(protocol, "https") == 0) { - /* ssl-bump on https_port configuration requires either tproxy or intercepted, and vice versa */ + /* ssl-bump on https_port configuration requires either tproxy or intercept, and vice versa */ const bool hijacked = s->spoof_client_ip || s->intercepted; if (s->sslBump && !hijacked) { - debugs(3, DBG_CRITICAL, "FATAL: ssl-bump on https_port requires tproxy/intercepted which is missing."); + debugs(3, DBG_CRITICAL, "FATAL: ssl-bump on https_port requires tproxy/intercept which is missing."); self_destruct(); } if (hijacked && !s->sslBump) { - debugs(3, DBG_CRITICAL, "FATAL: tproxy/intercepted on https_port requires ssl-bump which is missing."); + debugs(3, DBG_CRITICAL, "FATAL: tproxy/intercept on https_port requires ssl-bump which is missing."); self_destruct(); } } @@ -4537,36 +4538,101 @@ static void free_sslproxy_cert_sign(sslproxy_cert_sign **cert_sign) } } +class sslBumpCfgRr: public ::RegisteredRunner +{ +public: + static Ssl::BumpMode lastDeprecatedRule; + /* RegisteredRunner API */ + virtual void run(const RunnerRegistry &); +}; + +Ssl::BumpMode sslBumpCfgRr::lastDeprecatedRule = Ssl::bumpEnd; + +RunnerRegistrationEntry(rrFinalizeConfig, sslBumpCfgRr); + +void sslBumpCfgRr::run(const RunnerRegistry &r) +{ + if (lastDeprecatedRule != Ssl::bumpEnd) { + assert( lastDeprecatedRule == Ssl::bumpClientFirst || lastDeprecatedRule == Ssl::bumpNone); + static char buf[1024]; + if (lastDeprecatedRule == Ssl::bumpClientFirst) { + strcpy(buf, "ssl_bump deny all"); + debugs(3, DBG_CRITICAL, "WARNING: auto-converting deprecated implicit " + "\"ssl_bump deny all\" to \"ssl_bump none all\". New ssl_bump configurations " + "must not use implicit rules. Update your ssl_bump rules."); + } else { + strcpy(buf, "ssl_bump allow all"); + debugs(3, DBG_CRITICAL, "SECURITY NOTICE: auto-converting deprecated implicit " + "\"ssl_bump allow all\" to \"ssl_bump client-first all\" which is usually " + "inferior to the newer server-first bumping mode. New ssl_bump" + " configurations must not use implicit rules. Update your ssl_bump rules."); + } + parse_line(buf); + } +} + static void parse_sslproxy_ssl_bump(acl_access **ssl_bump) { + typedef const char *BumpCfgStyle; + BumpCfgStyle bcsNone = NULL; + BumpCfgStyle bcsNew = "new client/server-first/none"; + BumpCfgStyle bcsOld = "deprecated allow/deny"; + static BumpCfgStyle bumpCfgStyleLast = bcsNone; + BumpCfgStyle bumpCfgStyleNow = bcsNone; char *bm; if ((bm = strtok(NULL, w_space)) == NULL) { self_destruct(); return; } + // if this is the first rule proccessed + if (*ssl_bump == NULL) { + bumpCfgStyleLast = bcsNone; + sslBumpCfgRr::lastDeprecatedRule = Ssl::bumpEnd; + } + acl_access *A = new acl_access; A->allow = allow_t(ACCESS_ALLOWED); - if (strcmp(bm, Ssl::BumpModeStr[Ssl::bumpClientFirst]) == 0) + if (strcmp(bm, Ssl::BumpModeStr[Ssl::bumpClientFirst]) == 0) { A->allow.kind = Ssl::bumpClientFirst; - else if (strcmp(bm, Ssl::BumpModeStr[Ssl::bumpServerFirst]) == 0) + bumpCfgStyleNow = bcsNew; + } else if (strcmp(bm, Ssl::BumpModeStr[Ssl::bumpServerFirst]) == 0) { A->allow.kind = Ssl::bumpServerFirst; - else if (strcmp(bm, Ssl::BumpModeStr[Ssl::bumpNone]) == 0) + bumpCfgStyleNow = bcsNew; + } else if (strcmp(bm, Ssl::BumpModeStr[Ssl::bumpNone]) == 0) { A->allow.kind = Ssl::bumpNone; - else if (strcmp(bm, "allow") == 0 || strcmp(bm, "deny") == 0) { - // allow/deny rule sets may rely on an implicit "negate the last one" - // rule which we cannot support due to multuple "allow" keywords - debugs(3, DBG_CRITICAL, "FATAL: ssl_bump allow/deny rule(s) " << - "must be CAREFULLY converted to specify bump mode(s)."); - self_destruct(); - return; + bumpCfgStyleNow = bcsNew; + } else if (strcmp(bm, "allow") == 0) { + debugs(3, DBG_CRITICAL, "SECURITY NOTICE: auto-converting deprecated " + "\"ssl_bump allow \" to \"ssl_bump client-first \" which " + "is usually inferior to the newer server-first " + "bumping mode. Update your ssl_bump rules."); + A->allow.kind = Ssl::bumpClientFirst; + bumpCfgStyleNow = bcsOld; + sslBumpCfgRr::lastDeprecatedRule = Ssl::bumpClientFirst; + } else if (strcmp(bm, "deny") == 0) { + debugs(3, DBG_CRITICAL, "WARNING: auto-converting deprecated " + "\"ssl_bump deny \" to \"ssl_bump none \". Update " + "your ssl_bump rules."); + A->allow.kind = Ssl::bumpNone; + bumpCfgStyleNow = bcsOld; + sslBumpCfgRr::lastDeprecatedRule = Ssl::bumpNone; } else { debugs(3, DBG_CRITICAL, "FATAL: unknown ssl_bump mode: " << bm); self_destruct(); return; } + if (bumpCfgStyleLast != bcsNone && bumpCfgStyleNow != bumpCfgStyleLast) { + debugs(3, DBG_CRITICAL, "FATAL: do not mix " << bumpCfgStyleNow << " actions with " << + bumpCfgStyleLast << " actions. Update your ssl_bump rules."); + self_destruct(); + return; + } + + bumpCfgStyleLast = bumpCfgStyleNow; + aclParseAclList(LegacyParser, &A->aclList); acl_access *B, **T; diff --git a/src/cf.data.pre b/src/cf.data.pre index 66573a8a6f..458c850010 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -647,7 +647,7 @@ DOC_END NAME: acl TYPE: acl LOC: Config.aclList -IFDEF USE_SSL +IF USE_SSL DEFAULT: ssl::certHasExpired ssl_error X509_V_ERR_CERT_HAS_EXPIRED DEFAULT: ssl::certNotYetValid ssl_error X509_V_ERR_CERT_NOT_YET_VALID DEFAULT: ssl::certDomainMismatch ssl_error SQUID_X509_V_ERR_DOMAIN_MISMATCH @@ -878,7 +878,7 @@ DOC_START # effect in rules that affect the reply data stream such as # http_reply_access. -IFDEF USE_SSL +IF USE_SSL acl aclname ssl_error errorname # match against SSL certificate validation error [fast] # @@ -1562,6 +1562,10 @@ DOC_START accel Accelerator / reverse proxy mode + intercept Support for IP-Layer interception of + outgoing requests without browser settings. + NP: disables authentication and IPv6 on the port. + tproxy Support Linux TPROXY for spoofing outgoing connections using the client IP address. NP: disables authentication and maybe IPv6 on the port. @@ -1575,7 +1579,7 @@ DOC_START An "ssl_bump server-first" match is required to fully enable bumping of intercepted SSL connections. - Requires tproxy. + Requires tproxy or intercept. Omitting the mode flag causes default forward proxy mode to be used. diff --git a/src/cf_gen.cc b/src/cf_gen.cc index 29adf0e67f..3a067ec12f 100644 --- a/src/cf_gen.cc +++ b/src/cf_gen.cc @@ -259,16 +259,16 @@ main(int argc, char *argv[]) if ((t = strchr(buff, '\n'))) *t = '\0'; - if(strncmp(buff, "IFDEF ", 6) == 0) { - if ((ptr = strtok(buff + 6, WS)) == NULL) { - std::cerr << "Missing IFDEF parameter on line" << linenum << std::endl; + if(strncmp(buff, "IF ", 3) == 0) { + if ((ptr = strtok(buff + 3, WS)) == NULL) { + std::cerr << "Missing IF parameter on line" << linenum << std::endl; exit(1); } IFDEFS.push(ptr); continue; } else if (strcmp(buff, "ENDIF") == 0) { if (IFDEFS.size() == 0) { - std::cerr << "ENDIF without IFDEF before on line " << linenum << std::endl; + std::cerr << "ENDIF without IF before on line " << linenum << std::endl; exit(1); } IFDEFS.pop(); diff --git a/src/client_side.cc b/src/client_side.cc index 594e204b26..9914034c34 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -3648,7 +3648,7 @@ httpsAccept(const CommAcceptCbParams ¶ms) } // Create a fake HTTP request for ssl_bump ACL check, - // using tproxy-provided destination IP and port. + // using tproxy/intercept provided destination IP and port. HttpRequest *request = new HttpRequest(); static char ip[MAX_IPSTRLEN]; assert(params.conn->flags & (COMM_TRANSPARENT | COMM_INTERCEPTION)); diff --git a/src/client_side.h b/src/client_side.h index ca5e1b25fd..6c43258e5f 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -343,7 +343,12 @@ public: void switchToHttps(HttpRequest *request, Ssl::BumpMode bumpServerMode); bool switchedToHttps() const { return switchedToHttps_; } Ssl::ServerBump *serverBump() {return sslServerBump;} - void setServerBump(Ssl::ServerBump *srvBump) {if (!sslServerBump) sslServerBump = srvBump;} + inline void setServerBump(Ssl::ServerBump *srvBump) { + if (!sslServerBump) + sslServerBump = srvBump; + else + assert(sslServerBump == srvBump); + } /// Fill the certAdaptParams with the required data for certificate adaptation /// and create the key for storing/retrieve the certificate to/from the cache void buildSslCertGenerationParams(Ssl::CertificateProperties &certProperties); diff --git a/src/client_side_request.cc b/src/client_side_request.cc index c27598f93d..4522ac483c 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -1523,65 +1523,67 @@ ClientHttpRequest::doCallouts() if (!calloutContext->http->al.request) calloutContext->http->al.request = HTTPMSGLOCK(request); + if (!calloutContext->error) { // CVE-2009-0801: verify the Host: header is consistent with other known details. - if (!calloutContext->error && !calloutContext->host_header_verify_done) { - debugs(83, 3, HERE << "Doing calloutContext->hostHeaderVerify()"); - calloutContext->host_header_verify_done = true; - calloutContext->hostHeaderVerify(); - return; - } + if (!calloutContext->host_header_verify_done) { + debugs(83, 3, HERE << "Doing calloutContext->hostHeaderVerify()"); + calloutContext->host_header_verify_done = true; + calloutContext->hostHeaderVerify(); + return; + } - if (!calloutContext->error && !calloutContext->http_access_done) { - debugs(83, 3, HERE << "Doing calloutContext->clientAccessCheck()"); - calloutContext->http_access_done = true; - calloutContext->clientAccessCheck(); - return; - } + if (!calloutContext->http_access_done) { + debugs(83, 3, HERE << "Doing calloutContext->clientAccessCheck()"); + calloutContext->http_access_done = true; + calloutContext->clientAccessCheck(); + return; + } #if USE_ADAPTATION - if (!calloutContext->error && !calloutContext->adaptation_acl_check_done) { - calloutContext->adaptation_acl_check_done = true; - if (Adaptation::AccessCheck::Start( + if (!calloutContext->adaptation_acl_check_done) { + calloutContext->adaptation_acl_check_done = true; + if (Adaptation::AccessCheck::Start( Adaptation::methodReqmod, Adaptation::pointPreCache, request, NULL, this)) - return; // will call callback - } + return; // will call callback + } #endif - if (!calloutContext->error && !calloutContext->redirect_done) { - calloutContext->redirect_done = true; - assert(calloutContext->redirect_state == REDIRECT_NONE); + if (!calloutContext->redirect_done) { + calloutContext->redirect_done = true; + assert(calloutContext->redirect_state == REDIRECT_NONE); - if (Config.Program.redirect) { - debugs(83, 3, HERE << "Doing calloutContext->clientRedirectStart()"); - calloutContext->redirect_state = REDIRECT_PENDING; - calloutContext->clientRedirectStart(); - return; + if (Config.Program.redirect) { + debugs(83, 3, HERE << "Doing calloutContext->clientRedirectStart()"); + calloutContext->redirect_state = REDIRECT_PENDING; + calloutContext->clientRedirectStart(); + return; + } } - } - if (!calloutContext->error && !calloutContext->adapted_http_access_done) { - debugs(83, 3, HERE << "Doing calloutContext->clientAccessCheck2()"); - calloutContext->adapted_http_access_done = true; - calloutContext->clientAccessCheck2(); - return; - } + if (!calloutContext->adapted_http_access_done) { + debugs(83, 3, HERE << "Doing calloutContext->clientAccessCheck2()"); + calloutContext->adapted_http_access_done = true; + calloutContext->clientAccessCheck2(); + return; + } - if (!calloutContext->error && !calloutContext->interpreted_req_hdrs) { - debugs(83, 3, HERE << "Doing clientInterpretRequestHeaders()"); - calloutContext->interpreted_req_hdrs = 1; - clientInterpretRequestHeaders(this); - } + if (!calloutContext->interpreted_req_hdrs) { + debugs(83, 3, HERE << "Doing clientInterpretRequestHeaders()"); + calloutContext->interpreted_req_hdrs = 1; + clientInterpretRequestHeaders(this); + } - if (!calloutContext->error && !calloutContext->no_cache_done) { - calloutContext->no_cache_done = true; + if (!calloutContext->no_cache_done) { + calloutContext->no_cache_done = true; - if (Config.accessList.noCache && request->flags.cachable) { - debugs(83, 3, HERE << "Doing calloutContext->checkNoCache()"); - calloutContext->checkNoCache(); - return; + if (Config.accessList.noCache && request->flags.cachable) { + debugs(83, 3, HERE << "Doing calloutContext->checkNoCache()"); + calloutContext->checkNoCache(); + return; + } } - } + } // if !calloutContext->error if (!calloutContext->tosToClientDone) { calloutContext->tosToClientDone = true; diff --git a/src/errorpage.cc b/src/errorpage.cc index 0f90ad3509..d073580b5e 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -597,12 +597,6 @@ ErrorState::ErrorState(err_type t, http_status status, HttpRequest * req) : } } -void -ErrorState::detailError(int detailCode) -{ - detailCode = detailCode; -} - void errorAppendEntry(StoreEntry * entry, ErrorState * err) { diff --git a/src/errorpage.h b/src/errorpage.h index 5253106860..22e6228eee 100644 --- a/src/errorpage.h +++ b/src/errorpage.h @@ -106,7 +106,7 @@ public: HttpReply *BuildHttpReply(void); /// set error type-specific detail code - void detailError(int detailCode); + void detailError(int dCode) {detailCode = dCode;} private: /** diff --git a/src/url.cc b/src/url.cc index b560848166..44876fe144 100644 --- a/src/url.cc +++ b/src/url.cc @@ -462,6 +462,7 @@ urlParseFinish(const HttpRequestMethod& method, request = new HttpRequest(method, protocol, urlpath); else { request->initHTTP(method, protocol, urlpath); + safe_free(request->canonical); } request->SetHost(host); -- 2.47.2