sahvx655-wq opened a new pull request, #3588:
URL: https://github.com/apache/brpc/pull/3588
### What problem does this PR solve?
Issue Number: N/A
Problem Summary:
`URI::SetHttpURL` and `URI::SetH2Path` accept raw CR and LF in every
component of a URL. I went looking at `uri.cpp` after noticing that
`MakeRawHttpRequest` screens each header name and value through `HeaderHasCRLF`
but takes the request line and the `Host` header straight out of the `URI` with
no screening at all. The authority scan looks like it validates, except
`is_valid_char` is only reached on the three `URI_PARSE_CHECK` entries in
`g_url_parsing_fast_action_map` (`' '`, `':'`, `'@'`), so CR and LF take the
`URI_PARSE_CONTINUE` branch and are copied into `_host` unchanged. The path,
query and fragment loops test for a space and nothing else. The root cause is
that neither parser ever looks at the control characters that matter, and the
`is_valid_char` list is effectively unreachable.
Those bytes come back out verbatim. Linking a small program against the
built library, `SetHttpURL("http://a.com/s\r\nX-Injected:1";)` returns 0 with an
OK status and `MakeRawHttpRequest` writes `GET /s<CRLF>X-Injected:1 HTTP/1.1`;
putting the CRLF before the slash lands it in `_host` instead and gives `Host:
a.com<CRLF>X-Injected:1`. Query and fragment behave the same way, and
`SetH2Path` passes the sequence through to `GenerateH2Path`, which is forbidden
for an HTTP/2 field value by RFC 9113 section 8.2.1. None of these payloads
needs a space, so the existing `Invalid space in path/query/fragment` errors
never fire on them. Left unfixed, any service that builds a client URL out of
something a caller supplied lets that caller append headers to the backend
request, or smuggle a second request after a bare `<CRLF><CRLF>`, which is the
CWE-93 request splitting pattern the header guard was added for.
### What is changed and the side effects?
Changed:
CR and LF are rejected at the top of `SetHttpURL` and `SetH2Path`. These are
the only two `URI` entry points documented to parse and report through
`status()`, and `PackHttpRequest` already converts a failed `uri().status()`
into an RPC error, so the rejection surfaces without a new error channel.
`set_path`, `set_host` and `SetHostAndPort` sit under the `The input MUST be
guaranteed to be valid` note in `uri.h` and are deliberately untouched. Fixing
this in the parsers rather than in `MakeRawHttpRequest` is the layer that
works: the serialiser cannot drop a request target the way it drops a header,
and it has no way to report a failure.
One stale comment in `http2_rpc_protocol.cpp` is corrected, since too many
query parameters is no longer the only reason `SetH2Path` can fail. The error
code it reports is unchanged.
`URITest.invalid_crlf` covers the host, path, query and fragment cases for
`SetHttpURL`, bare CR and bare LF as well as the pair, and the three
`SetH2Path` cases. It fails on master at the first assertion (`SetHttpURL`
returns 0) and passes with the fix.
Side effects:
- Performance effects: one `strpbrk` over the URL per
`SetHttpURL`/`SetH2Path` call, both of which are per-request rather than
per-byte-of-body. No change inside the parsing loops, so the fast action map
path is untouched.
- Breaking backward compatibility: no. A URL carrying CR or LF was never
valid per RFC 3986, and the inbound `http_parser` already refuses a request
line containing either, so no request that reaches `SetHttpURL` from the wire
is affected. Only a caller that hand-built such a URL sees a change, and it now
gets `-1` plus a status instead of a split request.
Verified locally on macOS with the CMake build (`-DBUILD_UNIT_TESTS=ON`):
full library compiles clean, and `brpc_uri_unittest` (30),
`brpc_http_message_unittest` (28), `brpc_http_rpc_protocol_unittest` (62) and
`brpc_builtin_service_unittest` (19) all pass. `BuiltinServiceTest.dir` fails
identically with and without this patch on this machine because it reads
`/usr/include/sys/errno.h`, which the macOS SDK layout does not provide.
---
### 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]
