Bug 3390: Proxy auth data visible to scripts (#2249)

Original changes to redact credentials from error page %R code
expansion output was incomplete. It missed the parse failure
case where ErrorState::request_hdrs raw buffer contained
sensitive information.

Also missed was the %W case where full request message headers
were generated in a mailto link. This case is especially
problematic as it may be delivered over insecure SMTP even if
the error was secured with HTTPS.

After this change:
* The HttpRequest message packing code for error pages is de-duplicated
  and elides authentication headers for both %R and %W code outputs.
* The %R code output includes the CRLF request message terminator.
* The email_err_data directive causing advanced details to be added to
  %W mailto links is disabled by default.

Also redact credentials from generated TRACE responses.

---------

Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>

diff --git a/doc/release-notes/release-7.sgml.in b/doc/release-notes/release-7.sgml.in
index ef5c2f68753f95a4b6d20329a2356c51e743b721..304b7500138c3c9031d1384a503dbc7878ea1945 100644 (file)
--- a/doc/release-notes/release-7.sgml.in
+++ b/doc/release-notes/release-7.sgml.in
@@ -195,6 +195,9 @@ This section gives an account of those changes in three categories:
        <tag>logformat</tag>
        <p>Removed <em>%ui</em> format code with Ident protocol support.
 
+       <tag>email_err_data</tag>
+       <p>Since Squid-7.2, the default for this directive is <em>off</em>.
+
        <tag>external_acl_type</tag>
        <p>Removed <em>%IDENT</em> format code with Ident protocol support.
 

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index 0a9c01604f50fdd178f85135789e29af6c881aef..c8810f1d58a9d6a03568415605b81631d42f45b0 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -340,14 +340,14 @@ HttpRequest::swapOut(StoreEntry * e)
 
 /* packs request-line and headers, appends <crlf> terminator */
 void
-HttpRequest::pack(Packable * p) const
+HttpRequest::pack(Packable * const p, const bool maskSensitiveInfo) const
 {
     assert(p);
     /* pack request-line */
     packFirstLineInto(p, false /* origin-form */);
     /* headers */
-    header.packInto(p);
-    /* trailer */
+    header.packInto(p, maskSensitiveInfo);
+    /* indicate the end of the header section */
     p->append("\r\n", 2);
 }
 

diff --git a/src/HttpRequest.h b/src/HttpRequest.h
index ada8e9720b7aae7ca77ef90605cd888acb1227b4..3088efbd1a7f974906256039168cc50bc68c7a0f 100644 (file)
--- a/src/HttpRequest.h
+++ b/src/HttpRequest.h
@@ -204,7 +204,7 @@ public:
 
     void swapOut(StoreEntry * e);
 
-    void pack(Packable * p) const;
+    void pack(Packable * p, bool maskSensitiveInfo = false) const;
 
     static void httpRequestPack(void *obj, Packable *p);
 

diff --git a/src/cf.data.pre b/src/cf.data.pre
index 76570527c2684e072bd430185e042565707a27fc..451d64b67dc51baedaa724184f2a60f8012fef87 100644 (file)
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -8903,12 +8903,18 @@ NAME: email_err_data
 COMMENT: on|off
 TYPE: onoff
 LOC: Config.onoff.emailErrData
-DEFAULT: on
+DEFAULT: off
 DOC_START
        If enabled, information about the occurred error will be
        included in the mailto links of the ERR pages (if %W is set)
        so that the email body contains the data.
        Syntax is <A HREF="mailto:%w%W">%w</A>
+
+       SECURITY WARNING:
+               Request headers and other included facts may contain
+               sensitive information about transaction history, the
+               Squid instance, and its environment which would be
+               unavailable to error recipients otherwise.
 DOC_END
 
 NAME: deny_info

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index f031e8e584f73a6224a9550ba50ef00f656e5bb4..3fb52ac9ce32697eb7a703b8a97789ef590cc04e 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -93,7 +93,7 @@ clientReplyContext::clientReplyContext(ClientHttpRequest *clientContext) :
 void
 clientReplyContext::setReplyToError(
     err_type err, Http::StatusCode status, char const *uri,
-    const ConnStateData *conn, HttpRequest *failedrequest, const char *unparsedrequest,
+    const ConnStateData *conn, HttpRequest *failedrequest, const char *,
 #if USE_AUTH
     Auth::UserRequest::Pointer auth_user_request
 #else
@@ -103,9 +103,6 @@ clientReplyContext::setReplyToError(
 {
     auto errstate = clientBuildError(err, status, uri, conn, failedrequest, http->al);
 
-    if (unparsedrequest)
-        errstate->request_hdrs = xstrdup(unparsedrequest);
-
 #if USE_AUTH
     errstate->auth_user_request = auth_user_request;
 #endif
@@ -1005,11 +1002,14 @@ clientReplyContext::traceReply()
     triggerInitialStoreRead();
     http->storeEntry()->releaseRequest();
     http->storeEntry()->buffer();
+    MemBuf content;
+    content.init();
+    http->request->pack(&content, true /* hide authorization data */);
     const HttpReplyPointer rep(new HttpReply);
-    rep->setHeaders(Http::scOkay, nullptr, "text/plain", http->request->prefixLen(), 0, squid_curtime);
+    rep->setHeaders(Http::scOkay, nullptr, "message/http", content.contentSize(), 0, squid_curtime);
+    rep->body.set(SBuf(content.buf, content.size));
     http->storeEntry()->replaceHttpReply(rep);
-    http->request->swapOut(http->storeEntry());
-    http->storeEntry()->complete();
+    http->storeEntry()->completeSuccessfully("traceReply() stored the entire response");
 }
 
 #define SENDING_BODY 0

diff --git a/src/errorpage.cc b/src/errorpage.cc
index 4634c83ec2fad98d521087dc6158bf602ac54f9e..87698fc52a262b9588064ebbedb6fbf28efb57a2 100644 (file)
--- a/src/errorpage.cc
+++ b/src/errorpage.cc
@@ -837,7 +837,6 @@ ErrorState::~ErrorState()
 
     safe_free(redirect_url);
     safe_free(url);
-    safe_free(request_hdrs);
     wordlistDestroy(&ftp.server_msg);
     safe_free(ftp.request);
     safe_free(ftp.reply);
@@ -887,7 +886,7 @@ ErrorState::Dump(MemBuf * mb)
         body << "HTTP Request:\r\n";
         MemBuf r;
         r.init();
-        request->pack(&r);
+        request->pack(&r, true /* hide authorization data */);
         body << r.content();
     }
 
@@ -1149,18 +1148,10 @@ ErrorState::compileLegacyCode(Build &build)
                 p = "[no request]";
             break;
         }
-        if (request) {
-            mb.appendf(SQUIDSBUFPH " " SQUIDSBUFPH " %s/%d.%d\n",
-                       SQUIDSBUFPRINT(request->method.image()),
-                       SQUIDSBUFPRINT(request->url.originForm()),
-                       AnyP::ProtocolType_str[request->http_ver.protocol],
-                       request->http_ver.major, request->http_ver.minor);
-            request->header.packInto(&mb, true); //hide authorization data
-        } else if (request_hdrs) {
-            p = request_hdrs;
-        } else {
+        else if (request)
+            request->pack(&mb, true /* hide authorization data */);
+        else
             p = "[no request]";
-        }
         break;
 
     case 's':

diff --git a/src/errorpage.h b/src/errorpage.h
index abca4a17d7b4c8b30a08acedccf72a20647f1ebb..297b306978d53ee39aff6a85b778b920d3bb9357 100644 (file)
--- a/src/errorpage.h
+++ b/src/errorpage.h
@@ -193,7 +193,6 @@ public:
         MemBuf *listing = nullptr;
     } ftp;
 
