Skip to content

Fix out-of-bounds read on empty Location header in HTTP wrapper - #23467

Closed
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:fix/http-empty-location-84
Closed

Fix out-of-bounds read on empty Location header in HTTP wrapper#23467
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:fix/http-empty-location-84

Conversation

@iliaal

@iliaal iliaal commented Aug 25, 2026

Copy link
Copy Markdown
Member

When a server sends a redirect with an empty Location header, the wrapper allocates a single byte for it and the relative-redirect branch then reads location[1], one byte past the allocation, so a hostile server can make the over-read pick up heap garbage and turn the redirect target into the current path plus junk instead of the host root. The second-byte dereference is now guarded by header_info.location_len; an empty Location deterministically redirects to the host root. Sibling audit found no other unguarded indexing of header_info.location.

@iliaal
iliaal requested a review from bukka as a code owner August 25, 2026 23:27
@iliaal iliaal mentioned this pull request Aug 25, 2026
Closed
laruence
laruence reviewed Aug 26, 2026
View reviewed changes
Comment thread ext/standard/http_fopen_wrapper.c Outdated
char *loc_path = NULL;
if (*header_info.location != '/') {
if (*(header_info.location+1) != '\0' && resource->path) {
if (header_info.location_len > 0 && *(header_info.location+1) != '\0' && resource->path) {

@laruence laruence Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, this might not be related to your PR, but since it already have location_len, why it still call strlen(location) above?

@iliaal iliaal Aug 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to location_len.

@devnexen devnexen Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you still need *(header_info.location+1) != '\0' tough ?

@iliaal iliaal Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Empty Location is length 0. A 1-char relative path is valid, so location_len > 0 is enough.

iliaal added a commit to iliaal/php-src that referenced this pull request Aug 26, 2026
@iliaal
[http] Fix out-of-bounds read on empty Location header
330bb1e 
An empty Location header allocates a single byte for the NUL
terminator, so reading location[1] in the relative-redirect branch
over-reads heap memory and could append a garbage-derived path to the
redirect target instead of the correct host root. Guard the read with
location_len before dereferencing the second byte, and use that length
instead of strlen for the absolute-URL check.

Closes phpGH-23467
@iliaal
iliaal force-pushed the fix/http-empty-location-84 branch from 5e412a6 to 330bb1e Compare August 26, 2026 12:22
iliaal added a commit to iliaal/php-src that referenced this pull request Aug 26, 2026
@iliaal
[http] Fix out-of-bounds read on empty Location header
8a54f0a 
An empty Location header allocates a single byte for the NUL
terminator, so reading location[1] in the relative-redirect branch
over-reads heap memory and could append a garbage-derived path to the
redirect target instead of the correct host root. Use location_len
instead of strlen, and require location_len > 1 before treating the
value as a relative path, so the second byte is never read.

Closes phpGH-23467
@iliaal
iliaal force-pushed the fix/http-empty-location-84 branch 2 times, most recently from 330bb1e to 8a54f0a Compare August 26, 2026 20:43
@iliaal
[http] Fix out-of-bounds read on empty Location header
231f29f 
An empty Location header allocates a single byte for the NUL
terminator, so reading location[1] in the relative-redirect branch
over-reads heap memory and could append a garbage-derived path to the
redirect target instead of the correct host root. Use location_len
instead of strlen, and skip the relative join when location_len is 0,
so the second byte is never read.

Closes phpGH-23467
@iliaal
iliaal force-pushed the fix/http-empty-location-84 branch from 8a54f0a to 231f29f Compare August 26, 2026 20:53
@iliaal
iliaal requested review from devnexen and laruence August 26, 2026 20:55
@iliaal iliaal closed this in 8196275 Aug 27, 2026
jordikroon added a commit to jordikroon/php-src that referenced this pull request Aug 31, 2026
@jordikroon
ext/standard: Fix 1-char relative Location redirects after phpGH-23467
0ef62f9
@jordikroon jordikroon mentioned this pull request Aug 31, 2026
Closed
jordikroon added a commit to jordikroon/php-src that referenced this pull request Aug 31, 2026
@jordikroon
ext/standard: Fix 1-char relative Location redirects after phpGH-23467
f4a9152
jordikroon added a commit to jordikroon/php-src that referenced this pull request Aug 31, 2026
@jordikroon
ext/standard: Fix 1-char relative Location redirects after phpGH-23467
4c69c45
iliaal added a commit that referenced this pull request Sep 1, 2026
@iliaal
Merge branch 'PHP-8.5'
86c8fed 
* PHP-8.5:
  ext/standard: Fix 1-char relative Location redirects after GH-23467
pull Bot pushed a commit to dolfly/php-src that referenced this pull request Sep 1, 2026
@jordikroon @iliaal
ext/standard: Fix 1-char relative Location redirects after phpGH-23467
de3436c 
8196275 changed the relative-Location check from location_len > 1 to
> 0, so a single-character Location began resolving against the request
path instead of the host root as before.

Closes phpGH-23521
pull Bot pushed a commit to dolfly/php-src that referenced this pull request Sep 1, 2026
@iliaal
Merge branch 'PHP-8.4' into PHP-8.5
7724832 
* PHP-8.4:
  ext/standard: Fix 1-char relative Location redirects after phpGH-23467
bukka added a commit that referenced this pull request Sep 22, 2026
@bukka
Merge branch 'PHP-8.2' into PHP-8.3
f9e0418 
* PHP-8.2:
  ext/soap: make GHSA-cj93-vc83-wgqv test lean and reliable
  Add NEWS entries
  ext/standard: Fix 1-char relative Location redirects after GH-23467
  [http] Fix out-of-bounds read on empty Location header
  Fix GHSA-ch8v-r6jh-4vvr: encode 0xFF in FILTER_SANITIZE_ENCODED
  Fix GHSA-9f67-6fw4-hpfp
  Fix GHSA-j3wh-g957-2m85: phar tar entry injection
  Fix GHSA-cj93-vc83-wgqv
  Fix GHSA-rgrp-mwpx-f6rm: unbounded recursion in ext/soap XML parsing and decoding
  Fix GHSA-fpwc-w8rq-cr92: strip credentials from user headers on cross-origin redirects
  Fix GHSA-r6x9-5r99-36j7: Various packet overreads in mysqlnd wireprotocol
  Fix GHSA-xr7j-rvgx-xq5p: OOB read in php_openssl_matches_wildcard_name()
  Fix GHSA-vvx9-73fr-5jjx: do not fall back to CN if the cert has a service ID
  Fix GHSA-62xp-839h-2637: FastCGI allowed_clients compared only 96 bits of IPv6 addresses
  Fix heap-buffer-overflow in convert stream filters with NUL in line-break-chars
bukka added a commit that referenced this pull request Sep 22, 2026
@bukka
Merge branch 'PHP-8.3' into PHP-8.4
2cd1675 
* PHP-8.3:
  ext/soap: make GHSA-cj93-vc83-wgqv test lean and reliable
  Add NEWS entries
  ext/standard: Fix 1-char relative Location redirects after GH-23467
  [http] Fix out-of-bounds read on empty Location header
  Fix GHSA-ch8v-r6jh-4vvr: encode 0xFF in FILTER_SANITIZE_ENCODED
  Fix GHSA-9f67-6fw4-hpfp
  Fix GHSA-j3wh-g957-2m85: phar tar entry injection
  Fix GHSA-cj93-vc83-wgqv
  Fix GHSA-rgrp-mwpx-f6rm: unbounded recursion in ext/soap XML parsing and decoding
  Fix GHSA-fpwc-w8rq-cr92: strip credentials from user headers on cross-origin redirects
  Fix GHSA-r6x9-5r99-36j7: Various packet overreads in mysqlnd wireprotocol
  Fix GHSA-xr7j-rvgx-xq5p: OOB read in php_openssl_matches_wildcard_name()
  Fix GHSA-vvx9-73fr-5jjx: do not fall back to CN if the cert has a service ID
  Fix GHSA-62xp-839h-2637: FastCGI allowed_clients compared only 96 bits of IPv6 addresses
  Fix heap-buffer-overflow in convert stream filters with NUL in line-break-chars

# Conflicts:
#	ext/openssl/xp_ssl.c
bukka added a commit that referenced this pull request Sep 22, 2026
@bukka
Merge branch 'PHP-8.4' into PHP-8.5
2ef7525 
* PHP-8.4:
  Skip bz2 GH-20807 test when less than 13 GiB of memory is available
  ext/soap: make GHSA-cj93-vc83-wgqv test lean and reliable
  Add NEWS entries
  ext/standard: Fix 1-char relative Location redirects after GH-23467
  [http] Fix out-of-bounds read on empty Location header
  Fix GHSA-ch8v-r6jh-4vvr: encode 0xFF in FILTER_SANITIZE_ENCODED
  Fix GHSA-9f67-6fw4-hpfp
  Fix GHSA-j3wh-g957-2m85: phar tar entry injection
  Fix GHSA-cj93-vc83-wgqv
  Fix GHSA-rgrp-mwpx-f6rm: unbounded recursion in ext/soap XML parsing and decoding
  Fix GHSA-fpwc-w8rq-cr92: strip credentials from user headers on cross-origin redirects
  Fix GHSA-r6x9-5r99-36j7: Various packet overreads in mysqlnd wireprotocol
  Fix GHSA-xr7j-rvgx-xq5p: OOB read in php_openssl_matches_wildcard_name()
  Fix GHSA-vvx9-73fr-5jjx: do not fall back to CN if the cert has a service ID
  Fix GHSA-62xp-839h-2637: FastCGI allowed_clients compared only 96 bits of IPv6 addresses
  Fix heap-buffer-overflow in convert stream filters with NUL in line-break-chars

# Conflicts:
#	NEWS
#	ext/phar/tar.c
#	ext/soap/php_http.c
#	ext/soap/php_xml.c
#	ext/standard/http_fopen_wrapper.c
#	win32/ioutil.c
bukka added a commit that referenced this pull request Sep 22, 2026
@bukka
Merge branch 'PHP-8.5'
48e53b0 
* PHP-8.5:
  Skip bz2 GH-20807 test when less than 13 GiB of memory is available
  ext/soap: make GHSA-cj93-vc83-wgqv test lean and reliable
  Add NEWS entries
  ext/standard: Fix 1-char relative Location redirects after GH-23467
  [http] Fix out-of-bounds read on empty Location header
  Fix GHSA-ch8v-r6jh-4vvr: encode 0xFF in FILTER_SANITIZE_ENCODED
  Fix GHSA-9f67-6fw4-hpfp
  Fix GHSA-j3wh-g957-2m85: phar tar entry injection
  Fix GHSA-cj93-vc83-wgqv
  Fix GHSA-rgrp-mwpx-f6rm: unbounded recursion in ext/soap XML parsing and decoding
  Fix GHSA-fpwc-w8rq-cr92: strip credentials from user headers on cross-origin redirects
  Fix GHSA-r6x9-5r99-36j7: Various packet overreads in mysqlnd wireprotocol
  Fix GHSA-xr7j-rvgx-xq5p: OOB read in php_openssl_matches_wildcard_name()
  Fix GHSA-vvx9-73fr-5jjx: do not fall back to CN if the cert has a service ID
  Fix GHSA-62xp-839h-2637: FastCGI allowed_clients compared only 96 bits of IPv6 addresses
  Fix heap-buffer-overflow in convert stream filters with NUL in line-break-chars

# Conflicts:
#	NEWS
#	ext/openssl/xp_ssl.c
#	ext/phar/tar.c
#	ext/soap/php_http.c
#	ext/standard/http_fopen_wrapper.c
devnexen pushed a commit to devnexen/php-src that referenced this pull request Sep 22, 2026
@iliaal @bukka
[http] Fix out-of-bounds read on empty Location header
547566f 
An empty Location header allocates a single byte for the NUL
terminator, so reading location[1] in the relative-redirect branch
over-reads heap memory and could append a garbage-derived path to the
redirect target instead of the correct host root. Use location_len
instead of strlen, and skip the relative join when location_len is 0,
so the second byte is never read.

Closes phpGH-23467
devnexen pushed a commit to devnexen/php-src that referenced this pull request Sep 22, 2026
@jordikroon @bukka
ext/standard: Fix 1-char relative Location redirects after phpGH-23467
6466475 
8196275 changed the relative-Location check from location_len > 1 to
> 0, so a single-character Location began resolving against the request
path instead of the host root as before.

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

Reviewers

@devnexen devnexen devnexen approved these changes
@bukka bukka Awaiting requested review from bukka bukka is a code owner
+1 more reviewer
@laruence laruence laruence approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Extension: standard

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iliaal @laruence @devnexen