Do not expand errorpage %codes injected by X509 certificates - #2416
Do not expand errorpage %codes injected by X509 certificates#2416rousskov wants to merge 17 commits into
Conversation
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.
|
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 %>a ... %<a [not available]t/O=O1</p>
+ <p>Certificate does not match domainname: /OU=%ts.%03tu @Squid{%6tr} %ssl_ca_name %>a ... %<a %mt/O=O1</p> |
rousskov
added
the
S-could-use-an-approval
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
These comments do not request any PR changes.
|
|
||
| SBuf | ||
| Ftp::ErrorDetail::verbose(const HttpRequest::Pointer &) const | ||
| Ftp::ErrorDetail::verbose(const ErrorTemplateCompiler &) const |
There was a problem hiding this comment.
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.
|
|
||
| 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))); |
There was a problem hiding this comment.
Disclaimer: I have not tested this FTP code path.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done.
220 Service ready
451-ERR_CONNECT_FAIL
451-Error-Detail-Brief: WITH_SERVER
451-Error-Detail-Verbose: WITH_SERVER
451-Error-Detail-Brief: errno=111
451-Error-Detail-Verbose: (111) Connection refused
451 Service Unavailable
kinkie
left a comment
kinkie
left a comment
There was a problem hiding this comment.
LGTM, also looks cleaner than current state
| // below, adjust compile*() methods to avoid ErrorState modifications. | ||
|
|
||
| auto blockStart = build.input; | ||
| while (const auto letter = *build.input) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The order of refactoring is causing regressions and an increase in technical debt.
I am not aware of any new regressions or a significant increase in technical debt caused by this PR. The reviews have not identified any (so far)12. This PR does add TODOs and XXXs, but they do not mark new regressions or new technical debt.
FWIW, this PR changes were necessary for me to fix the bug. I am not aware of a simple workaround or hack that can properly fix this big without some refactoring work. I could have missed it, of course, but the proposed refactoring will be needed anyway (along with other changes), so we would be making progress with this PR even if I missed a simple workaround.
Footnotes
-
There was one incorrect "this move to
src/is a regression" assertion about code that was not actually moved tosrc/. ↩ -
This PR does not add new
.hsource file for the one-methodPercentCodeCompilerAPI. IMO, that source file should not be added in this PR as detailed in another change request thread; I will add that file if you insist. The review assertion that.ccfile should be added was incorrect because that abstract class does not have any method definitions. ↩
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.
squid-anubis
added
M-waiting-staging-checks
| class ErrorState; | ||
|
|
||
| namespace ErrorPage { | ||
| class PercentCodeCompiler; |
There was a problem hiding this comment.
Why "PercentCode" ? we dont have other types of page compiler for error pages.
| 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.
There was a problem hiding this comment.
Why "PercentCode" ? we dont have other types of page compiler for error pages.
This compiler does not compile error pages; it compiles percent codes. There is another compiler inside ErrorState that compiles error page templates. ErrorState uses PercentCodeCompiler objects to handle %code occurrences inside error page templates.
I do not see the required .h/.cc being added for this new class.
There are no .cc file because this abstract class has no method definitions or other code that can be placed in a .cc file. This new API has a single pure virtual method.
As for .h file, I agree that it should be eventually added. It was not added in this PR primarily because, I assume, we want the resulting commit to be used as a bug-fixing patch (referenced from certain well-known pages) without requiring bootstrapping. If you insist, I will move PercentCodeCompiler declaration into a dedicated/new src/error/PercentCodeCompiler.h source file, making bootstrapping a requirement. Do you insist?
| { | ||
| public: | ||
| using Pointer = ErrorDetailPointer; | ||
| using ErrorTemplateCompiler = ErrorState; |
There was a problem hiding this comment.
"Error" word is redundant within the scope ErrorPage::
| using ErrorTemplateCompiler = ErrorState; | |
| using TemplateCompiler = ErrorState; |
| /// \sa compileDetail() | ||
| SBuf compile(const char *input, bool building_deny_info_url, bool allowRecursion); | ||
|
|
||
| void compile(Build &build) const; |
There was a problem hiding this comment.
Redundant parameter and missing documentation on new method.
| void compile(Build &build) const; | |
| void compile(Build &) const; |
There was a problem hiding this comment.
I moved documentation of this private methods from its .cc file to this header file to avoid arguing. That documentation uses the previously redundant name, so that was left in place. Commit 7715b40.
| { | ||
| Assure(build.input); | ||
|
|
||
| // TODO: Instead of violating const-correctness with const_cast<ErrorState*> |
There was a problem hiding this comment.
Indeed please do that. I suggest looking into whether the member holding output be mutable would avoid a lot of the issues.
There was a problem hiding this comment.
I do not think those changes belong to this PR, but done in commit e84ee90.
| err_type templateCode; ///< The internal code for this template. | ||
| }; | ||
|
|
||
| namespace ErrorPage { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
This move to
src/is a regression. Please leave these classes in theerror/and include their.hfiles where needed.
These two classes were not in error/ before this PR. Build was (and still is) in src/errorpage.h. PercentCodeCompiler is a new class. There is no good place for their definitions in existing error/ files. Creating a new file would complicate sharing this patch. It is best to keep them where they are (for now).
Yes the badly named
class Errorcauses 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 thatclass Errornaming as well (I suggest the former).
I do not insist on any unnecessary code polish and redesign. I believe that this PR does not contain a huge amount of unnecessary redesign; it contained more-or-less necessary changes/additions. Polishing was applied to code already modified for non-polishing reasons. This PR now also contains const changes that were not necessary. I will undo the latter if you retract the corresponding change request.
This PR does not add ErrorPage namespace. This PR does not move existing ErrorPage::Build into a new namespace or a new source code directory. All that is old/existing code. Removing ErrorPage from ErrorPage::Build would increase the number of changes in this PR (and is not a good idea for other reasons as well).
ErrorPage::PercentCodeCompiler is new code, but it belongs to the existing ErrorPage namespace (even if one does not like that namespace current name spelling).
When/if we decide to introduce an Error namespace, it would be way easier (and even safer and less noisy!) to rename ErrorPage namespace to Error namespace than to move ErrorFoo declaration from the global namespace into Error namespace while renaming the class itself to Error::Foo!
Please withdraw this change request. It is based on false assumptions, and the requested changes will make anticipated future improvements more difficult/risky/noisy.
| # 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This change is out of scope
This change is an in-scope surgical bug fix. Here is branch commit 48a03a6 message with more information:
Generate src/error/categories.cc without wrong ErrorPage namespaceld: 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
ErrorPagenamespace. The AWK script incorrectly applied that namespace to the generatederr_type_strdefinition.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.
I have now explicitly mentioned the above fix in the PR description.
Amos: and seems to be adding support for invalid C++ syntax
If this change also adds support for some invalid C++ syntax, it is not a problem: The C++ compiler will not let folks use invalid syntax, so this AWK script can assume that it is getting valid C++ syntax.
| // below, adjust compile*() methods to avoid ErrorState modifications. | ||
|
|
||
| auto blockStart = build.input; | ||
| while (const auto letter = *build.input) { |
There was a problem hiding this comment.
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.
|
|
||
| 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))); |
There was a problem hiding this comment.
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.
| class ErrorDetail; | ||
| class ErrorState; | ||
|
|
||
| namespace ErrorPage { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Wrong namespace.
The ErrorPage namespace is an existing namespace. Whether that existing name is wrong or right is irrelevant to this PR.
Moreover, one of the two forward declarations in this forward.h file is also for an existing name -- ErrorPage::Build exists in the current official code.
For now please use Error prefix on classes that should be in the namespace Error.
Using a namespace is better than using prefixes (where we want to use a namespace), even where the currently available namespace name is inferior to some future one. C++ namespaces affect more than spelling, often in very subtle ways. Also, renaming a namespace is often less noisy than replacing a name prefix with a namespace.
Please withdraw this change request. It is based on a false assumption, and implementing the requested changes will make things worse.
squid-anubis
removed
the
M-passed-staging-checks
|
Issues in the PR description:
Likely a side effect of only X509 certificate (
Vaguely correct, specifically implemented wrong. Correctly defined: Both case 'D':
if (!build.allowRecursion)
p = "%D"; // if recursion is not allowed, do not convertLooks to me like something is not setting
What?
Per the wiki: Strange that the one code called out as example of what does expand is the one you say is not supported.
This is bad change, and wrong statements about what is supposed to be.
And yes, deployed configurations do use %code's in that file. |
I see a lot of problems/misunderstandings in that comment. It will take me a long time to fix/address all of them, especially in writing. My current plan is to focus on addressing change requests in #2416 (review). If I succeed (and that is a big "if"!), I will start to nibble at that comment. |
rousskov
added
S-waiting-for-author
... into mutable data members. The long-term TODO remains the same. Context: squid-cache#2416 (comment)
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Amos: Issues in the PR description:
In summary: AFAICT, no issues in the PR description were identified in this review comment. I adjusted PR description to add a commit reference (in hope to address one "insufficient documentation for an old Squid feature" concern) and tried to clarify several misunderstandings (that did not require PR description updates). Details are below.
PR description: This bug applies to both legacy errorpage
%codesequences like%Rand modern@Squid{logformat %code}errorpage sequences.Amos:
@Squidis not a format code documented anywhere.
Insufficient documentation for @Squid sequences in error page templates is not a problem created by this PR and is not something this PR should fix. 2019 commit 7e6eabb message can be used as a starting point if one wants to familiarize themselves with that Squid feature or author the corresponding documentation PRs. I have now added that commit reference to the PR description in hope to address the above concern.
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.
Likely a side effect of only X509 certificate (
%ssl_and%ssl::) codes being supported inside%Dexpansion.
%D expansion supports all other errorpage legacy %codes. For example, %R and %t codes are expanded as expected when found inside %D expansion.
%D expansion supports non-X509 error details, and I cannot rule out that, say, some exotic deployment environment has strerror(3) output that contains byte sequences that Squid will mistake for %codes. Thus, I cannot claim that only X509 certificate fields can be an injection vector. X509 certificate fields are the only known injection vector.
Two primary solution candidates were considered:
A) Escape-then-compile-to-unescape: Security::ErrorDetail could escape
%codesequences received from "external" sources, so that ErrorState::compile() would see%%Xand 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
%codecompiling code in ErrorState, so that ErrorDetail can decide what to compile and what not to. Now, Security::ErrorDetail only compiles%codesequences found indetail; ErrorState does not compile ErrorDetail output.
Vaguely correct, specifically implemented wrong.
I do not know what was "implemented wrong". I have identified several false assumptions (above and below). I hope those corrections address this "implemented wrong" concern. If they do not, please detail what was implemented incorrectly.
case 'D': if (!build.allowRecursion) p = "%D"; // if recursion is not allowed, do not convertLooks to me like something is not setting
allowRecursion=falsecorrectly before expanding X.509 cert strings into their nested place inside a%Dexpansion.
build.allowRecursion does not control "expanding X.509 cert strings". Those strings should never be expanded, so no special variable is needed to control that logic.
N.B. build.allowRecursion does not control expansion of error-details.txt templates either. Those templates should always be expanded, so no special variable is needed to control that logic.
build.allowRecursion controls expanding %D inside %D: Only top-level %D should be expanded; a %D code found inside expanded %D should not be expanded again because such expansion could presumably lead to infinite recursion. This PR does not change how this existing protection works.
That is the actual bug, the rest of this PR is refactoring and code polish
The above assertions are false (as detailed above). This bug and PR are not about this particular recursion kind and build.allowRecursion. This PR fixes the bug without changing when/how build.allowRecursion is set. This bug cannot be fixed by changing when/how build.allowRecursion is set.
the rest of this PR is refactoring and code polish due to author dislike of code style/design.
The above speculation is also incorrect.
Side effects
The
detailsfield inerror-details.txtnow supports modern@Squid{logformat %code}errorpage sequences by design rather than by accident, addressing an old (and essentially misleading) TODO inside Security::ErrorDetail::convertErrorCodeToDescription().
What?
FWIW, AFAICT, the quoted PR description text (that the above "What?" is reacting to) is fairly clear and correct.
@Squidis not documented, either in the "old error page" , or the "modern" format syntax.
Correct, but I do not see how that fact is relevant to this PR.
error-details.txt,errors/templates/files,logformat,deny_infoand helperkey_extrastemplates are all supposed to support the same set of %code's. Only the output format/encoding for %code may change to suit the one-vs-multi line syntax each template needs.
The above assertion is incorrect or very misleading: The set of logformat %codes and the set of legacy errorpage %codes are two very different/independent sets. The meaning of some %codes overlaps (e.g., %t errorpage code and %tl logformat code both produce local time), but even similar-meaning %codes in two sets use different %code name spelling (e.g., errorpage codes use single-letter names, while there are no single-letter logformat %code names besides %%). And C++ code responsible for %code expansion is also set-specific. Prior to @Squid support addition, there was no way to reference a logformat %code from an errorpage template. See 2019 commit 7e6eabb for details. This PR does not change any of that, of course.
The
descrfield inerror-details.txtno longer supports errorpage%codesequences. It was never documented to support them, and it did not support Security::ErrorDetail %codes like %ssl_ca_name.Per the wiki:
%D Squid-generated error details. ***May contain other error page formatting codes***. Currently only TLS/SSL connection failures are detailed. For example, %D in a customized ERR_SECURE_CONNECT_FAIL response may be expanded into “The host name you are connecting to (foo.com) does not match any of the certificate names (foo.org, foo.net)…”). Supported since [Squid-3.2](http://wiki.squid-cache.org/Releases/Squid-3.2). See also: application-level error code (%x) and system level error code/detail (%e/%E).Strange that the one code called out as example of what does expand is the one you say is not supported.
The two texts describe two different fields in an error detail template: My statement explicitly talks about the descr field, while the wiki page talks about the detail field. The latter still supports errorpage %code sequences, among others. For example, here is the default %D configuration for SQUID_TLS_ERR_CONNECT errors showing all three fields (the third one being the error "name"):
name: SQUID_TLS_ERR_CONNECT
detail: "%ssl_error_descr: %ssl_lib_error"
descr: "Failed to establish a secure connection"
Note that the above specific detail field includes the descr field value via %ssl_error_descr macro. This kind of inclusion helps keep descr values free from %codes.
Official code already documents the corresponding C++ data members accordingly:
SBuf detail; ///< for error page %D macro expansion; may contain macros
SBuf descr; ///< short error description (for use in debug messages or error pages)Note how "may contain macros" is limited to detail. This PR does not change any of that either.
The field is not meant to contain %code sequences -- they belong to the
detailsfield that specifies detail reporting format. Hopefully, no deployed configurations use%codesequences in thedescrfield.This is bad change, and wrong statements about what is supposed to be.
I disagree on both counts. I hope that identifying false assumptions/assertions (see above) addresses this concern.
error-details.txtis a list of templates expected to support %code and language translation just like theerrors/templates/files.
Yes, and it still does. However, unlike structure-less1 HTML errors/templates/ERR_* files, each template inside error-details.txt has three distinct fields. Only two of those three fields support translations: detail and descr. Since this PR, only one of them supports %codes: detail. The descr field was never meant to support %codes; it was added as a human-friendlier alternative to printing the OpenSSL-based name field (e.g., replacing or supplementing hard-coded X509_V_ERR_AKID_SKID_MISMATCH that we used to print in error pages prior to 2011 commit cf09bec with "Authority and subject key identifier mismatch" phrase that can be translated and customized).
We could complicate things further by keeping prior accidental support for %codes in descr fields. The infrastructure introduced by this PR should be enough to support that (mis)feature for valid configurations. However, I think it is best to restrict descr to simple text it was intended for, avoiding three-level deep recursion into %codes (i.e. errorpage %D, %ssl_error_descr in error detail field, some %code in error detail descr field, and only then some final text produces by that last %code). Our current lack of %code (including %code recursion) validation in error detail fields makes the intended simple/flat structure implemented in this PR even more attractive!
And yes, deployed configurations do use %code's in that file.
Do they use %codes in descr fields? If not, they are not relevant here. Otherwise, can you post a sample of such existing use? One representative error detail template (i.e. the three fields: name, detail, and descr) could be enough.
If some highly-customized setups do use %codes in descr fields, they will need to update their (accidentally/unknowingly supported up to this PR) custom configurations. We might even be able to help them with the necessary conversions. Requiring such one-time configuration updates in those rare cases is still the lesser of the two evils IMO.
Footnotes
-
Structure-less from Squid point of view: Squid does not really try to fully understand their (HTML) structure when substituting
%codes. ↩
|
|
||
| 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))); |
There was a problem hiding this comment.
Done.
220 Service ready
451-ERR_CONNECT_FAIL
451-Error-Detail-Brief: WITH_SERVER
451-Error-Detail-Verbose: WITH_SERVER
451-Error-Detail-Brief: errno=111
451-Error-Detail-Verbose: (111) Connection refused
451 Service Unavailable
| // below, adjust compile*() methods to avoid ErrorState modifications. | ||
|
|
||
| auto blockStart = build.input; | ||
| while (const auto letter = *build.input) { |
There was a problem hiding this comment.
The order of refactoring is causing regressions and an increase in technical debt.
I am not aware of any new regressions or a significant increase in technical debt caused by this PR. The reviews have not identified any (so far)12. This PR does add TODOs and XXXs, but they do not mark new regressions or new technical debt.
FWIW, this PR changes were necessary for me to fix the bug. I am not aware of a simple workaround or hack that can properly fix this big without some refactoring work. I could have missed it, of course, but the proposed refactoring will be needed anyway (along with other changes), so we would be making progress with this PR even if I missed a simple workaround.
Footnotes
-
There was one incorrect "this move to
src/is a regression" assertion about code that was not actually moved tosrc/. ↩ -
This PR does not add new
.hsource file for the one-methodPercentCodeCompilerAPI. IMO, that source file should not be added in this PR as detailed in another change request thread; I will add that file if you insist. The review assertion that.ccfile should be added was incorrect because that abstract class does not have any method definitions. ↩
| err_type templateCode; ///< The internal code for this template. | ||
| }; | ||
|
|
||
| namespace ErrorPage { |
There was a problem hiding this comment.
This move to
src/is a regression. Please leave these classes in theerror/and include their.hfiles where needed.
These two classes were not in error/ before this PR. Build was (and still is) in src/errorpage.h. PercentCodeCompiler is a new class. There is no good place for their definitions in existing error/ files. Creating a new file would complicate sharing this patch. It is best to keep them where they are (for now).
Yes the badly named
class Errorcauses 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 thatclass Errornaming as well (I suggest the former).
I do not insist on any unnecessary code polish and redesign. I believe that this PR does not contain a huge amount of unnecessary redesign; it contained more-or-less necessary changes/additions. Polishing was applied to code already modified for non-polishing reasons. This PR now also contains const changes that were not necessary. I will undo the latter if you retract the corresponding change request.
This PR does not add ErrorPage namespace. This PR does not move existing ErrorPage::Build into a new namespace or a new source code directory. All that is old/existing code. Removing ErrorPage from ErrorPage::Build would increase the number of changes in this PR (and is not a good idea for other reasons as well).
ErrorPage::PercentCodeCompiler is new code, but it belongs to the existing ErrorPage namespace (even if one does not like that namespace current name spelling).
When/if we decide to introduce an Error namespace, it would be way easier (and even safer and less noisy!) to rename ErrorPage namespace to Error namespace than to move ErrorFoo declaration from the global namespace into Error namespace while renaming the class itself to Error::Foo!
Please withdraw this change request. It is based on false assumptions, and the requested changes will make anticipated future improvements more difficult/risky/noisy.
| # 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 |
There was a problem hiding this comment.
This change is out of scope
This change is an in-scope surgical bug fix. Here is branch commit 48a03a6 message with more information:
Generate src/error/categories.cc without wrong ErrorPage namespaceld: 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
ErrorPagenamespace. The AWK script incorrectly applied that namespace to the generatederr_type_strdefinition.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.
I have now explicitly mentioned the above fix in the PR description.
Amos: and seems to be adding support for invalid C++ syntax
If this change also adds support for some invalid C++ syntax, it is not a problem: The C++ compiler will not let folks use invalid syntax, so this AWK script can assume that it is getting valid C++ syntax.
| class ErrorDetail; | ||
| class ErrorState; | ||
|
|
||
| namespace ErrorPage { |
There was a problem hiding this comment.
Wrong namespace.
The ErrorPage namespace is an existing namespace. Whether that existing name is wrong or right is irrelevant to this PR.
Moreover, one of the two forward declarations in this forward.h file is also for an existing name -- ErrorPage::Build exists in the current official code.
For now please use Error prefix on classes that should be in the namespace Error.
Using a namespace is better than using prefixes (where we want to use a namespace), even where the currently available namespace name is inferior to some future one. C++ namespaces affect more than spelling, often in very subtle ways. Also, renaming a namespace is often less noisy than replacing a name prefix with a namespace.
Please withdraw this change request. It is based on a false assumption, and implementing the requested changes will make things worse.
| class ErrorState; | ||
|
|
||
| namespace ErrorPage { | ||
| class PercentCodeCompiler; |
There was a problem hiding this comment.
Why "PercentCode" ? we dont have other types of page compiler for error pages.
This compiler does not compile error pages; it compiles percent codes. There is another compiler inside ErrorState that compiles error page templates. ErrorState uses PercentCodeCompiler objects to handle %code occurrences inside error page templates.
I do not see the required .h/.cc being added for this new class.
There are no .cc file because this abstract class has no method definitions or other code that can be placed in a .cc file. This new API has a single pure virtual method.
As for .h file, I agree that it should be eventually added. It was not added in this PR primarily because, I assume, we want the resulting commit to be used as a bug-fixing patch (referenced from certain well-known pages) without requiring bootstrapping. If you insist, I will move PercentCodeCompiler declaration into a dedicated/new src/error/PercentCodeCompiler.h source file, making bootstrapping a requirement. Do you insist?
| { | ||
| public: | ||
| using Pointer = ErrorDetailPointer; | ||
| using ErrorTemplateCompiler = ErrorState; |
| { | ||
| Assure(build.input); | ||
|
|
||
| // TODO: Instead of violating const-correctness with const_cast<ErrorState*> |
There was a problem hiding this comment.
I do not think those changes belong to this PR, but done in commit e84ee90.
| /// \sa compileDetail() | ||
| SBuf compile(const char *input, bool building_deny_info_url, bool allowRecursion); | ||
|
|
||
| void compile(Build &build) const; |
There was a problem hiding this comment.
I moved documentation of this private methods from its .cc file to this header file to avoid arguing. That documentation uses the previously redundant name, so that was left in place. Commit 7715b40.
rousskov
added
S-waiting-for-reviewer
4 participants
When available, Security::ErrorDetail is added to Squid-generated errors
that use customizable errorpage formats containing
%Derrorpage%code. Security::ErrorDetail itself supports customizable verbosereporting format (see
detailinerrors/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
%codesequence configured byerror-details.txtfrom a%codesequence that came from, say, areceived X509 certificate field. Both would be expanded!
This bug applies to both legacy errorpage
%codesequences like%Rand modern
@Squid{logformat %code}errorpage sequences (supportedsince 2019 commit 7e6eabb).
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
%codesequences received from "external" sources, so thatErrorState::compile() would see
%%Xand replace that with%X,correctly relaying received
%X. This solution was rejected becausewe 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
%codecompiling code inErrorState, so that ErrorDetail can decide what to compile and what
not to. Now, Security::ErrorDetail only compiles
%codesequencesfound in
detail; ErrorState does not compile ErrorDetail output.Side effects
The
detailsfield inerror-details.txtnow supports modern@Squid{logformat %code}errorpage sequences by design rather than byaccident, addressing an old (and essentially misleading) TODO inside
Security::ErrorDetail::convertErrorCodeToDescription().
The
descrfield inerror-details.txtno longer supports errorpage%codesequences. It was never documented to support them, and it didnot support Security::ErrorDetail %codes like %ssl_ca_name. The field is
not meant to contain %code sequences -- they belong to the
detailsfield that specifies detail reporting format. Hopefully, no deployed
configurations use
%codesequences in thedescrfield.Also improved
src/mk-string-arrays.awksupport for C++ forwarddeclarations inside a namespace. After we added forward declarations for
classes declared inside an
ErrorPagenamespace, the AWK scriptincorrectly applied that namespace to the generated
err_type_strdefinition, breaking the build.