Intelligent handling of CONNECT denials

Without authentication, bump-server-first CONNECT requests allow uncontrolled
SSL handhsakes with origin servers, which is not desirable if the proxy operatordoes not want to allow users to access external resources anonymously.
Authenticating CONNECT requests is troublesome because when CONNECT
authentication fails, the proxy has difficulties communicating details of the
error to the browser, due to security vulnerabilities discussed at
  https://bugzilla.mozilla.org/show_bug.cgi?id=479880

This patch implements the following logic to allow for seamless authentication
of CONNECT requests in a bump-server-first setup:

    - Process http_access. Authenticate CONNECT request if needed, which may
      require several HTTP CONNECT exchanges. This should be already supported.

    - If access is allowed, use Connect-To-Server-First (for bumped connections)      or normal TCP tunneling (for regular connections). This should be already       supported.

    - If access is denied, check ssl_bump and delay the error (for bumped
      connections) or serve the error immediately (for regular connections).
      This needs work.

"Delaying the error" in this context means remembering the error, responding
with 200 Established, establishing a bumped secure connection with the client,
not connecting to the origin server at all, and serving the error to the client
when the first encapsulated request comes.

diff --git a/src/ClientRequestContext.h b/src/ClientRequestContext.h
index 6290eaf9b293a02adfc902362bda0282352e5632..9abf326ce78ddd783931ed59ce56f098a38fd311 100644 (file)
--- a/src/ClientRequestContext.h
+++ b/src/ClientRequestContext.h
@@ -68,6 +68,8 @@ public:
 #if USE_SSL
     bool sslBumpCheckDone;
 #endif
+    ErrorState *error;
+    bool readNextRequest;
 
     /// Send authentication response (challenge or error) if ACL result indicates one is needed
     /// \return true if an error page of any kind has been sent back to the client.

diff --git a/src/client_side.cc b/src/client_side.cc
index c24364564a73727d35639bc1af9081822e36b787..f29464ec1ad3d712c2fa07abedf2cb4ca7197ddf 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3873,8 +3873,10 @@ ConnStateData::switchToHttps(const char *host, const int port)
     debugs(33, 5, HERE << "converting " << clientConnection << " to SSL");
 
     const bool alwaysBumpServerFirst = true;
-    if (alwaysBumpServerFirst) {
-        Must(!sslServerBump);
+    // If sslServerBump is set, then we have decided to deny CONNECT
+    // and now want to switch to SSL to send the error to the client
+    // without even peeking at the origin server certificate.
+    if (alwaysBumpServerFirst && !sslServerBump) {
         HttpRequest *fakeRequest = new HttpRequest;
         fakeRequest->flags.sslPeek = 1;
         fakeRequest->SetHost(sslConnectHostOrIp.termedBuf());

diff --git a/src/client_side.h b/src/client_side.h
index 62a0d2ab9b5e33831fc72ca300c0df0f3c935ac8..d3a14e54af335709588f2d26bb5ea59e73998abb 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -343,6 +343,7 @@ public:
     void switchToHttps(const char *host, const int port);
     bool switchedToHttps() const { return switchedToHttps_; }
     Ssl::ServerBump *serverBump() {return sslServerBump;}
+    void setServerBump(Ssl::ServerBump *srvBump) {if (!sslServerBump) sslServerBump = srvBump;}
     /// Fill the certAdaptParams with the required data for certificate adaptation
     /// and create the key for storing/retrieve the certificate to/from the cache
     void buildSslCertGenerationParams(Ssl::CertificateProperties &certProperties);

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 47b284e7f7cddaf2aabf6fd3b8625cd83a587e8c..90c0a8d9a4b5eaf04ebe771d793e284c577b251d 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -65,6 +65,7 @@
 #include "comm/Connection.h"
 #include "comm/Write.h"
 #include "compat/inet_pton.h"
+#include "errorpage.h"
 #include "fde.h"
 #include "format/Token.h"
 #include "HttpHdrCc.h"
@@ -79,6 +80,7 @@
 #include "err_detail_type.h"
 #if USE_SSL
 #include "ssl/support.h"
+#include "ssl/ServerBump.h"
 #endif
 
 
@@ -92,6 +94,8 @@ static const char *const crlf = "\r\n";
 static void clientFollowXForwardedForCheck(allow_t answer, void *data);
 #endif /* FOLLOW_X_FORWARDED_FOR */
 
+extern ErrorState *clientBuildError(err_type, http_status, char const *url, Ip::Address &, HttpRequest *);
+
 CBDATA_CLASS_INIT(ClientRequestContext);
 
 void *
@@ -135,10 +139,11 @@ ClientRequestContext::~ClientRequestContext()
     if (http)
         cbdataReferenceDone(http);
 
+    delete error;
     debugs(85,3, HERE << this << " ClientRequestContext destructed");
 }
 
-ClientRequestContext::ClientRequestContext(ClientHttpRequest *anHttp) : http(cbdataReference(anHttp)), acl_checklist (NULL), redirect_state (REDIRECT_NONE)
+ClientRequestContext::ClientRequestContext(ClientHttpRequest *anHttp) : http(cbdataReference(anHttp)), acl_checklist (NULL), redirect_state (REDIRECT_NONE), error(NULL), readNextRequest(false)
 {
     http_access_done = false;
     redirect_done = false;
@@ -815,26 +820,19 @@ ClientRequestContext::maybeSendAuthChallenge(const allow_t &answer)
             page_id = ERR_ACCESS_DENIED;
     }
 
-    clientStreamNode *node = (clientStreamNode *)http->client_stream.tail->prev->data;
-    clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
-    assert (repContext);
     Ip::Address tmpnoaddr;
     tmpnoaddr.SetNoAddr();
-    repContext->setReplyToError(page_id, status,
-                                http->request->method,
-                                NULL,
-                                http->getConn() != NULL ? http->getConn()->clientConnection->remote : tmpnoaddr,
-                                http->request,
-                                NULL,
-#if USE_AUTH
-                                http->getConn() != NULL && http->getConn()->auth_user_request != NULL ?
-                                http->getConn()->auth_user_request : http->request->auth_user_request);
-#else
-                                NULL);
-#endif
-    http->getConn()->flags.readMore = true; // resume any pipeline reads.
-    node = (clientStreamNode *)http->client_stream.tail->data;
-    clientStreamRead(node, http, node->readBuffer);
+    error = clientBuildError(page_id, status, 
+                             NULL,
+                             http->getConn() != NULL ? http->getConn()->clientConnection->remote : tmpnoaddr,
+                             http->request
+        );
+
+    error->auth_user_request = 
+        http->getConn() != NULL && http->getConn()->auth_user_request != NULL ?
+        http->getConn()->auth_user_request : http->request->auth_user_request;
+
+    readNextRequest = true;
     return true;
 }
 
@@ -847,9 +845,8 @@ ClientRequestContext::clientAccessCheckDone(const allow_t &answer)
            ", because it matched '" <<
            (AclMatchedName ? AclMatchedName : "NO ACL's") << "'" );
 
-    if (maybeSendAuthChallenge(answer))
-        return;
-
+    maybeSendAuthChallenge(answer);
+     
     /* ACCESS_ALLOWED (or auth in grace period ACCESS_AUTH_EXPIRED_OK) continues here ... */
     safe_free(http->uri);
 
