------------------------------------------------------------ revno: 14149 revision-id: squid3@treenet.co.nz-20170330133122-zcpblbvnuq7mjvq3 parent: squid3@treenet.co.nz-20170226110942-90rcwhx3fwa2l7is fixes bug: http://bugs.squid-cache.org/show_bug.cgi?id=4508 author: Christos Tsantilas committer: Amos Jeffries branch nick: 3.5 timestamp: Fri 2017-03-31 01:31:22 +1200 message: Bug 4508: Host forgery stalls intercepted being-spliced connections. Most SslBump splicing happens after getting SNI. SNI goes into the second fake CONNECT request, where it may fail the host forgery check. A failed check triggers an HTTP error response from Squid. When attempting to send that response to the TLS client, Squid checks whether all previously pipelined HTTP requests on the connection have finished. Prior to this fix, Squid left the first fake CONNECT request in the connection pipeline despite adding the second fake CONNECT. That first CONNECT stalled the error response described above, with Squid waiting, in vain, for that already handled [fake] transaction to finish. Also call quitAfterError() to force Squid to close the connection (after writing the discussed error response) instead of just logging a [misleading] "kick abandoning [connection]" message in cache.log. TODO: Always pop the first CONNECT when generating a second one. Unifying CONNECT treatment is difficult because code like tunnel.cc wants that CONNECT to be in the pipeline. Polishing that would probably require disassociating ConnStateData from tunnel.cc (at least). TODO: Apply the existing "delayed error" logic (that optionally bumps TLS connections to deliver [some] errors to [some] SSL/TLS clients) to host forgery errors. Otherwise, the plain HTTP error message cannot be understood by the intercepted TLS client. This is a Measurement Factory project ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20170330133122-zcpblbvnuq7mjvq3 # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: db616fff2ac0df73cf41d380f07a96b773cf2be5 # timestamp: 2017-03-30 13:51:17 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: squid3@treenet.co.nz-20170226110942-\ # 90rcwhx3fwa2l7is # # Begin patch === modified file 'src/client_side.cc' --- src/client_side.cc 2017-01-27 13:38:24 +0000 +++ src/client_side.cc 2017-03-30 13:31:22 +0000 @@ -4376,7 +4376,12 @@ fd_table[connState->clientConnection->fd].read_method = &default_read_method; fd_table[connState->clientConnection->fd].write_method = &default_write_method; + ClientSocketContext::Pointer context = connState->getCurrentContext(); + Must(context != NULL); if (connState->transparent()) { + // If we are going to fake the second CONNECT, clear the first one. + context->connIsFinished(); + // fake a CONNECT request to force connState to tunnel // XXX: copy from MemBuf reallocates, not a regression since old code did too SBuf temp; === modified file 'src/client_side_request.cc' --- src/client_side_request.cc 2017-02-25 05:50:14 +0000 +++ src/client_side_request.cc 2017-03-30 13:31:22 +0000 @@ -561,6 +561,7 @@ debugs(85, DBG_IMPORTANT, "SECURITY ALERT: on URL: " << urlCanonical(http->request)); // IP address validation for Host: failed. reject the connection. + http->getConn()->quitAfterError(http->request); clientStreamNode *node = (clientStreamNode *)http->client_stream.tail->prev->data; clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext);