http1: unsafe ctype usage and size_t to int narrowing in the HTTP/1 parser
- Dominant language
- C++
- Stars
- 28.9k
- Forks
- 5.6k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 437
Description
*Title*: http1: unsafe ctype usage and size_t to int narrowing in the HTTP/1 parser
*Description*:
Two type-safety defects in the HTTP/1 parser, found while working on #46496. Neither appears to be a security issue. There's no crash, no memory-safety consequence on supported platforms, and no reachable trigger for (2), so I'm raising publicly. Happy to move this to a security advisory if a maintainer disagrees.
**1. ctype calls on raw `char` in `isUrlValid()`** - `balsa_parser.cc:114,120,139`
`std::isalpha`/`isdigit`/`isalnum` require an argument representable as `unsigned char` or `EOF`. Where `char` is signed (x86-64 Linux), a request-target byte >= 0x80 arrives as a negative `int` which is undefined behaviour, and reachable from untrusted input:
`isUrlValid()` is called from `OnRequestFirstLineInput()` and line 114 runs before any range check, because [this line](https://github.com/envoyproxy/envoy/blob/main/source/common/http/http1/balsa_parser.cc#L93) only short-circuits targets beginning `/` or `*`. These functions are also locale-sensitive by contract.
No impact in practice: glibc's ctype table is defined for -128..255, musl handles negatives, and Envoy never calls `setlocale`.
`isVersionValid()` in the same file (line 164) already uses `absl::ascii_isdigit`.
Switching the three sites to the `absl::ascii_*` equivalents is locale-independent and well-defined, and `absl/strings/ascii.h` is already included. Happy to send that PR.
**2. `size_t` to `int` narrowing on the parse path** - `parser.h:121`
[Parser::execute()](https://github.com/envoyproxy/envoy/blob/main/source/common/http/http1/parser.h#L121) takes `int len`, but is fed `RawSlice::len_` (a `size_t`, `buffer.h:32`) via [dispatchSlice()](https://github.com/envoyproxy/envoy/blob/main/source/common/http/http1/codec_impl.cc#L713).
A slice above `INT_MAX` truncates negative; every guard in `BalsaParser::execute()` tests `> 0` or `== 0` (lines 217, 238, 245), so all are skipped and line 251 hands the negative value to `BalsaFrame::ProcessInput()`, where it widens to a large `size_t`. `LegacyHttpParserImpl` has the same shape (`legacy_parser_impl.cc:83,141`). The bound on the returned count in `codec_impl.cc` is a debug-only `ASSERT`.
Envoy's read path never produces slices near 2 GB, so this is latent rather than triggerable. The fix touches `Parser`, `BalsaParser` and `LegacyHttpParserImpl` - worth a maintainer's view on scope before anyone writes it.
[optional *Relevant Links*:]
1. https://github.com/envoyproxy/envoy/pull/46496 - the PR this was found alongside
2. https://en.cppreference.com/w/cpp/string/byte/isalpha - argument requirements for the ctype functions
Contributor guide
Research direction
Start in source/common/http/http1/balsa_parser.cc at isUrlValid(), OnRequestFirstLineInput(), and isVersionValid(), then inspect parser.h:121 and dispatchSlice() in codec_impl.cc. Trace the related LegacyHttpParserImpl path in legacy_parser_impl.cc; done means the ctype calls are well-defined and locale-independent, and the identified size conversions are handled consistently across Parser, BalsaParser, and LegacyHttpParserImpl.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100