chenBright opened a new pull request, #3521:
URL: https://github.com/apache/brpc/pull/3521
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
Four header forms are parsed here in a way a conforming front-end does
not, and
one legal form is not parsed at all. In a `client -> proxy -> bRPC` chain
both
parties parse the same bytes independently, so every disagreement about
where a
message ends turns the tail of message N into the head of message N+1 for
one of
them. The primitive behind request smuggling and response desync.
1. **`Content-Length : 5`** — this fork's `TOKEN(c)` maps SP to itself.
[Upstream](https://github.com/nodejs/http-parser)
has a `STRICT_TOKEN()` and a `HTTP_PARSER_STRICT` branch. Both were
dropped
here, leaving the lenient path as the only path. A space before the
colon
therefore keeps `header_state` on the framing header, so the value still
drives our framing while the field name we report is `"Content-Length
"`.
RFC 7230 3.2.6 excludes SP from `tchar`, and 3.2.4 forbids whitespace
before
the colon.
2. **`Content-Length: 1 3`** — interior blanks were skipped, so we read 13
where
a proxy reads 1 or rejects. Same class as CVE-2022-32213, fixed
upstream by
http-parser https://github.com/nodejs/http-parser/commit/01da95f and
https://github.com/nodejs/http-parser/commit/cd88eef.
Neither was picked up here.
3. **`Transfer-Encoding:\r\n chunked`, `Content-Length:\r\n 5`** — both
obs-fold
states (RFC 7230 3.2.4) carry `header_state` across the fold, so a
folded
framing value still drove our framing while a front-end that does not
unfold
sees a different message.
4. **chunked trailers** — after the last chunk the parser sets
`F_TRAILING` and
returns to `s_header_field_start`, so trailer fields reach the same
`on_header_field` / `on_header_value` callbacks as the header section.
Neither
looked at `F_TRAILING`, so a trailer landed in the header map like a
real
header: appended to an existing entry of the same name, or added as a
new one.
A front-end only filters the header section, so any chunked request
could put
an `Authorization` or `X-Forwarded-For` past it:
POST / HTTP/1.1
Host: a.com
X-Forwarded-For: 10.0.0.1 <- set by the proxy
Transfer-Encoding: chunked
5
hello
0
X-Forwarded-For: 6.6.6.6 <- invisible to the proxy,
appended by us
Authorization: Bearer stolen
5. **`Transfer-Encoding: gzip, chunked`** — found while auditing the
`TOKEN()`
call sites for (1). `TOKEN(' ')` is truthy, which made the
`else if (c == ' ' || c == '\t')` arm of
`h_matching_transfer_encoding_token_start` dead code: the OWS after the
comma
started a new coding name, so `chunked` never matched. This is the form
RFC 7230 7 spells out. Requests were rejected with
`HPE_INVALID_TRANSFER_ENCODING` per RFC 7230 3.3.3, and responses were
read to
EOF instead of as chunked.
### What is changed and the side effects?
Changed:
- Reintroduce `STRICT_TOKEN()`. This fork's `tokens[]` already omits SP as
RFC 7230 3.2.6 does, so the strict form is the plain lookup and `TOKEN()`
is
the leniency layered on top.
- Reject SP before the colon of `Content-Length`, `Transfer-Encoding`,
`Connection` and `Upgrade`. Those four are the only terminal
`header_state`s
reachable in that switch arm. Any other name is `h_general` or still
matching, and keeps the historical leniency unless the new flag is on.
- Backport https://github.com/nodejs/http-parser/commit/01da95f and
https://github.com/nodejs/http-parser/commit/cd88eef.
- Reject obs-fold of a value whose `header_state` decides where the body ends
(`is_framing_header_state()`, i.e. the Content-Length and
Transfer-Encoding
states). Both fold states need the check: `s_header_value_lws` for a
value
that has started, `s_header_value_discard_lws` for one that has not.
- Use `STRICT_TOKEN()` unconditionally in
`h_matching_transfer_encoding_token_start`, as upstream does, so the
OWS
after a comma is skipped instead of starting a coding name.
- Add the `h_content_length*`, `h_transfer_encoding`, `h_upgrade` and
`h_matching_transfer_encoding_*` cases missing from
`http_parser_header_state_name()`, which printed `h_unknown` for them.
- **`http_message.cpp`**
- `on_header_field` and `on_header_value` return early when `F_TRAILING`
is
set, so trailers are discarded rather than merged. `parser->flags` is
cleared at the start of each message, so the check is scoped to one
trailer.
- `-http_allow_obs_fold` and `-http_strict_header_token`, both default
false,
read into the parser in the `HttpMessage` constructor. Keeping the
gflags out
of the vendored parser preserves its diffability against upstream.
HTTP/1 trailers have never been readable through any bRPC API, so dropping
them
is the minimal fix; if they are ever supported they should get their own
map
rather than reuse `_headers`. h2 is unaffected — although `H2StreamContext`
derives from `HttpMessage`, h2 headers come from HPACK via `OnHeaders` and
never
reach these two callbacks, which are registered only for the HTTP/1
parser, and
`grpc-status` is read only when `is_http2` is true.
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of
Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
