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

diff --git a/src/client_side.cc b/src/client_side.cc
index a9bd038035752f997a10cfe3cd21e95e2ded16e6..b6265735f40b60b0be335b595f322c2800d96741 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -4376,7 +4376,12 @@ void httpsSslBumpStep2AccessCheckDone(allow_t answer, void *data)
         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;

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index d65ba2c4c7fbe0d2cb9b0d46e206e462233150af..e75b757ab6b4d97468e86b701790becd6c2d03d8 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -561,6 +561,7 @@ ClientRequestContext::hostHeaderVerifyFailed(const char *A, const char *B)
     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<clientReplyContext *>(node->data.getRaw());
     assert (repContext);