Skip to content

fix char-signedness out-of-bounds read in url and header lookup tables#3376

Open
ubeddulla wants to merge 1 commit into
apache:masterfrom
ubeddulla:char-signedness-oob
Open

fix char-signedness out-of-bounds read in url and header lookup tables#3376
ubeddulla wants to merge 1 commit into
apache:masterfrom
ubeddulla:char-signedness-oob

Conversation

@ubeddulla

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

Two +128-biased 256-entry lookup tables are read out of bounds when an input byte has the high bit set, on builds where char is unsigned (aarch64, riscv64).

  1. g_url_parsing_fast_action_map (uri.cpp, used by URI::SetHttpURL and ParseURL) is indexed with (int)*p, where *p walks the untrusted request-URI. On an unsigned-char platform a byte in 0x80..0xFF gives index 128..255, so raw[256..383] is read, up to 127 bytes past the table.
  2. g_tolower_map (ascii_tolower in case_ignored_flat_map.h) has the same +128 bias and is indexed by the promoted char argument. It is reached from CaseIgnoredHasher over HTTP header names and from HPACK, so the same high-bit byte over-reads the 256-entry table. The note already there records an aarch64 char-signedness crash; making the parameter int did not fix it, because a char argument still promotes to 0..255.

On x86_64 (char signed) both indices fall in [-128,127] and stay in bounds, which is why the current CI does not surface it.

What is changed and the side effects?

Changed:

Fold the index back into signed-char range at the three lookup sites with a (signed char) cast. The table layout is untouched and behavior on signed-char platforms is unchanged. Verified with an ASan build under -funsigned-char: a URL host byte 0xC3 and a header-name byte >= 0x80 each trip a buffer-overflow before the change and run clean after, and ascii_tolower output matches the expected value for all 256 byte values.

Side effects:

  • Performance effects: none (a single cast).

  • Breaking backward compatibility: no.


Check List:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant