From: Alex Rousskov Date: Wed, 14 Dec 2011 18:24:29 +0000 (-0700) Subject: Peek at the origin server SSL certificate when bumping intercepted HTTPS. X-Git-Tag: BumpSslServerFirst.take01~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d7ce0bcd1ca3d7f068e8238aa2bf90bdbf5b386f;p=thirdparty%2Fsquid.git Peek at the origin server SSL certificate when bumping intercepted HTTPS. * Configuration changes: Allow intercepted SSL connections to be bumped, in addition to the tproxied SSL connections. Honor and check ssl-bump flag in https_port. Earlier code apparently assumed that the flag must be present in http_port and left Ssl::TheGlobalContextStorage uninitialized if only https_port had the flag. * Client-side changes: Added a new Ssl::ServerPeeker class to do client-side error handling while peeking at the origin server certificate. Peeking is done at the server-side. Server-side uses Store and store_client API to report errors to the client-side. That works OK when the errors can be sent to the client, but when we bump intercepted connections, the client does not yet have a secure connection established with Squid so errors cannot be sent (popular browsers do not display CONNECT-stage errors). Instead, the errors must be accumulated and sent after the secure connection with the client is established (in response to the first HTTP request on that connection). Ssl::ServerPeeker needs work to support such accumulation. Start Ssl::ServerPeeker job in ConnStateData::switchToHttps() and wait for somebody to call the new ConnStateData::httpsPeeked() method back. Needs more work to actually use the peeked certificate and handle errors. Changed ConnStateData::switchToHttps() profile to require destination port number. Without it, we cannot switch intercepted SSL connections because they do not have a proper request structure that can supply port details. Polished pinned connection cleaning code: If our Comm close handler for the pinned connection was called, do no try to remove the handler. If the pinned connection was closed (e.g., by a server-side error), do not try to close it again. If we already called unpinConnection(), do not try to close the pinned connection again. Do not assume we have a request when pinning a connection to the server. Intercepted connections do not have requests at the connection pinning stage. * Server-side changes: Bug 3243 (CVE 2009-0801: Bypass of browser same-origin access control in intercepted communication) fix always created a new connection to the origin server. I think it is safe (and possibly even safer!) to reuse a pinned connection instead (if one is available). We now do that in the new FwdState::selectPeerForIntercepted() method. If bump-server-first does not reuse a pinned connection (left from certificate peeking), Squid would be opening and closing to-server connections just to learn the certificate, which is not kosher. Added a new internal protocol type (PROTO_SSL_PEEK) to allow FwdState to detect peeking requests and end processing after the certificate is received (instead of proceeding with to httpStart()). --- diff --git a/src/anyp/ProtocolType.h b/src/anyp/ProtocolType.h index 8a43b39fad..a3b4b2c610 100644 --- a/src/anyp/ProtocolType.h +++ b/src/anyp/ProtocolType.h @@ -27,8 +27,11 @@ typedef enum { #endif PROTO_URN, PROTO_WHOIS, - PROTO_INTERNAL, + PROTO_INTERNAL, // miss on an internal object such as an icon PROTO_ICY, +#if USE_SSL + PROTO_SSL_PEEK, // an internal request to peek at an HTTPS server +#endif PROTO_UNKNOWN, PROTO_MAX } ProtocolType; diff --git a/src/client_side.cc b/src/client_side.cc index 84bd32334e..636455d166 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -121,6 +121,7 @@ #if USE_SSL #include "ssl/context_storage.h" #include "ssl/helper.h" +#include "ssl/ServerPeeker.h" #include "ssl/support.h" #include "ssl/gadgets.h" #endif @@ -3435,7 +3436,7 @@ httpsEstablish(ConnStateData *connState, SSL_CTX *sslContext) else { char buf[MAX_IPSTRLEN]; debugs(33, 4, HERE << details << " try to generate a Dynamic SSL CTX"); - connState->switchToHttps(details->local.NtoA(buf, sizeof(buf))); + connState->switchToHttps(details->local.NtoA(buf, sizeof(buf)), details->local.GetPort()); } } @@ -3511,7 +3512,7 @@ httpsAccept(const CommAcceptCbParams ¶ms) // using tproxy-provided destination IP and port. HttpRequest *request = new HttpRequest(); static char ip[MAX_IPSTRLEN]; - assert(params.conn->flags & COMM_TRANSPARENT); + assert(params.conn->flags & (COMM_TRANSPARENT | COMM_INTERCEPTION)); request->SetHost(params.conn->local.NtoA(ip, sizeof(ip))); request->port = params.conn->local.GetPort(); request->myportname = s->name; @@ -3644,7 +3645,7 @@ ConnStateData::getSslContextDone(SSL_CTX * sslContext, bool isNew) } void -ConnStateData::switchToHttps(const char *host) +ConnStateData::switchToHttps(const char *host, const int port) { assert(!switchedToHttps_); @@ -3655,10 +3656,36 @@ ConnStateData::switchToHttps(const char *host) freeAllContexts(); //currentobject->connIsFinished(); + /* careful: freeAllContexts() above frees request, host, etc. */ + // We are going to read new request flags.readMore = true; debugs(33, 5, HERE << "converting " << clientConnection << " to SSL"); + const bool alwaysBumpServerFirst = true; + if (alwaysBumpServerFirst) { + Must(!httpsPeeker.set()); + httpsPeeker = AsyncJob::Start(new Ssl::ServerPeeker( + this, sslHostName.termedBuf(), port)); + // will call httpsPeeked() with certificate and connection, eventually + return; + } + + // otherwise, use sslHostName + getSslContextStart(); +} + +void +ConnStateData::httpsPeeked(Comm::ConnectionPointer serverConnection) +{ + Must(httpsPeeker.set()); + // XXX: handle httpsPeeker errors + + pinConnection(serverConnection, NULL, NULL, false); + + // XXX: change sslHostName based on httpsPeeker results + debugs(33, 5, HERE << "bumped HTTPS server: " << sslHostName); + httpsPeeker.clear(); getSslContextStart(); } @@ -3759,6 +3786,22 @@ clientHttpsConnectionsOpen(void) continue; } + // TODO: merge with similar code in clientHttpConnectionsOpen() + if (s->sslBump && !Config.accessList.ssl_bump) { + debugs(33, DBG_IMPORTANT, "WARNING: No ssl_bump configured. Disabling ssl-bump on " << s->protocol << "_port " << s->http.s); + s->sslBump = 0; + } + + if (s->sslBump && !s->staticSslContext && !s->generateHostCertificates) { + debugs(1, DBG_IMPORTANT, "Will not bump SSL at http_port " << s->http.s << " due to SSL initialization failure."); + s->sslBump = 0; + } + + if (s->sslBump) { + // Create ssl_ctx cache for this port. + Ssl::TheGlobalContextStorage.addLocalStorage(s->s, s->dynamicCertMemCacheSize == std::numeric_limits::max() ? 4194304 : s->dynamicCertMemCacheSize); + } + // Fill out a Comm::Connection which IPC will open as a listener for us s->http.listenConn = new Comm::Connection; s->http.listenConn->local = s->http.s; @@ -4099,10 +4142,11 @@ ConnStateData::sendControlMsg(HttpControlMsg msg) clientConnection->close(); } -/* This is a comm call normally scheduled by comm_close() */ +/// Our close handler called by Comm when the pinned connection is closed void ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &io) { + pinning.closeHandler = NULL; // Comm unregisters handlers before calling unpinConnection(); } @@ -4114,22 +4158,30 @@ ConnStateData::pinConnection(const Comm::ConnectionPointer &pinServer, HttpReque if (Comm::IsConnOpen(pinning.serverConnection)) { if (pinning.serverConnection->fd == pinServer->fd) return; + } - unpinConnection(); // clears fields ready for re-use. Prevent close() scheduling our close handler. - pinning.serverConnection->close(); - } else - unpinConnection(); // clears fields ready for re-use. + unpinConnection(); // closes pinned connection, if any, and resets fields. pinning.serverConnection = pinServer; - pinning.host = xstrdup(request->GetHost()); - pinning.port = request->port; + + // when pinning an SSL bumped connection, the request may be NULL + const char *pinnedHost = "[unknown]"; + if (request) { + pinning.host = xstrdup(request->GetHost()); + pinning.port = request->port; + pinnedHost = pinning.host; + } else { + pinning.port = pinServer->remote.GetPort(); + } pinning.pinned = true; if (aPeer) pinning.peer = cbdataReference(aPeer); pinning.auth = auth; char stmp[MAX_IPSTRLEN]; snprintf(desc, FD_DESC_SZ, "%s pinned connection for %s (%d)", - (auth || !aPeer) ? request->GetHost() : aPeer->name, clientConnection->remote.ToURL(stmp,MAX_IPSTRLEN), clientConnection->fd); + (auth || !aPeer) ? pinnedHost : aPeer->name, + clientConnection->remote.ToURL(stmp,MAX_IPSTRLEN), + clientConnection->fd); fd_note(pinning.serverConnection->fd, desc); typedef CommCbMemFunT Dialer; @@ -4171,12 +4223,16 @@ ConnStateData::unpinConnection() if (pinning.peer) cbdataReferenceDone(pinning.peer); + if (Comm::IsConnOpen(pinning.serverConnection)) { if (pinning.closeHandler != NULL) { comm_remove_close_handler(pinning.serverConnection->fd, pinning.closeHandler); pinning.closeHandler = NULL; } /// also close the server side socket, we should not use it for any future requests... + // TODO: do not close if called from our close handler? pinning.serverConnection->close(); + } + safe_free(pinning.host); /* NOTE: pinning.pinned should be kept. This combined with fd == -1 at the end of a request indicates that the host diff --git a/src/client_side.h b/src/client_side.h index f0960f138d..295bc4c9f8 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -306,6 +306,9 @@ public: virtual void swanSong(); #if USE_SSL + /// called by Ssl::ServerPeeker when it is done bumping the server + void httpsPeeked(Comm::ConnectionPointer serverConnection); + /// Start to create dynamic SSL_CTX for host or uses static port SSL context. void getSslContextStart(); /** @@ -319,7 +322,7 @@ public: /// Proccess response from ssl_crtd. void sslCrtdHandleReply(const char * reply); - void switchToHttps(const char *host); + void switchToHttps(const char *host, const int port); bool switchedToHttps() const { return switchedToHttps_; } #else bool switchedToHttps() const { return false; } @@ -343,8 +346,14 @@ private: CBDATA_CLASS2(ConnStateData); bool closing_; +#if USE_SSL bool switchedToHttps_; String sslHostName; ///< Host name for SSL certificate generation + + /// a job that connects to the HTTPS server to get its SSL certificate + AsyncJob::Pointer httpsPeeker; +#endif + AsyncCall::Pointer reader; ///< set when we are reading BodyPipe::Pointer bodyPipe; // set when we are reading request body }; diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 27b8e842d7..ef6640f73c 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -1373,7 +1373,7 @@ ClientHttpRequest::sslBumpEstablish(comm_err_t errflag) return; } - getConn()->switchToHttps(request->GetHost()); + getConn()->switchToHttps(request->GetHost(), request->port); } void diff --git a/src/forward.cc b/src/forward.cc index 56f3c65471..449124a10e 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -122,11 +122,7 @@ void FwdState::start(Pointer aSelf) // To resolve this we must force DIRECT and only to the original client destination. if (Config.onoff.client_dst_passthru && request && !request->flags.redirected && (request->flags.intercepted || request->flags.spoof_client_ip)) { - Comm::ConnectionPointer p = new Comm::Connection(); - p->remote = clientConn->local; - p->peerType = ORIGINAL_DST; - getOutgoingAddress(request, p); - serverDestinations.push_back(p); + selectPeerForIntercepted(); // destination "found". continue with the forwarding. startConnectionOrFail(); @@ -136,6 +132,31 @@ void FwdState::start(Pointer aSelf) } } +/// bypasses peerSelect() when dealing with intercepted requests +void +FwdState::selectPeerForIntercepted() +{ + // use pinned connection if available + Comm::ConnectionPointer p; + if (ConnStateData *client = request->pinnedConnection()) + p = client->validatePinnedConnection(request, NULL); + + if (p != NULL && Comm::IsConnOpen(p)) { + debugs(17, 3, HERE << "reusing a pinned conn: " << *p); + /* duplicate peerSelectPinned() effects */ + p->peerType = PINNED; + entry->ping_status = PING_DONE; /* Skip ICP */ + } else { + debugs(17, 3, HERE << "opening a new conn: " << *p); + p = new Comm::Connection(); + p->peerType = ORIGINAL_DST; + p->remote = clientConn->local; + getOutgoingAddress(request, p); + } + + serverDestinations.push_back(p); +} + void FwdState::completed() { @@ -755,7 +776,8 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, comm_err_t status, in #if USE_SSL if ((serverConnection()->getPeer() && serverConnection()->getPeer()->use_ssl) || - (!serverConnection()->getPeer() && request->protocol == AnyP::PROTO_HTTPS)) { + (!serverConnection()->getPeer() && + (request->protocol == AnyP::PROTO_HTTPS || request->protocol == AnyP::PROTO_SSL_PEEK))) { initiateSSL(); return; } @@ -959,6 +981,16 @@ FwdState::dispatch() } #endif +#if USE_SSL + if (request->protocol == AnyP::PROTO_SSL_PEEK) { + CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData, + ConnStateData::httpsPeeked, serverConnection()); + unregister(serverConn); // async call owns it now + complete(); // destroys us + return; + } +#endif + if (serverConnection()->getPeer() != NULL) { serverConnection()->getPeer()->stats.fetches++; request->peer_login = serverConnection()->getPeer()->login; @@ -992,6 +1024,10 @@ FwdState::dispatch() case AnyP::PROTO_INTERNAL: +#if USE_SSL + case AnyP::PROTO_SSL_PEEK: +#endif + case AnyP::PROTO_URN: fatal_dump("Should never get here"); break; diff --git a/src/forward.h b/src/forward.h index 01cd931f21..505eff5df8 100644 --- a/src/forward.h +++ b/src/forward.h @@ -69,6 +69,7 @@ private: FwdState(const Comm::ConnectionPointer &client, StoreEntry *, HttpRequest *); void start(Pointer aSelf); + void selectPeerForIntercepted(); static void logReplyStatus(int tries, http_status status); void doneWithRetries(); void completed(); diff --git a/src/ssl/Makefile.am b/src/ssl/Makefile.am index e73e9db059..4cde18b11e 100644 --- a/src/ssl/Makefile.am +++ b/src/ssl/Makefile.am @@ -30,6 +30,8 @@ libsslsquid_la_SOURCES = \ ErrorDetail.h \ ErrorDetailManager.cc \ ErrorDetailManager.h \ + ServerPeeker.cc \ + ServerPeeker.h \ support.cc \ support.h diff --git a/src/ssl/ServerPeeker.cc b/src/ssl/ServerPeeker.cc new file mode 100644 index 0000000000..21c04c2d36 --- /dev/null +++ b/src/ssl/ServerPeeker.cc @@ -0,0 +1,59 @@ +/* + * $Id$ + * + * DEBUG: section 33 Client-side Routines + * + */ + +#include "config.h" + +#include "client_side.h" +#include "forward.h" +#include "ssl/ServerPeeker.h" +#include "Store.h" + + +CBDATA_NAMESPACED_CLASS_INIT(Ssl, ServerPeeker); + + +Ssl::ServerPeeker::ServerPeeker(ConnStateData *anInitiator, + const char *host, const int port): + AsyncJob("Ssl::ServerPeeker"), + initiator(anInitiator), + clientConnection(anInitiator->clientConnection), + request(new HttpRequest), + entry(NULL) +{ + debugs(33, 4, HERE << "will peek at " << host << ':' << port); + + request->SetHost(host); + request->port = port; + request->protocol = AnyP::PROTO_SSL_PEEK; + request->clientConnectionManager = initiator; +} + +void +Ssl::ServerPeeker::start() +{ + const char *uri = urlCanonical(request); + entry = storeCreateEntry(uri, uri, request->flags, request->method); + + FwdState::fwdStart(clientConnection, entry, request); + + // XXX: wait for FwdState to tell us the connection is ready + + // TODO: send our answer to the initiator + // CallJobHere(33, 4, initiator, ConnStateData, ConnStateData::httpsPeeked); + initiator.clear(); // will trigger the end of the job +} + +bool +Ssl::ServerPeeker::doneAll() const +{ + return !initiator.valid() && AsyncJob::doneAll(); +} + +void +Ssl::ServerPeeker::swanSong() +{ +} diff --git a/src/ssl/ServerPeeker.h b/src/ssl/ServerPeeker.h new file mode 100644 index 0000000000..e0e4620f9d --- /dev/null +++ b/src/ssl/ServerPeeker.h @@ -0,0 +1,57 @@ +#ifndef _SQUID_SSL_PEEKER_H +#define _SQUID_SSL_PEEKER_H + +#include "base/AsyncJob.h" +#include "base/CbcPointer.h" +#include "comm/forward.h" +#include "HttpRequest.h" +#include "ip/Address.h" + +class ConnStateData; + +namespace Ssl +{ + +/** + \ingroup ServerProtocolSSLAPI + * A job to facilitate connecting to the HTTPS server to learn its certificate. + * + * The Peeker job calls FwdState::fwdStart(). There are two possible outcomes: + * + * Success: FwdState calls ConnStateData which pins the establihsed connection + * for future bumped HTTP requests (TODO: and stops this job). + * + * Error: FwdState Stores the error (TODO: and this job preserves it for + * for serving to the client in response to the first bumped request). + */ +class ServerPeeker: public AsyncJob +{ +public: + typedef CbcPointer Pointer; + + explicit ServerPeeker(ConnStateData *anInitiator, const char *host, const int port); + + /* AsyncJob API */ + //virtual ~ServerPeeker(); + virtual void start(); + virtual bool doneAll() const; + virtual void swanSong(); + +private: + /// connection manager waiting for peeked server info + CbcPointer initiator; + + /// client-Squid connection which triggered this job + Comm::ConnectionPointer clientConnection; + + /// faked, minimal request; required by server-side API + HttpRequest::Pointer request; + + StoreEntry *entry; ///< for receiving Squid-generated error messages + + CBDATA_CLASS2(ServerPeeker); +}; + +} // namespace Ssl + +#endif