@@ -895,10 +892,7 @@ clientRedirectAccessCheckDone(allow_t answer, void *data)
 {
     ClientRequestContext *context = (ClientRequestContext *)data;
 
-    if (context->maybeSendAuthChallenge(answer))
-        return;
-
-    if (answer == ACCESS_ALLOWED)
+    if (!context->maybeSendAuthChallenge(answer) && answer == ACCESS_ALLOWED)
         redirectStart(context->http, clientRedirectDoneWrapper, context);
     else
         context->clientRedirectDone(NULL);
@@ -1525,14 +1519,14 @@ ClientHttpRequest::doCallouts()
         calloutContext->http->al.request = HTTPMSGLOCK(request);
 
     // CVE-2009-0801: verify the Host: header is consistent with other known details.
-    if (!calloutContext->host_header_verify_done) {
+    if (!calloutContext->error && !calloutContext->host_header_verify_done) {
         debugs(83, 3, HERE << "Doing calloutContext->hostHeaderVerify()");
         calloutContext->host_header_verify_done = true;
         calloutContext->hostHeaderVerify();
         return;
     }
 
-    if (!calloutContext->http_access_done) {
+    if (!calloutContext->error && !calloutContext->http_access_done) {
         debugs(83, 3, HERE << "Doing calloutContext->clientAccessCheck()");
         calloutContext->http_access_done = true;
         calloutContext->clientAccessCheck();
@@ -1540,7 +1534,7 @@ ClientHttpRequest::doCallouts()
     }
 
 #if USE_ADAPTATION
-    if (!calloutContext->adaptation_acl_check_done) {
+    if (!calloutContext->error && !calloutContext->adaptation_acl_check_done) {
         calloutContext->adaptation_acl_check_done = true;
         if (Adaptation::AccessCheck::Start(
                     Adaptation::methodReqmod, Adaptation::pointPreCache,
@@ -1549,7 +1543,7 @@ ClientHttpRequest::doCallouts()
     }
 #endif
 
-    if (!calloutContext->redirect_done) {
+    if (!calloutContext->error && !calloutContext->redirect_done) {
         calloutContext->redirect_done = true;
         assert(calloutContext->redirect_state == REDIRECT_NONE);
 
@@ -1561,20 +1555,20 @@ ClientHttpRequest::doCallouts()
         }
     }
 
-    if (!calloutContext->adapted_http_access_done) {
+    if (!calloutContext->error && !calloutContext->adapted_http_access_done) {
         debugs(83, 3, HERE << "Doing calloutContext->clientAccessCheck2()");
         calloutContext->adapted_http_access_done = true;
         calloutContext->clientAccessCheck2();
         return;
     }
 
-    if (!calloutContext->interpreted_req_hdrs) {
+    if (!calloutContext->error && !calloutContext->interpreted_req_hdrs) {
         debugs(83, 3, HERE << "Doing clientInterpretRequestHeaders()");
         calloutContext->interpreted_req_hdrs = 1;
         clientInterpretRequestHeaders(this);
     }
 
-    if (!calloutContext->no_cache_done) {
+    if (!calloutContext->error && !calloutContext->no_cache_done) {
         calloutContext->no_cache_done = true;
 
         if (Config.accessList.noCache && request->flags.cachable) {
@@ -1609,6 +1603,8 @@ ClientHttpRequest::doCallouts()
     }
 
 #if USE_SSL
+    // We need to check for SSL bump even if the calloutContext->error is set
+    // because we are handling with different way the error inside SSL-bump
     if (!calloutContext->sslBumpCheckDone) {
         calloutContext->sslBumpCheckDone = true;
         if (calloutContext->sslBumpAccessCheck())
@@ -1617,6 +1613,38 @@ ClientHttpRequest::doCallouts()
     }
 #endif
 
+    if (calloutContext->error) {
+        const char *uri = urlCanonical(request);
+        StoreEntry *e= storeCreateEntry(uri, uri, request->flags, request->method);
+#if USE_SSL
+        if (sslBumpNeeded() && 
+            calloutContext->error->httpStatus != HTTP_PROXY_AUTHENTICATION_REQUIRED) {
+            // set final error but delay sending until we bump
+            Ssl::ServerBump *srvBump = new Ssl::ServerBump(request, e);
+            errorAppendEntry(e, calloutContext->error);
+            calloutContext->error = NULL;
+            getConn()->setServerBump(srvBump);
+            e->unlock();
+        } else 
+#endif
+        {
+            // send the error to the client
+            clientStreamNode *node = (clientStreamNode *)client_stream.tail->prev->data;
+            clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
+            assert (repContext);
+            repContext->setReplyToStoreEntry(e);
+            errorAppendEntry(e, calloutContext->error);
+            calloutContext->error = NULL;
+            if (calloutContext->readNextRequest)
+                getConn()->flags.readMore = true; // resume any pipeline reads.
+            node = (clientStreamNode *)client_stream.tail->data;
+            clientStreamRead(node, this, node->readBuffer);
+            e->unlock();
+            // Stop here.
+            return;
+        }
+    }
+
     cbdataReferenceDone(calloutContext->http);
     delete calloutContext;
     calloutContext = NULL;
@@ -1849,24 +1877,24 @@ ClientHttpRequest::handleAdaptationFailure(int errDetail, bool bypassable)
     // The original author of the code also wanted to pass an errno to
     // setReplyToError, but it seems unlikely that the errno reflects the
     // true cause of the error at this point, so I did not pass it.
-    Ip::Address noAddr;
-    noAddr.SetNoAddr();
-    ConnStateData * c = getConn();
-    repContext->setReplyToError(ERR_ICAP_FAILURE, HTTP_INTERNAL_SERVER_ERROR,
-                                request->method, NULL,
-                                (c != NULL ? c->clientConnection->remote : noAddr), request, NULL,
-#if USE_AUTH
-                                (c != NULL && c->auth_user_request != NULL ?
-                                 c->auth_user_request : request->auth_user_request));
-#else
-                                NULL);
-#endif
-
-    request->detailError(ERR_ICAP_FAILURE, errDetail);
-    c->flags.readMore = true;
-    c->expectNoForwarding();
-    node = (clientStreamNode *)client_stream.tail->data;
-    clientStreamRead(node, this, node->readBuffer);
+    if (calloutContext) {
+        Ip::Address noAddr;
+        noAddr.SetNoAddr();
+        ConnStateData * c = getConn();
+        calloutContext->error = clientBuildError(ERR_ICAP_FAILURE, HTTP_INTERNAL_SERVER_ERROR,
+                                 NULL,
+                                 c != NULL ? c->clientConnection->remote : noAddr,
+                                 request
+            );
+
+        calloutContext->error->auth_user_request = 
+            c != NULL && c->auth_user_request != NULL ? c->auth_user_request : request->auth_user_request;
+        request->detailError(ERR_ICAP_FAILURE, errDetail);
+        calloutContext->readNextRequest = true;
+        c->expectNoForwarding();
+        doCallouts();
+    }
+    //else if(calloutContext == NULL) is it possible?
 }
 
 #endif

diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc
index b2f85b3d2a17eaee8cd65b6a407ce7caaf108f9d..bac940f24a29d1732d3f9b576cfba7728fbc4617 100644 (file)
--- a/src/ssl/ServerBump.cc
+++ b/src/ssl/ServerBump.cc
@@ -16,13 +16,17 @@
 CBDATA_NAMESPACED_CLASS_INIT(Ssl, ServerBump);
 
 
-Ssl::ServerBump::ServerBump(HttpRequest *fakeRequest):
+Ssl::ServerBump::ServerBump(HttpRequest *fakeRequest, StoreEntry *e):
     request(fakeRequest),
     bumpSslErrorNoList(NULL)
 {
     debugs(33, 4, HERE << "will peek at " << request->GetHost() << ':' << request->port);
     const char *uri = urlCanonical(request);
-    entry = storeCreateEntry(uri, uri, request->flags, request->method);
+    if (e) {
+        entry = e;
+        entry->lock();
+    } else
+        entry = storeCreateEntry(uri, uri, request->flags, request->method);
     // We do not need to be a client because the error contents will be used
     // later, but an entry without any client will trim all its contents away.
     sc = storeClientListAdd(entry, this);

diff --git a/src/ssl/ServerBump.h b/src/ssl/ServerBump.h
index af41bfcdc273be20aae2164f20c6bbec58334306..df62acae61e90628f4fcba868acfea37fbda05f1 100644 (file)
--- a/src/ssl/ServerBump.h
+++ b/src/ssl/ServerBump.h
@@ -19,7 +19,7 @@ namespace Ssl
 class ServerBump
 {
 public:
-    explicit ServerBump(HttpRequest *fakeRequest);
+    explicit ServerBump(HttpRequest *fakeRequest, StoreEntry *e = NULL);
     ~ServerBump();
     /// faked, minimal request; required by server-side API
     HttpRequest::Pointer request;