security hardening - #2538
Open
Alexandru2984 wants to merge 5 commits into
Open
Conversation
Alexandru2984
commented
Jul 2, 2026
Alexandru2984
commented
- Fix path traversal on Windows in static file router
- Reject ambiguous request framing to prevent HTTP request smuggling
- Strip CR/LF from response headers, redirects and cookies
- Validate the comparison operator in Criteria's JSON constructor
The traversal-depth check only split the request path on '/', so a target using backslash separators (e.g. "..%5c..%5c", url-decoded to "..\..\") produced a single path segment that was never recognized as "..". On Windows, where the filesystem treats '\' as a directory separator, this allowed escaping the document root and reading files with a whitelisted extension. Normalize backslashes to forward slashes in a copy of the path used solely for the traversal check; the path used to locate the file is left unchanged, so legitimate filenames are unaffected.
The request parser accepted several forms of ambiguous message framing
that a front-end proxy might interpret differently, enabling request
smuggling:
- A request carrying both Content-Length and Transfer-Encoding was
handled by reading Content-Length bytes and ignoring the chunked
encoding (a CL.TE desync). Such requests are now rejected with 400.
- Multiple Content-Length headers with differing values were collapsed
to the first value with no error. They are now detected in addHeader
and rejected with 400.
- Content-Length values were parsed with std::stoull, which silently
accepts a leading sign ("-1" wrapping to a huge size_t) and trailing
garbage ("5abc"). The value is now required to be digits only.
Also harden chunked parsing: the chunk size was read with signed strtol
and added to the accumulated length before the size check, which could
wrap past the limit. It now uses strtoull, validates the field, and
compares against the remaining allowance without overflowing.
Header names and values set via HttpResponse::addHeader, the redirect target, and the Set-Cookie fields (key, value, domain, path) were written into the response verbatim. When any of these are derived from untrusted input, an embedded "\r\n" allowed HTTP response splitting / header injection. Strip CR and LF characters from these fields at the point they are placed into the response, so injected line breaks can no longer forge additional headers or a second response.
Criteria(const Json::Value&) is used by the generated RESTful controllers to build a WHERE clause from the client-supplied "filter" query. The value is bound as a parameter, but the comparison operator (json[1]) was concatenated into the SQL string verbatim, so a filter such as ["id", "= 1 OR 1=1 --", 0] injected arbitrary SQL. This holds even for the default masquerading controllers, which whitelist the column name but never checked the operator. Restrict the operator to a fixed allowlist (=, !=, <>, >, >=, <, <=, like, not like, ilike, not ilike), normalized for spacing and case, and throw on anything else. The field name still cannot be parameterized, so document that it must be trusted / whitelisted by the caller.
an-tao
reviewed
Jul 3, 2026
Comment on lines
+180
to
+182
| auto sanitizedValue = sanitizeHeaderField(value); | ||
| headers_[sanitizeHeaderField(std::move(field))] = | ||
| std::move(sanitizedValue); |
Member
There was a problem hiding this comment.
For the field parameter – This is almost always a hard‑coded string (either from framework internals or from the developer). It rarely, if ever, comes from untrusted input. So we could skip sanitization entirely for the field name, or at least treat it separately to reduce overhead.
For the value parameter – Instead of unconditionally calling erase + remove_if on every invocation, we can first perform a quick scan using find_first_of("\r\n"). In the vast majority of cases (legitimate values contain no CR or LF), this returns npos and we can use the original string immediately, avoiding any memory writes and moves. Only when the scan detects a problematic character do we enter the slower cleaning path.
if (value.find_first_of("\r\n")!=std::string::npos)
{
headers_[std::move(field)] = sanitizeHeaderField(value);
}
else
{
headers_[std::move(field)] = value;
}
Member
|
@Alexandru2984 Thanks for this important security fix. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.