From: Alex Rousskov Date: Thu, 30 Jul 2015 03:51:08 +0000 (-0700) Subject: Do not blindly forward cache peer CONNECT responses. X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d224c727762062f42aa02991f6b1367b9d98eb9d;p=thirdparty%2Fsquid.git Do not blindly forward cache peer CONNECT responses. Squid blindly forwards cache peer CONNECT responses to clients. This may break things if the peer responds with something like HTTP 403 (Forbidden) and keeps the connection with Squid open: - The client application issues a CONNECT request. - Squid forwards this request to a cache peer. - Cache peer correctly responds back with a "403 Forbidden". - Squid does not parse cache peer response and just forwards it as if it was a Squid response to the client. - The TCP connections are not closed. At this stage, Squid is unaware that the CONNECT request has failed. All subsequent requests on the user agent TCP connection are treated as tunnelled traffic. Squid is forwarding these requests to the peer on the TCP connection previously used for the 403-ed CONNECT request, without proper processing. The additional headers which should have been applied by Squid to these requests are not applied, and the requests are being forwarded to the cache peer even though the Squid configuration may state that these requests must go directly to the origin server. This fixes Squid to parse cache peer responses, and if an error response found, respond with "502 Bad Gateway" to the client and close the connections. Backport by Raphaël Hertzog based on original work by Alex Rousskov --- diff --git a/src/tunnel.cc b/src/tunnel.cc index 6495a211b4..f567ccbd5e 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -36,6 +36,7 @@ #include "squid.h" #include "errorpage.h" #include "HttpRequest.h" +#include "HttpReply.h" #include "fde.h" #include "comm.h" #include "client_side_request.h" @@ -61,6 +62,12 @@ public: static void WriteClientDone(int fd, char *buf, size_t len, comm_err_t flag, int xerrno, void *data); static void WriteServerDone(int fd, char *buf, size_t len, comm_err_t flag, int xerrno, void *data); + /// Starts reading peer response to our CONNECT request. + void readConnectResponse(); + + /// Called when we may be done handling a CONNECT exchange with the peer. + void connectExchangeCheckpoint(); + bool noConnections() const; char *url; char *host; /* either request->host or proxy host */ @@ -68,6 +75,23 @@ public: HttpRequest *request; FwdServer *servers; + /// Whether we are writing a CONNECT request to a peer. + bool waitingForConnectRequest() const { return connectReqWriting; } + /// Whether we are reading a CONNECT response from a peer. + bool waitingForConnectResponse() const { return connectRespBuf; } + /// Whether we are waiting for the CONNECT request/response exchange with the peer. + bool waitingForConnectExchange() const { return waitingForConnectRequest() || waitingForConnectResponse(); } + + /// Whether the client sent a CONNECT request to us. + bool clientExpectsConnectResponse() const { + return !(request != NULL && + (request->flags.spoof_client_ip || request->flags.intercepted)); + } + + /// Sends "502 Bad Gateway" error response to the client, + /// if it is waiting for Squid CONNECT response, closing connections. + void informUserOfPeerError(const char *errMsg); + class Connection { @@ -89,9 +113,12 @@ public: int debugLevelForError(int const xerrno) const; void closeIfOpen(); void dataSent (size_t amount); + /// writes 'b' buffer, setting the 'writer' member to 'callback'. + void write(const char *b, int size, AsyncCall::Pointer &callback, FREE * free_func); int len; char *buf; int64_t *size_ptr; /* pointer to size in an ConnStateData for logging */ + AsyncCall::Pointer writer; ///< pending Comm::Write callback private: int fd_; @@ -104,15 +131,23 @@ public: Connection client, server; int *status_ptr; /* pointer to status for logging */ + MemBuf *connectRespBuf; ///< accumulates peer CONNECT response when we need it + bool connectReqWriting; ///< whether we are writing a CONNECT request to a peer + void copyRead(Connection &from, IOCB *completion); private: CBDATA_CLASS(TunnelStateData); - void copy (size_t len, comm_err_t errcode, int xerrno, Connection &from, Connection &to, IOCB *); + bool keepGoingAfterRead(size_t len, comm_err_t errcode, int xerrno, Connection &from, Connection &to); + void copy (size_t len, Connection &from, Connection &to, IOCB *); + void handleConnectResponse(const size_t chunkSize); void readServer(char *buf, size_t len, comm_err_t errcode, int xerrno); void readClient(char *buf, size_t len, comm_err_t errcode, int xerrno); void writeClientDone(char *buf, size_t len, comm_err_t flag, int xerrno); void writeServerDone(char *buf, size_t len, comm_err_t flag, int xerrno); + + static void ReadConnectResponseDone(int fd, char *buf, size_t len, comm_err_t errcode, int xerrno, void *data); + void readConnectResponseDone(char *buf, size_t len, comm_err_t errcode, int xerrno); }; #define fd_closed(fd) (fd == -1 || fd_table[fd].closing()) @@ -136,13 +171,14 @@ tunnelServerClosed(int fd, void *data) debugs(26, 3, "tunnelServerClosed: FD " << fd); assert(fd == tunnelState->server.fd()); tunnelState->server.fd(-1); + tunnelState->server.writer = NULL; if (tunnelState->noConnections()) { tunnelStateFree(tunnelState); return; } - if (!tunnelState->server.len) { + if (!tunnelState->client.writer) { comm_close(tunnelState->client.fd()); return; } @@ -155,13 +191,14 @@ tunnelClientClosed(int fd, void *data) debugs(26, 3, "tunnelClientClosed: FD " << fd); assert(fd == tunnelState->client.fd()); tunnelState->client.fd(-1); + tunnelState->client.writer = NULL; if (tunnelState->noConnections()) { tunnelStateFree(tunnelState); return; } - if (!tunnelState->client.len) { + if (!tunnelState->server.writer) { comm_close(tunnelState->server.fd()); return; } @@ -252,9 +289,121 @@ TunnelStateData::readServer(char *buf, size_t len, comm_err_t errcode, int xerrn kb_incr(&statCounter.server.other.kbytes_in, len); } - copy (len, errcode, xerrno, server, client, WriteClientDone); + if (keepGoingAfterRead(len, errcode, xerrno, server, client)) + copy (len, server, client, WriteClientDone); +} + +/// Called when we read [a part of] CONNECT response from the peer +void +TunnelStateData::readConnectResponseDone(char *buf, size_t len, comm_err_t errcode, int xerrno) +{ + debugs(26, 3, server.fd() << ", read " << len << " bytes, err=" << errcode); + assert(waitingForConnectResponse()); + + if (errcode == COMM_ERR_CLOSING) + return; + + if (len > 0) { + connectRespBuf->appended(len); + server.bytesIn(len); + kb_incr(&(statCounter.server.all.kbytes_in), len); + kb_incr(&(statCounter.server.other.kbytes_in), len); + } + + if (keepGoingAfterRead(len, errcode, xerrno, server, client)) + handleConnectResponse(len); +} + +void +TunnelStateData::informUserOfPeerError(const char *errMsg) +{ + server.len = 0; + if (!clientExpectsConnectResponse()) { + // closing the connection is the best we can do here + debugs(50, 3, server.fd() << " closing on error: " << errMsg); + close(server.fd()); + return; + } + ErrorState *err = errorCon(ERR_CONNECT_FAIL, HTTP_BAD_GATEWAY, request); + err->callback = tunnelErrorComplete; + err->callback_data = this; + *status_ptr = HTTP_BAD_GATEWAY; + errorSend(client.fd(), err); +} + +/* Read from client side and queue it for writing to the server */ +void +TunnelStateData::ReadConnectResponseDone(int fd, char *buf, size_t len, comm_err_t errcode, int xerrno, void *data) +{ + TunnelStateData *tunnelState = (TunnelStateData *)data; + assert (cbdataReferenceValid (tunnelState)); + + tunnelState->readConnectResponseDone(buf, len, errcode, xerrno); +} + +/// Parses [possibly incomplete] CONNECT response and reacts to it. +/// If the tunnel is being closed or more response data is needed, returns false. +/// Otherwise, the caller should handle the remaining read data, if any. +void +TunnelStateData::handleConnectResponse(const size_t chunkSize) +{ + assert(waitingForConnectResponse()); + + // Ideally, client and server should use MemBuf or better, but current code + // never accumulates more than one read when shoveling data (XXX) so it does + // not need to deal with MemBuf complexity. To keep it simple, we use a + // dedicated MemBuf for accumulating CONNECT responses. TODO: When shoveling + // is optimized, reuse server.buf for CONNEC response accumulation instead. + + /* mimic the basic parts of HttpStateData::processReplyHeader() */ + HttpReply *rep = new HttpReply; + http_status parseErr = HTTP_STATUS_NONE; + int eof = !chunkSize; + const bool parsed = rep->parse(connectRespBuf, eof, &parseErr); + if (!parsed) { + if (parseErr > 0) { // unrecoverable parsing error + informUserOfPeerError("malformed CONNECT response from peer"); + return; + } + + // need more data + assert(!eof); + assert(!parseErr); + + if (!connectRespBuf->hasSpace()) { + informUserOfPeerError("huge CONNECT response from peer"); + return; + } + + // keep reading + readConnectResponse(); + return; + } + + // CONNECT response was successfully parsed + *status_ptr = rep->sline.status; + + // bail if we did not get an HTTP 200 (Connection Established) response + if (rep->sline.status != HTTP_OK) { + informUserOfPeerError("unsupported CONNECT response status code"); + return; + } + + if (rep->hdr_sz < connectRespBuf->contentSize()) { + // preserve bytes that the server already sent after the CONNECT response + server.len = connectRespBuf->contentSize() - rep->hdr_sz; + memcpy(server.buf, connectRespBuf->content() + rep->hdr_sz, server.len); + } else { + // reset; delay pools were using this field to throttle CONNECT response + server.len = 0; + } + + delete connectRespBuf; + connectRespBuf = NULL; + connectExchangeCheckpoint(); } + void TunnelStateData::Connection::error(int const xerrno) { @@ -297,11 +446,14 @@ TunnelStateData::readClient(char *buf, size_t len, comm_err_t errcode, int xerrn kb_incr(&statCounter.client_http.kbytes_in, len); } - copy (len, errcode, xerrno, client, server, WriteServerDone); + if (keepGoingAfterRead(len, errcode, xerrno, client, server)) + copy (len, client, server, WriteServerDone); } -void -TunnelStateData::copy (size_t len, comm_err_t errcode, int xerrno, Connection &from, Connection &to, IOCB *completion) +/// Updates state after reading from client or server. +/// Returns whether the caller should use the data just read. +bool +TunnelStateData::keepGoingAfterRead(size_t len, comm_err_t errcode, int xerrno, Connection &from, Connection &to) { /* I think this is to prevent free-while-in-a-callback behaviour * - RBC 20030229 @@ -326,10 +478,22 @@ TunnelStateData::copy (size_t len, comm_err_t errcode, int xerrno, Connection &f if (from.len == 0 && !fd_closed(to.fd()) ) { comm_close(to.fd()); } - } else if (cbdataReferenceValid(this)) - comm_write(to.fd(), from.buf, len, completion, this, NULL); + } else if (cbdataReferenceValid(this)) { + cbdataInternalUnlock(this); /* ??? */ + return true; + } cbdataInternalUnlock(this); /* ??? */ + return false; +} + +void +TunnelStateData::copy (size_t len, Connection &from, Connection &to, IOCB *completion) +{ + debugs(26, 3, HERE << "Schedule Write"); + AsyncCall::Pointer call = commCbCall(5,5, "TunnelBlindCopyWriteHandler", + CommIoCbPtrFun(completion, this)); + to.write(from.buf, len, call, NULL); } /* Writes data from the client buffer to the server side */ @@ -402,6 +566,13 @@ TunnelStateData::Connection::dataSent (size_t amount) *size_ptr += amount; } +void +TunnelStateData::Connection::write(const char *b, int size, AsyncCall::Pointer &callback, FREE * free_func) +{ + writer = callback; + comm_write(fd(), b, size, callback, free_func); +} + void TunnelStateData::writeClientDone(char *buf, size_t len, comm_err_t flag, int xerrno) { @@ -523,7 +694,33 @@ tunnelConnectedWriteDone(int fd, char *buf, size_t size, comm_err_t flag, int xe static void tunnelProxyConnectedWriteDone(int fd, char *buf, size_t size, comm_err_t flag, int xerrno, void *data) { - tunnelConnectedWriteDone(fd, buf, size, flag, xerrno, data); + TunnelStateData *tunnelState = (TunnelStateData *)data; + debugs(26, 3, fd << ", flag=" << flag); + tunnelState->server.writer = NULL; + assert(tunnelState->waitingForConnectRequest()); + + if (flag != COMM_OK) { + *tunnelState->status_ptr = HTTP_INTERNAL_SERVER_ERROR; + tunnelErrorComplete(fd, data, 0); + return; + } + + tunnelState->connectReqWriting = false; + tunnelState->connectExchangeCheckpoint(); +} + +void +TunnelStateData::connectExchangeCheckpoint() +{ + if (waitingForConnectResponse()) { + debugs(26, 5, "still reading CONNECT response on " << server.fd()); + } else if (waitingForConnectRequest()) { + debugs(26, 5, "still writing CONNECT request on " << server.fd()); + } else { + assert(!waitingForConnectExchange()); + debugs(26, 3, "done with CONNECT exchange on " << server.fd()); + tunnelConnected(server.fd(), this); + } } static void @@ -742,9 +939,31 @@ tunnelProxyConnected(int fd, void *data) mb.append("\r\n", 2); comm_write_mbuf(tunnelState->server.fd(), &mb, tunnelProxyConnectedWriteDone, tunnelState); + tunnelState->connectReqWriting = true; + + tunnelState->connectRespBuf = new MemBuf; + // SQUID_TCP_SO_RCVBUF: we should not accumulate more than regular I/O buffer + // can hold since any CONNECT response leftovers have to fit into server.buf. + // 2*SQUID_TCP_SO_RCVBUF: HttpMsg::parse() zero-terminates, which uses space. + tunnelState->connectRespBuf->init(SQUID_TCP_SO_RCVBUF, 2*SQUID_TCP_SO_RCVBUF); + + // Start accumulating answer + tunnelState->readConnectResponse(); + commSetTimeout(tunnelState->server.fd(), Config.Timeout.read, tunnelTimeout, tunnelState); } +void +TunnelStateData::readConnectResponse() +{ + assert(waitingForConnectResponse()); + + AsyncCall::Pointer call = commCbCall(5,4, "readConnectResponseDone", + CommIoCbPtrFun(ReadConnectResponseDone, this)); + comm_read(server.fd(), connectRespBuf->space(), + server.bytesWanted(1, connectRespBuf->spaceSize()), call); +} + static void tunnelPeerSelectComplete(FwdServer * fs, void *data) { @@ -804,6 +1023,8 @@ TunnelStateData::operator new (size_t) { CBDATA_INIT_TYPE(TunnelStateData); TunnelStateData *result = cbdataAlloc(TunnelStateData); + result->connectReqWriting = false; + result->connectRespBuf = NULL; return result; } @@ -811,6 +1032,7 @@ void TunnelStateData::operator delete (void *address) { TunnelStateData *t = static_cast(address); + delete t->connectRespBuf; cbdataFree(t); }