Skip to content

Fix in-place URL decoding of interned userinfo in the http and ftp wrappers - #24341

Open
alepacheco wants to merge 1 commit into
php:PHP-8.5from
alepacheco:fix-http-wrapper-userinfo-decode
Open

alepacheco wants to merge 1 commit into
php:PHP-8.5from
alepacheco:fix-http-wrapper-userinfo-decode

Conversation

@alepacheco

Copy link
Copy Markdown

The http and ftp stream wrappers url-decode the user and password of the parsed URL in place (php_url_decode(ZSTR_VAL(resource->user), ...)), and the http wrapper does the same for user, password and path when it checks a redirect target for control characters.

Since the wrappers take a php_uri from uri_parser_class, those components can be interned strings: the WHATWG parser returns one-character usernames and passwords through ZVAL_STRINGL_FAST (the interned single-char strings), and the RFC 3986 parser returns the path / with ZVAL_CHAR. With uri_parser_class => Uri\WhatWg\Url, a redirect to http://+@host/ therefore rewrites the shared "+" string to " " for the rest of the process (with opcache, in shared memory).

This changes the wrappers to never write into php_uri fields:

  • http_fopen_wrapper.c: the Basic auth header and the redirect control-character check decode into new strings; the relative redirect path is built from the directory part of the current path instead of truncating resource->path in place (same resulting URL as before).
  • ftp_fopen_wrapper.c: USER/PASS are sent from decoded copies.

The new test follows a redirect with a one-character username under the WHATWG parser and checks both the Authorization header sent and that chr(43) is still "+" afterwards.

Targeting PHP-8.5, the first branch with uri_parser_class.

…appers

The http and ftp stream wrappers decoded the user and password (and,
when checking a redirect, the path) of the parsed URL in place. The
components of a php_uri may be interned strings (the WHATWG parser
returns one-character usernames and passwords as interned single-char
strings, the RFC 3986 parser returns the path "/" as one), so decoding a
redirect target such as http://+@host/ rewrote the shared "+" string for
the whole process.

Decode the components into new strings instead, and build the relative
redirect path without writing into the current path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant