Do not expand errorpage %codes injected by X509 certificates by rousskov · Pull Request #2416 · squid-cache/squid

rousskov · 2026-04-28T17:25:18Z

When available, Security::ErrorDetail is added to Squid-generated errors
that use customizable errorpage formats containing %D errorpage
%code. Security::ErrorDetail itself supports customizable verbose
reporting format (see detail in errors/templates/error-details.txt).
The latter format may contain both Security::ErrorDetail-specific %codes
and generic legacy errorpage %codes handled by ErrorState.

To support the above "nested" formats when processing %D,
ErrorState::compileLegacyCode() compiled ErrorDetail output,
substituting any legacy errorpage %codes outputted by ErrorDetail. That
re-compilation could not distinguish a %code sequence configured by
error-details.txt from a %code sequence that came from, say, a
received X509 certificate field. Both would be expanded!

This bug applies to both legacy errorpage %code sequences like %R
and modern @Squid{logformat %code} errorpage sequences.

X509 certificate fields are the only known injection vector, but it is
conceivable that some other error details may contain character
sequences that match errorpage legacy %codes (e.g., SysErrorDetail
returns strerror(3) output that Squid does not control or escape.

This bug is similar to the bug fixed in 2018 commit 6feeb15 but deals
with injected errorpage %codes (that target Squid error response
assembling code) rather than injected HTML tags (that target browsers).

Two primary solution candidates were considered:

A) Escape-then-compile-to-unescape: Security::ErrorDetail could escape
%code sequences received from "external" sources, so that
ErrorState::compile() would see %%X and replace that with %X,
correctly relaying received %X. This solution was rejected because
we risk forgetting to escape some input, now or during code
refactoring, especially since the risk applies to all ErrorDetail
classes. Also, escaping what we know we will immediately un-escape is
wasteful.

B) Never compile "external" input. This implemented solution required
giving ErrorDetail::verbose() access to the %code compiling code in
ErrorState, so that ErrorDetail can decide what to compile and what
not to. Now, Security::ErrorDetail only compiles %code sequences
found in detail; ErrorState does not compile ErrorDetail output.

Side effects

The details field in error-details.txt now supports modern
@Squid{logformat %code} errorpage sequences by design rather than by
accident, addressing an old (and essentially misleading) TODO inside
Security::ErrorDetail::convertErrorCodeToDescription().

The descr field in error-details.txt no longer supports errorpage
%code sequences. It was never documented to support them, and it did
not support Security::ErrorDetail %codes like %ssl_ca_name. The field is
not meant to contain %code sequences -- they belong to the details
field that specifies detail reporting format. Hopefully, no deployed
configurations use %code sequences in the descr field.

This WIP commit records a failed attempt to solve the problem by exposing ErrorState::compileLeadingCode() code to Security::ErrorDetail. This code does not compile, but that is easy to fix. The primary changes committed here already show many XXXs that would be difficult to address. I hope to find a better solution. The problem description below should still apply, and even the high-level solution analysis below may apply. ---- When available, Security::ErrorDetail is added to Squid-generated errors that use customizable errorpage formats containing `%D` errorpage `%code`. Security::ErrorDetail itself supports customizable verbose reporting format (see `detail` in `errors/templates/error-details.txt`). ErrorDetail format may contain both ErrorDetail-specific %codes and generic legacy errorpage %codes handled by ErrorState. To support the above "nested" formats when processing `%D`, ErrorState::compileLegacyCode() compiled ErrorDetail output, substituting any legacy errorpage %codes outputted by ErrorDetail. That re-compilation could not distinguish a `%code` sequence configured by `error-details.txt` from a `%code` sequence that came from, say, a received X509 certificate field. Both would be expanded! This bug applies to both legacy errorpage `%code` sequences like `%R` and modern `@Squid{logformat %code}` errorpage sequences. X509 certificate fields are the only known injection vector, but it is conceivable that some other error details may contain character sequences that match errorpage legacy %codes (e.g., SysErrorDetail returns strerror(3) output that Squid does not control and did not escape. This bug is similar to a bug fixed in 2018 commit 6feeb15, but this one deals with injected errorpage %codes rather than HTML tags. Two primary solution candidates were considered: A) Escape-then-compile-to-unescape: Security::ErrorDetail could escape `%code` sequences received from "external" sources, so that ErrorState::compile() would see `%%X` and replace that with `%X`, correctly relaying received `%X`. This solution was rejected because we risk forgetting to escape some input, now or during code refactoring, especially since the risk applies to all ErrorDetail classes. Also, escaping what we know we will immediately un-escape is wasteful. B) Never compile "external" input. This implemented solution required giving ErrorDetail::verbose() access to the `%code` compiling code in ErrorState, so that ErrorDetail can decide what to compile and what not to. Now, Security::ErrorDetail only compiles `%code` sequences found in `detail`; ErrorState does not compile ErrorDetail output. Side effects ------------ The `details` field in `error-details.txt` now supports modern `@Squid{logformat %code}` errorpage sequences, addressing an old TODO. The `descr` field in `error-details.txt` no longer supports `%code` sequences. It was never documented to support them. It is not meant to contain such sequences -- they belong to the `details` field. Hopefully, no deployed configurations use `%code` sequences in that field.

