From: Amos Jeffries Date: Sat, 11 Oct 2025 03:33:02 +0000 (+1300) Subject: Bug 3390: Proxy auth data visible to scripts (#2249) X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e7e9073a2435cc93b913553d147b497fda77c1ab;p=thirdparty%2Fsquid.git 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 --- diff --git a/doc/release-notes/release-7.sgml.in b/doc/release-notes/release-7.sgml.in index ef5c2f6875..304b750013 100644 --- 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: logformat

Removed %ui format code with Ident protocol support. + email_err_data +

Since Squid-7.2, the default for this directive is off. + external_acl_type

Removed %IDENT format code with Ident protocol support. diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index 0a9c01604f..c8810f1d58 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -340,14 +340,14 @@ HttpRequest::swapOut(StoreEntry * e) /* packs request-line and headers, appends 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 ada8e9720b..3088efbd1a 100644 --- 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 76570527c2..451d64b67d 100644 --- 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 %w + + 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 f031e8e584..3fb52ac9ce 100644 --- 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 4634c83ec2..87698fc52a 100644 --- 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 abca4a17d7..297b306978 100644 --- 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 495597d9a1..48a0f1ce03 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)