|
| 1 | +From 0951a0681011dfca3d78c84fd7f1e19c78a4443f Mon Sep 17 00:00:00 2001 |
| 2 | +From: Amos Jeffries < [email protected]> |
| 3 | +Date: Sat, 11 Oct 2025 16:33:02 +1300 |
| 4 | +Subject: [PATCH] Bug 3390: Proxy auth data visible to scripts (#2249) |
| 5 | + |
| 6 | +Original changes to redact credentials from error page %R code |
| 7 | +expansion output was incomplete. It missed the parse failure |
| 8 | +case where ErrorState::request_hdrs raw buffer contained |
| 9 | +sensitive information. |
| 10 | + |
| 11 | +Also missed was the %W case where full request message headers |
| 12 | +were generated in a mailto link. This case is especially |
| 13 | +problematic as it may be delivered over insecure SMTP even if |
| 14 | +the error was secured with HTTPS. |
| 15 | + |
| 16 | +After this change: |
| 17 | +* The HttpRequest message packing code for error pages is de-duplicated |
| 18 | + and elides authentication headers for both %R and %W code outputs. |
| 19 | +* The %R code output includes the CRLF request message terminator. |
| 20 | +* The email_err_data directive causing advanced details to be added to |
| 21 | + %W mailto links is disabled by default. |
| 22 | + |
| 23 | +Also redact credentials from generated TRACE responses. |
| 24 | + |
| 25 | +--------- |
| 26 | + |
| 27 | +Co-authored-by: Alex Rousskov < [email protected]> |
| 28 | + |
| 29 | +Modified to apply to AzureLinux |
| 30 | +Upstream Patch Reference: https://github.com/squid-cache/squid/commit/0951a0681011dfca3d78c84fd7f1e19c78a4443f.patch |
| 31 | +--- |
| 32 | + src/HttpRequest.cc | 6 +++--- |
| 33 | + src/HttpRequest.h | 2 +- |
| 34 | + src/cf.data.pre | 8 +++++++- |
| 35 | + src/client_side_reply.cc | 14 +++++++------- |
| 36 | + src/errorpage.cc | 22 ++++------------------ |
| 37 | + src/errorpage.h | 1 - |
| 38 | + src/tests/stub_HttpRequest.cc | 2 +- |
| 39 | + 7 files changed, 23 insertions(+), 32 deletions(-) |
| 40 | + |
| 41 | +diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc |
| 42 | +index 513f5e9..b374ec3 100644 |
| 43 | +--- a/src/HttpRequest.cc |
| 44 | ++++ b/src/HttpRequest.cc |
| 45 | +@@ -341,7 +341,7 @@ HttpRequest::swapOut(StoreEntry * e) |
| 46 | + |
| 47 | + /* packs request-line and headers, appends <crlf> terminator */ |
| 48 | + void |
| 49 | +-HttpRequest::pack(Packable * p) const |
| 50 | ++HttpRequest::pack(Packable * const p, const bool maskSensitiveInfo) const |
| 51 | + { |
| 52 | + assert(p); |
| 53 | + /* pack request-line */ |
| 54 | +@@ -349,8 +349,8 @@ HttpRequest::pack(Packable * p) const |
| 55 | + SQUIDSBUFPRINT(method.image()), SQUIDSBUFPRINT(url.path()), |
| 56 | + http_ver.major, http_ver.minor); |
| 57 | + /* headers */ |
| 58 | +- header.packInto(p); |
| 59 | +- /* trailer */ |
| 60 | ++ header.packInto(p, maskSensitiveInfo); |
| 61 | ++ /* indicate the end of the header section */ |
| 62 | + p->append("\r\n", 2); |
| 63 | + } |
| 64 | + |
| 65 | +diff --git a/src/HttpRequest.h b/src/HttpRequest.h |
| 66 | +index bf27729..baf8427 100644 |
| 67 | +--- a/src/HttpRequest.h |
| 68 | ++++ b/src/HttpRequest.h |
| 69 | +@@ -206,7 +206,7 @@ public: |
| 70 | + |
| 71 | + void swapOut(StoreEntry * e); |
| 72 | + |
| 73 | +- void pack(Packable * p) const; |
| 74 | ++ void pack(Packable * p, bool maskSensitiveInfo = false) const; |
| 75 | + |
| 76 | + static void httpRequestPack(void *obj, Packable *p); |
| 77 | + |
| 78 | +diff --git a/src/cf.data.pre b/src/cf.data.pre |
| 79 | +index 78a73a3..d9b3d2e 100644 |
| 80 | +--- a/src/cf.data.pre |
| 81 | ++++ b/src/cf.data.pre |
| 82 | +@@ -8941,12 +8941,18 @@ NAME: email_err_data |
| 83 | + COMMENT: on|off |
| 84 | + TYPE: onoff |
| 85 | + LOC: Config.onoff.emailErrData |
| 86 | +-DEFAULT: on |
| 87 | ++DEFAULT: off |
| 88 | + DOC_START |
| 89 | + If enabled, information about the occurred error will be |
| 90 | + included in the mailto links of the ERR pages (if %W is set) |
| 91 | + so that the email body contains the data. |
| 92 | + Syntax is <A HREF="mailto:%w%W">%w</A> |
| 93 | ++ |
| 94 | ++ SECURITY WARNING: |
| 95 | ++ Request headers and other included facts may contain |
| 96 | ++ sensitive information about transaction history, the |
| 97 | ++ Squid instance, and its environment which would be |
| 98 | ++ unavailable to error recipients otherwise. |
| 99 | + DOC_END |
| 100 | + |
| 101 | + NAME: deny_info |
| 102 | +diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc |
| 103 | +index 601e8bd..427f5a9 100644 |
| 104 | +--- a/src/client_side_reply.cc |
| 105 | ++++ b/src/client_side_reply.cc |
| 106 | +@@ -94,7 +94,7 @@ clientReplyContext::clientReplyContext(ClientHttpRequest *clientContext) : |
| 107 | + void |
| 108 | + clientReplyContext::setReplyToError( |
| 109 | + err_type err, Http::StatusCode status, char const *uri, |
| 110 | +- const ConnStateData *conn, HttpRequest *failedrequest, const char *unparsedrequest, |
| 111 | ++ const ConnStateData *conn, HttpRequest *failedrequest, const char *, |
| 112 | + #if USE_AUTH |
| 113 | + Auth::UserRequest::Pointer auth_user_request |
| 114 | + #else |
| 115 | +@@ -104,9 +104,6 @@ clientReplyContext::setReplyToError( |
| 116 | + { |
| 117 | + auto errstate = clientBuildError(err, status, uri, conn, failedrequest, http->al); |
| 118 | + |
| 119 | +- if (unparsedrequest) |
| 120 | +- errstate->request_hdrs = xstrdup(unparsedrequest); |
| 121 | +- |
| 122 | + #if USE_AUTH |
| 123 | + errstate->auth_user_request = auth_user_request; |
| 124 | + #endif |
| 125 | +@@ -995,11 +992,14 @@ clientReplyContext::traceReply() |
| 126 | + triggerInitialStoreRead(); |
| 127 | + http->storeEntry()->releaseRequest(); |
| 128 | + http->storeEntry()->buffer(); |
| 129 | ++ MemBuf content; |
| 130 | ++ content.init(); |
| 131 | ++ http->request->pack(&content, true /* hide authorization data */); |
| 132 | + const HttpReplyPointer rep(new HttpReply); |
| 133 | +- rep->setHeaders(Http::scOkay, nullptr, "text/plain", http->request->prefixLen(), 0, squid_curtime); |
| 134 | ++ rep->setHeaders(Http::scOkay, nullptr, "message/http", content.contentSize(), 0, squid_curtime); |
| 135 | ++ rep->body.set(SBuf(content.buf, content.size)); |
| 136 | + http->storeEntry()->replaceHttpReply(rep); |
| 137 | +- http->request->swapOut(http->storeEntry()); |
| 138 | +- http->storeEntry()->complete(); |
| 139 | ++ http->storeEntry()->completeSuccessfully("traceReply() stored the entire response"); |
| 140 | + } |
| 141 | + |
| 142 | + #define SENDING_BODY 0 |
| 143 | +diff --git a/src/errorpage.cc b/src/errorpage.cc |
| 144 | +index 4a24bf1..a33bd40 100644 |
| 145 | +--- a/src/errorpage.cc |
| 146 | ++++ b/src/errorpage.cc |
| 147 | +@@ -792,7 +792,6 @@ ErrorState::~ErrorState() |
| 148 | + { |
| 149 | + safe_free(redirect_url); |
| 150 | + safe_free(url); |
| 151 | +- safe_free(request_hdrs); |
| 152 | + wordlistDestroy(&ftp.server_msg); |
| 153 | + safe_free(ftp.request); |
| 154 | + safe_free(ftp.reply); |
| 155 | +@@ -845,12 +844,7 @@ ErrorState::Dump(MemBuf * mb) |
| 156 | + /* - HTTP stuff */ |
| 157 | + str.append("HTTP Request:\r\n", 15); |
| 158 | + if (request) { |
| 159 | +- str.appendf(SQUIDSBUFPH " " SQUIDSBUFPH " %s/%d.%d\n", |
| 160 | +- SQUIDSBUFPRINT(request->method.image()), |
| 161 | +- SQUIDSBUFPRINT(request->url.path()), |
| 162 | +- AnyP::ProtocolType_str[request->http_ver.protocol], |
| 163 | +- request->http_ver.major, request->http_ver.minor); |
| 164 | +- request->header.packInto(&str); |
| 165 | ++ request->pack(&str, true /* hide authorization data */); |
| 166 | + } |
| 167 | + |
| 168 | + str.append("\r\n", 2); |
| 169 | +@@ -1112,18 +1106,10 @@ ErrorState::compileLegacyCode(Build &build) |
| 170 | + p = "[no request]"; |
| 171 | + break; |
| 172 | + } |
| 173 | +- if (request) { |
| 174 | +- mb.appendf(SQUIDSBUFPH " " SQUIDSBUFPH " %s/%d.%d\n", |
| 175 | +- SQUIDSBUFPRINT(request->method.image()), |
| 176 | +- SQUIDSBUFPRINT(request->url.path()), |
| 177 | +- AnyP::ProtocolType_str[request->http_ver.protocol], |
| 178 | +- request->http_ver.major, request->http_ver.minor); |
| 179 | +- request->header.packInto(&mb, true); //hide authorization data |
| 180 | +- } else if (request_hdrs) { |
| 181 | +- p = request_hdrs; |
| 182 | +- } else { |
| 183 | ++ else if (request) |
| 184 | ++ request->pack(&mb, true /* hide authorization data */); |
| 185 | ++ else |
| 186 | + p = "[no request]"; |
| 187 | +- } |
| 188 | + break; |
| 189 | + |
| 190 | + case 's': |
| 191 | +diff --git a/src/errorpage.h b/src/errorpage.h |
| 192 | +index cf15949..56fe5ef 100644 |
| 193 | +--- a/src/errorpage.h |
| 194 | ++++ b/src/errorpage.h |
| 195 | +@@ -194,7 +194,6 @@ public: |
| 196 | + MemBuf *listing = nullptr; |
| 197 | + } ftp; |
| 198 | + |
| 199 | +- char *request_hdrs = nullptr; |
| 200 | + char *err_msg = nullptr; /* Preformatted error message from the cache */ |
| 201 | + |
| 202 | + AccessLogEntryPointer ale; ///< transaction details (or nil) |
| 203 | +diff --git a/src/tests/stub_HttpRequest.cc b/src/tests/stub_HttpRequest.cc |
| 204 | +index ab03fa2..d8d3f10 100644 |
| 205 | +--- a/src/tests/stub_HttpRequest.cc |
| 206 | ++++ b/src/tests/stub_HttpRequest.cc |
| 207 | +@@ -45,7 +45,7 @@ bool HttpRequest::expectingBody(const HttpRequestMethod &, int64_t &) const STUB |
| 208 | + bool HttpRequest::bodyNibbled() const STUB_RETVAL(false) |
| 209 | + int HttpRequest::prefixLen() const STUB_RETVAL(0) |
| 210 | + void HttpRequest::swapOut(StoreEntry *) STUB |
| 211 | +-void HttpRequest::pack(Packable *) const STUB |
| 212 | ++void HttpRequest::pack(Packable *, bool) const STUB |
| 213 | + void HttpRequest::httpRequestPack(void *, Packable *) STUB |
| 214 | + HttpRequest * HttpRequest::FromUrl(const SBuf &, const MasterXaction::Pointer &, const HttpRequestMethod &) STUB_RETVAL(nullptr) |
| 215 | + HttpRequest * HttpRequest::FromUrlXXX(const char *, const MasterXaction::Pointer &, const HttpRequestMethod &) STUB_RETVAL(nullptr) |
| 216 | +-- |
| 217 | +2.43.0 |
| 218 | + |
0 commit comments