envoyproxy / envoyproxy/envoy

http1: unsafe ctype usage and size_t to int narrowing in the HTTP/1 parser

Closed
#46,505 5 comments 0 reactions 0 assignees View on GitHub
area/http bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.