Removed the compilation loop from Security::ErrorDetail::verbose(). The old ErrorState::compile() loop is now responsible for calling Security::ErrorDetail for parsing custom %codes. Besides out-of-scope legacy const-correctness problems and basic build fixes, we need to resolve one new API problem: An ErrorDetail::verbose() caller supplies build.output, but the method returns its output instead of updating the supplied field. The solution may affect verbose() API, so it blocks those "basic build fixes".

ErrorDetail::verbose() does not need access to Build. Unlike internal ErrorPage methods, current ErrorDetail objects do not need to parse portions of error templates, engaging multiple portion-specific parsers. ErrorDetail::verbose() API is much simpler -- "dump details" (into the returned SBuf value). The only wrinkle here is that Security::ErrorDetail needs ErrorState compilation abilities to do that, but that can be handled by adding an ErrorState parameter. And since ErrorState objects already have `request` data members, we can drop the existing HttpRequest parameter. TODO: ErrorDetail::verbose() may benefit from switching to an std::ostream-based API, but doing so now would increase out-of-scope noise. It is probably best done together with upgrading ErrorState "compilation" methods to use std::ostream.

ld: errorpage.o: warning: relocation against `err_type_str' in read-only section `.text' src/errorpage.cc:276: undefined reference to `err_type_str' src/errorpage.cc:631: undefined reference to `err_type_str' ... Branch changes added forward declarations for classes declared inside an ErrorPage namespace. The AWK script incorrectly applied that namespace to the generated err_type_str definition. This surgical fix does not cover all possible namespace-related variations. The correct fix is to stop parsing C++ using AWK, at least as far as namespaces are concerned: The script caller knows what namespace(s) must be used for the container definition.

... to verbose() methods that did not use HttpRequest.

rousskov · 2026-04-28T17:31:46Z

The work on this fix was triggered by @jro-calif report implications.

Here is a diff between Squid error response generated by official and PR code (when the origin server is using a bogus x509 certificate field):

<pre>[No Error] (TLS code: SQUID_X509_V_ERR_DOMAIN_MISMATCH+broken_cert)</pre>
- <p>Certificate does not match domainname: /OU=28/Apr/2026:10:28:39 -0400s.%03tu      0 squid/8.0.0-VCSsl_ca_name %&gt;a ... %&lt;a [not available]t/O=O1</p>
+ <p>Certificate does not match domainname: /OU=%ts.%03tu @Squid{%6tr} %ssl_ca_name %&gt;a ... %&lt;a %mt/O=O1</p>

rousskov

These comments do not request any PR changes.

rousskov · 2026-04-28T17:51:44Z


 SBuf
-Ftp::ErrorDetail::verbose(const HttpRequest::Pointer &) const
+Ftp::ErrorDetail::verbose(const ErrorTemplateCompiler &) const


It is possible to avoid noisy changes like this one in this PR by adding a temporary API shim. I have not done that because these changes do not impact v7 backporting in my quick-and-dirty tests (cherry-picking fails because v7 lacks some src/security/ErrorDetail.cc changes in master/v8), and because we would have to post another official PR to remove that shim, of course.

rousskov · 2026-04-28T17:53:01Z

+
+        for (const auto &detail: request->error.details) {
+            mb.appendf("%i-Error-Detail-Brief: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->brief()));
+            mb.appendf("%i-Error-Detail-Verbose: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->verbose(*err)));


Disclaimer: I have not tested this FTP code path.

Please do. It should be just a matter of running a few FTP requests to a non-FTP server and looking at the cache.log level 11,2 entries for those messages.

kinkie

LGTM, also looks cleaner than current state

rousskov · 2026-04-29T13:35:58Z

+    // below, adjust compile*() methods to avoid ErrorState modifications.
+
    auto blockStart = build.input;
    while (const auto letter = *build.input) {


Francesco: also looks cleaner than current state

Yes, albeit slightly. This change removes one (poor) duplicate of this "find and replace all %codes" loop. It also provides better access to detail-rendering context via the ErrorTemplateCompiler parameter; I expect future code to use that access while rendering additional error details.

A lot of work is still required to modernize legacy errorpage code, of course, including addressing const-correctness problems marked in this PR.

Looks can be deceiving. The order of refactoring is causing regressions and an increase in technical debt.
A better approach would be to separate the refactoring from the bug fix. The former needs a lot more work, the latter can accept workarounds (wit TODO notes) for design issues.

When available, Security::ErrorDetail is added to Squid-generated errors that use customizable errorpage formats containing `%D` errorpage `%code`. Security::ErrorDetail itself supports customizable verbose reporting format (see `detail` in `errors/templates/error-details.txt`). The latter format may contain both Security::ErrorDetail-specific %codes and generic legacy errorpage %codes handled by ErrorState. To support the above "nested" formats when processing `%D`, ErrorState::compileLegacyCode() compiled ErrorDetail output, substituting any legacy errorpage %codes outputted by ErrorDetail. That re-compilation could not distinguish a `%code` sequence configured by `error-details.txt` from a `%code` sequence that came from, say, a received X509 certificate field. Both would be expanded! This bug applies to both legacy errorpage `%code` sequences like `%R` and modern `@Squid{logformat %code}` errorpage sequences. X509 certificate fields are the only known injection vector, but it is conceivable that some other error details may contain character sequences that match errorpage legacy %codes (e.g., SysErrorDetail returns strerror(3) output that Squid does not control or escape. This bug is similar to the bug fixed in 2018 commit 6feeb15 but deals with injected errorpage %codes (that target Squid error response assembling code) rather than injected HTML tags (that target browsers). Two primary solution candidates were considered: A) Escape-then-compile-to-unescape: Security::ErrorDetail could escape `%code` sequences received from "external" sources, so that ErrorState::compile() would see `%%X` and replace that with `%X`, correctly relaying received `%X`. This solution was rejected because we risk forgetting to escape some input, now or during code refactoring, especially since the risk applies to all ErrorDetail classes. Also, escaping what we know we will immediately un-escape is wasteful. B) Never compile "external" input. This implemented solution required giving ErrorDetail::verbose() access to the `%code` compiling code in ErrorState, so that ErrorDetail can decide what to compile and what not to. Now, Security::ErrorDetail only compiles `%code` sequences found in `detail`; ErrorState does not compile ErrorDetail output. Side effects ------------ The `details` field in `error-details.txt` now supports modern `@Squid{logformat %code}` errorpage sequences by design rather than by accident, addressing an old (and essentially misleading) TODO inside Security::ErrorDetail::convertErrorCodeToDescription(). The `descr` field in `error-details.txt` no longer supports errorpage `%code` sequences. It was never documented to support them, and it did not support Security::ErrorDetail %codes like %ssl_ca_name. The field is not meant to contain %code sequences -- they belong to the `details` field that specifies detail reporting format. Hopefully, no deployed configurations use `%code` sequences in the `descr` field.

yadij · 2026-05-04T03:13:07Z

 class ErrorState;

+namespace ErrorPage {
+class PercentCodeCompiler;


Why "PercentCode" ? we dont have other types of page compiler for error pages.

Suggested change

class PercentCodeCompiler;

class ErrorPageCompiler;

I do not see the required .h/.cc being added for this new class. Please keep the components in separate build units to avoid monolithic unit and dependency loop issues.

yadij · 2026-05-04T03:14:54Z

 {
 public:
    using Pointer = ErrorDetailPointer;
+    using ErrorTemplateCompiler = ErrorState;


"Error" word is redundant within the scope ErrorPage::

Suggested change

using ErrorTemplateCompiler = ErrorState;

using TemplateCompiler = ErrorState;

yadij · 2026-05-04T03:28:23Z

+    /// \sa compileDetail()
    SBuf compile(const char *input, bool building_deny_info_url, bool allowRecursion);

+    void compile(Build &build) const;


Redundant parameter and missing documentation on new method.

Suggested change

void compile(Build &build) const;

void compile(Build &) const;

yadij · 2026-05-04T03:41:01Z

+{
+    Assure(build.input);
+
+    // TODO: Instead of violating const-correctness with const_cast<ErrorState*>


Indeed please do that. I suggest looking into whether the member holding output be mutable would avoid a lot of the issues.

yadij · 2026-05-04T03:43:16Z

    err_type templateCode; ///< The internal code for this template.
 };

+namespace ErrorPage {


This move to src/ is a regression. Please leave these classes in the error/ and include their .h files where needed.

Yes the badly named class Error causes namespace issues. If you are going to insist on the huge amount of code polish and redesign added by this PR (beyond the actual needed bug fix), then either do ont use a namespace around the classes or fix that class Error naming as well (I suggest the former).

yadij · 2026-05-04T03:53:28Z

+# XXX: We should remember the name(s) of the namespace(s) surrounding the enum
+# instead. TODO: Replace this C++ parsing hack with a command-line parameter.
 /^namespace *[a-zA-Z]+/	{
+	if (type) next


This change is out of scope and seems to be adding support for invalid C++ syntax:

enum Foo { namespace Blah { ... } };

Please remove, or if necessary please discuss the error being produced that requires a change.

yadij · 2026-05-04T04:00:41Z

+    // below, adjust compile*() methods to avoid ErrorState modifications.
+
    auto blockStart = build.input;
    while (const auto letter = *build.input) {


Looks can be deceiving. The order of refactoring is causing regressions and an increase in technical debt.
A better approach would be to separate the refactoring from the bug fix. The former needs a lot more work, the latter can accept workarounds (wit TODO notes) for design issues.

yadij · 2026-05-04T04:02:17Z

+
+        for (const auto &detail: request->error.details) {
+            mb.appendf("%i-Error-Detail-Brief: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->brief()));
+            mb.appendf("%i-Error-Detail-Verbose: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->verbose(*err)));


Please do. It should be just a matter of running a few FTP requests to a non-FTP server and looking at the cache.log level 11,2 entries for those messages.

yadij · 2026-05-04T04:08:56Z

 class ErrorDetail;
 class ErrorState;

+namespace ErrorPage {


Wrong namespace. This is the forward declarations for the errors/liberror.la library which is supposed to use namespace Error.

The maintenance work fixing class Error conflicts is stalled in backlog. For now please use Error prefix on classes that should be in the namespace Error.

rousskov added 12 commits April 28, 2026 10:56

fixup: Apply ErrorDetail::verbose() API changes

b7c4596

... to verbose() methods that did not use HttpRequest.

fixup: Fix "make check"

9e3285b

fixup: Apply compileLegacyCode() when there is no secondaryCompiler

fda31c0

fixup: Formatted modifies sources

f81c9ad

fixup: This new code meets Assure() requirements

eacceb3

fixup: Polished comments describing new code

96a7d12

fixup: Removed out-of-scope comment/to-do

7838ebc

fixup: Removed an old TODO effectively addressed by this branch

679ac79

fixup: Improve buildtests/layer-01-minimal build

95408d6

rousskov added the S-could-use-an-approval An approval may speed this PR merger (but is not required) label Apr 28, 2026

rousskov commented Apr 28, 2026

View reviewed changes

fixup: Fix include order broken by previous fixup

d05744a

rousskov mentioned this pull request Apr 28, 2026

Fix handling of truncated legacy errorpage %codes #2411

Closed

kinkie approved these changes Apr 29, 2026

View reviewed changes

rousskov commented Apr 29, 2026

View reviewed changes

yadij requested changes May 4, 2026

View reviewed changes

squid-anubis removed the M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label May 4, 2026

	using ErrorTemplateCompiler = ErrorState;
	using TemplateCompiler = ErrorState;

	void compile(Build &build) const;
	void compile(Build &) const;

Conversation

rousskov commented Apr 28, 2026

Side effects

Uh oh!

rousskov commented Apr 28, 2026

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinkie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants