| From 0951a0681011dfca3d78c84fd7f1e19c78a4443f Mon Sep 17 00:00:00 2001 |
| From: Amos Jeffries <yadij@users.noreply.github.com> |
| Date: Sat, 11 Oct 2025 16:33:02 +1300 |
| Subject: [PATCH] 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> |
| |
| CVE: CVE-2025-62168 |
| Upstream: https://github.com/squid-cache/squid/commit/0951a0681011dfca3d78c84fd7f1e19c78a4443f |
| [thomas: remove release note, backport errorpage.cc] |
| Signed-off-by: Thomas Perale <thomas.perale@mind.be> |
| --- |
| src/HttpRequest.cc | 6 +++--- |
| src/HttpRequest.h | 2 +- |
| src/cf.data.pre | 8 +++++++- |
| src/client_side_reply.cc | 14 +++++++------- |
| src/errorpage.cc | 17 ++++------------- |
| src/errorpage.h | 1 - |
| src/tests/stub_HttpRequest.cc | 2 +- |
| 8 files changed, 26 insertions(+), 27 deletions(-) |
| |
| diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc |
| index cd7ee71d4af..c6ed5bee45d 100644 |
| --- a/src/HttpRequest.cc |
| +++ b/src/HttpRequest.cc |
| @@ -341,7 +341,7 @@ 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 */ |
| @@ -349,8 +349,8 @@ HttpRequest::pack(Packable * p) const |
| SQUIDSBUFPRINT(method.image()), SQUIDSBUFPRINT(url.path()), |
| http_ver.major, http_ver.minor); |
| /* 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 6d369029322..28dc4daf99d 100644 |
| --- a/src/HttpRequest.h |
| +++ b/src/HttpRequest.h |
| @@ -206,7 +206,7 @@ class HttpRequest: public Http::Message |
| |
| 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 0a73020e111..2dce65a4d0a 100644 |
| --- a/src/cf.data.pre |
| +++ b/src/cf.data.pre |
| @@ -8941,12 +8941,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 d73bf3f99f6..fc2feccf802 100644 |
| --- a/src/client_side_reply.cc |
| +++ b/src/client_side_reply.cc |
| @@ -94,7 +94,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 |
| @@ -104,9 +104,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 |
| @@ -995,11 +992,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 d7a588d099f..06046de9ebb 100644 |
| --- a/src/errorpage.cc |
| +++ b/src/errorpage.cc |
| @@ -792,7 +792,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); |
| @@ -850,7 +849,7 @@ ErrorState::Dump(MemBuf * mb) |
| SQUIDSBUFPRINT(request->url.path()), |
| AnyP::ProtocolType_str[request->http_ver.protocol], |
| request->http_ver.major, request->http_ver.minor); |
| - request->header.packInto(&str); |
| + request->header.packInto(&str, true /* hide authorization data */); |
| } |
| |
| str.append("\r\n", 2); |
| @@ -1112,18 +1111,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.path()), |
| - 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 abca4a17d7b..297b306978d 100644 |
| --- a/src/errorpage.h |
| +++ b/src/errorpage.h |
| @@ -194,7 +194,6 @@ class ErrorState |
| 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 495597d9a1b..48a0f1ce03e 100644 |
| --- 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) |