reject CR and LF in basic_fields field name and value - #3105
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3105 +/- ##
===========================================
- Coverage 93.30% 93.20% -0.11%
===========================================
Files 177 177
Lines 13764 13739 -25
===========================================
- Hits 12843 12805 -38
- Misses 921 934 +13
... and 31 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Hi, thanks for the PR. A few caveats worth flagging:
Separately, if( sname.find('\r') != string_view::npos ||
sname.find('\n') != string_view::npos)
{
BOOST_BEAST_ASSIGN_EC(ec, error::bad_field);
return nullptr;
}
if( value.find('\r') != string_view::npos ||
value.find('\n') != string_view::npos)
{
BOOST_BEAST_ASSIGN_EC(ec, error::bad_value);
return nullptr;
}is ~3x faster for ~30 char inputs and ~10x faster around ~300 chars. Note: I'm not suggesting this exact version, just that |
Replace find_first_of with two single-octet finds, which lower to memchr and measure notably faster from ~30 char inputs up.
|
Good points, took a proper look at each. On the scan: I benchmarked before touching anything, and on Apple clang with core::string_view two single-octet find() calls measure about 1.4x faster than find_first_of("\r\n") at 30 chars and roughly 5x at 300, with find_first_of only winning marginally below ~10 chars. The root cause is that find_first_of walks the input octet by octet testing membership in the needle set, while find(char) forwards to memchr and gets vectorised. I've switched to the two-find form through a small detail::contains_crlf helper and extended the same rejection to set_method_impl, set_target_impl and set_reason_impl, reusing the parser's bad_method, bad_target and bad_reason codes, with tests in message.cpp. The read path is unaffected since basic_parser never passes those strings through with a bare CR or LF; the only new cost there is one memchr pass per start line. One quirk I noticed while testing: header::method_string assigns its cached verb before calling set_method_impl, so a rejected method leaves the cached verb as unknown while the stored bytes stay intact. I left that ordering alone to keep the diff contained. On layering, the case for doing it here is that try_create_new_element is the one choke point every set and insert overload funnels through, so the check is written once and costs two memchr passes over bytes the function is about to copy anyway. Delegating means every call site that touches request-derived data has to remember to pre-scan, and a single miss puts a split header on the wire, which is the failure mode that worries me. Full charset validation would cover this too, but it needs a per-octet table lookup rather than memchr, and its main additional effect beyond CR/LF is rejecting stray C0 controls, which are ugly but don't alter framing on a correct peer; CR and LF are the two octets that actually change message structure. If you think the table-driven version is worth that cost I'm fine doing it instead. |
|
any update? |
Regarding the code, I think we could probably do a bit better than: s.find('\r') != string_view::npos ||
s.find('\n') != string_view::npos;Especially since there’s an opportunity here to validate names and values against their complete allowed character sets. That said, I think this is the less important part. It would be good to first settle the questions above:
If we decide to perform the validation, there are a few options:
The following is a table of popular HTTP libraries and frameworks, showing whether they perform validation and, if so, whether it happens at the container level or at serialization time (generated using Claude Code): Beast is neither a client nor a server library, so there are other vulnerabilities on both the client and server sides that it doesn’t, and can’t, prevent. This wouldn’t be the only case where the higher-level application needs to take responsibility for the appropriate validation. Given that, I believe delegating this validation to the higher level would be a defensible position. |
2 participants
basic_fields::try_create_new_element assembles the on-wire field as name, ": ", value, then a terminating CRLF, but it only checks the name and value lengths, never their contents. A CR or LF sitting inside a value is copied straight into that buffer, so a header built from attacker-influenced data such as a redirect target or a reflected parameter splits the field and starts new header lines. Setting Location to "/next\r\nSet-Cookie: sid=attacker" and serialising the response puts Set-Cookie on its own line, which is classic response splitting.
The fix rejects a name or value containing CR or LF in try_create_new_element, alongside the existing size checks, reusing the bad_field and bad_value codes the parser already returns for those octets when reading. All of the set and insert overloads, including the error_code variants, route through this one function, so the check covers them together and stops the malformed field at the point it is created instead of relying on every caller to pre-scan its input.