apache / apache/trafficserver

Bare carriage returns in header line endings are still forwarded when separated from the CRLF by whitespace

Open
#13,624 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
2k
Forks
874
Avg merge
6d 15h
Merged PRs (30d)
46

Description

Follow-up to #12195 and #13595.

#13595 stops the raw input line from being preserved when the byte immediately before the terminating CRLF is a carriage return. That covers the line reported in #12195, but not the general case.

`mime_parser_parse` trims the field value with `field_value.rtrim_if(&ParseRules::is_wslfcr)`, which strips SP and HT as well as CR and LF. So a bare CR only has to be separated from the CRLF by one space or tab to end up back in the raw-print pad and be forwarded unchanged.

Built on master with #13595 applied, output captured from `HTTPHdr::print`:

| request header line | serialized output |
|---|---|
| `Extra-CRs: \r\r\r\r\n` | `Extra-CRs: \r\n` (fixed by #13595) |
| `Extra-CRs: \r \r\n` | `Extra-CRs: \r \r\n` |
| `Extra-CRs: \r\r \r\n` | `Extra-CRs: \r\r \r\n` |
| `Extra-CRs: bar\r \r\n` | `Extra-CRs: bar\r \r\n` |
| `Extra-CRs: \r\t\r\n` | `Extra-CRs: \r\t\r\n` |

RFC 9112 section 2.2 says a recipient of a bare CR must either consider the element invalid or replace each bare CR with SP before processing or forwarding the message. The last four rows do neither.

A positional check will keep missing spellings of this. Making it structural works and keeps the fast path: once the value is trimmed, require that everything between the end of the value and the end of the line is optional whitespace followed by exactly one CRLF.

```cpp
// find value first
field_value.ltrim_if(&ParseRules::is_ws);
field_value.rtrim_if(&ParseRules::is_wslfcr);

if (raw_print_field) {
// Raw printing copies the original input bytes instead of re-serializing, so it would
// replay this line ending verbatim. Everything between the end of the trimmed value and
// the end of the line must be optional whitespace followed by exactly one CRLF; a bare
// CR in there is malformed (RFC 9112 section 2.2) and must not be forwarded.
TextView tail{field_value.data() + field_value.size(), parsed.data() + parsed.size()};
if (tail.suffix(2) != "\r\n" || tail.remove_suffix(2).find_first_of("\r\n") != TextView::npos) {
raw_print_field = false;
}
}
```

With that in place all five rows normalize to a single CRLF and `test_proxy_hdrs` stays green. Well-formed fields keep the raw fast path: `X: a \r\n`, `X:\ta\r\n` and a folded line are all still emitted byte for byte. The `else if` on the line ending goes away, as does the need for the `size() > 2` guard, since `suffix()` clamps.

Three smaller items for the same change:

1. The unit test fixture added in #13595 produces a raw-print pad of exactly 7, and `mime_field_name_value_set` only honors raw printing when the pad is 7 or less. One more CR and the pre-existing pad gate suppresses raw printing on its own, so the test would pass without any fix at all. It needs a comment saying so, plus a case that is not on the boundary.
2. Only the request direction is covered. The same function serves responses, so a `parse_resp` case is cheap.
3. `tests/gold_tests/headers/field_name_space.test.py` covers the sibling whitespace-before-colon case end to end. Because `MIMEFieldBlockImpl::move_strings` clears the raw flag on any string-heap relocation, whether these bytes reach the origin depends on heap state a unit test does not exercise, so a gold test with a raw-socket client would pin the actual wire contract.

This reproduces on 10.2.x and 10.1.x as well.

Contributor guide

Open the contributing guide

Research direction

Start in mime_parser_parse and review the raw-print handling described in the issue, then run test_proxy_hdrs. Check request and parse_resp cases for the five malformed line endings, while confirming well-formed and folded fields retain byte-for-byte output. Use tests/gold_tests/headers/field_name_space.test.py as the end-to-end reference for the wire-level behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
networking
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.