Hi, On Fri, 24 Jul 2015, Amos Jeffries wrote: > That redesign has proven non-trivial and halted the (brief) two attempts > I've made at it so far. Anyone wanting to dig in and assist is welcome. > I can offer naming credits in the official Advisory document, and > gratitude from all distros for a working Squid 3.3 or earlier patch.
I gave a try at this... I have something that compiles but I won't have the time to test it any time soon (vacation coming up). It was definitely not trivial to backport and I made the choice to backport http://bazaar.launchpad.net/~squid/squid/3.4/revision/12905 too otherwise I could not imagine any way to do it. I would be glad if someone else could test it and review it. It's a patch for the Debian package in Debian 6 squeeze, thus 3.1.6. Beware, I have almost zero practical experience with C++ and there might be some newbie mistakes. Cheers, -- Raphaël Hertzog ◈ Debian Developer Support Debian LTS: http://www.freexian.com/services/debian-lts.html Learn to master Debian: http://debian-handbook.info/get/
#! /bin/sh /usr/share/dpatch/dpatch-run ## CVE-2015-5400.dpatch by Raphaël Hertzog <hert...@debian.org> ## ## All lines beginning with `## DP:' are a description of the patch. ## DP: Do not blindly forward cache peer CONNECT responses. ## DP: Backport of http://bazaar.launchpad.net/~squid/squid/3.4/revision/13225 ## DP: Required backport of http://bazaar.launchpad.net/~squid/squid/3.4/revision/12905 too @DPATCH@ diff -urNad '--exclude=CVS' '--exclude=.svn' '--exclude=.git' '--exclude=.arch' '--exclude=.hg' '--exclude=_darcs' '--exclude=.bzr' squid3-3.1.6~/src/tunnel.cc squid3-3.1.6/src/tunnel.cc --- squid3-3.1.6~/src/tunnel.cc 2015-07-27 17:00:26.000000000 +0000 +++ squid3-3.1.6/src/tunnel.cc 2015-07-27 17:06:56.707652156 +0000 @@ -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" @@ -60,6 +61,12 @@ 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 */ @@ -67,6 +74,23 @@ 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 { @@ -88,9 +112,12 @@ 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_; @@ -103,15 +130,23 @@ 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()) @@ -135,13 +170,14 @@ 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; } @@ -154,13 +190,14 @@ 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; } @@ -251,9 +288,121 @@ 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) { @@ -296,11 +445,14 @@ 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 @@ -320,10 +472,22 @@ 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 */ @@ -399,6 +563,13 @@ } 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) { debugs(26, 3, "tunnelWriteClient: FD " << client.fd() << ", " << len << " bytes written"); @@ -521,7 +692,33 @@ 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 @@ -718,9 +915,31 @@ 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) { @@ -780,6 +999,8 @@ { CBDATA_INIT_TYPE(TunnelStateData); TunnelStateData *result = cbdataAlloc(TunnelStateData); + result->connectReqWriting = false; + result->connectRespBuf = NULL; return result; } @@ -787,6 +1008,7 @@ TunnelStateData::operator delete (void *address) { TunnelStateData *t = static_cast<TunnelStateData *>(address); + delete t->connectRespBuf; cbdataFree(t); }