-    char *request_hdrs = nullptr;
     char *err_msg = nullptr; /* Preformatted error message from the cache */
 
     AccessLogEntryPointer ale; ///< transaction details (or nil)

diff --git a/src/tests/stub_HttpRequest.cc b/src/tests/stub_HttpRequest.cc
index 495597d9a1bb7359de0c13ef007b1f4f0c22c6f9..48a0f1ce03e09b20d5a145d57b10e7a719b0b7fc 100644 (file)
--- a/src/tests/stub_HttpRequest.cc
+++ b/src/tests/stub_HttpRequest.cc
@@ -45,7 +45,7 @@ bool HttpRequest::expectingBody(const HttpRequestMethod &, int64_t &) const STUB
 bool HttpRequest::bodyNibbled() const STUB_RETVAL(false)
 int HttpRequest::prefixLen() const STUB_RETVAL(0)
 void HttpRequest::swapOut(StoreEntry *) STUB
-void HttpRequest::pack(Packable *) const STUB
+void HttpRequest::pack(Packable *, bool) const STUB
 void HttpRequest::httpRequestPack(void *, Packable *) STUB
 HttpRequest * HttpRequest::FromUrl(const SBuf &, const MasterXaction::Pointer &, const HttpRequestMethod &) STUB_RETVAL(nullptr)
 HttpRequest * HttpRequest::FromUrlXXX(const char *, const MasterXaction::Pointer &, const HttpRequestMethod &) STUB_RETVAL(nullptr)