Skip to content

Security fixes - #24626

Closed
AmmaarBakshi wants to merge 8 commits into
php:masterfrom
AmmaarBakshi:security-fixes
Closed

AmmaarBakshi wants to merge 8 commits into
php:masterfrom
AmmaarBakshi:security-fixes

Conversation

@AmmaarBakshi

Copy link
Copy Markdown

No description provided.

SoapClient follows Location redirects to any host, and kept sending the
Authorization header built from the login and password options, as well
as the Authorization and Cookie headers of the http stream context, to
the redirect target. A redirect to another origin, including an https to
http downgrade, thus leaked the credentials.

Withhold them once a redirect leaves the origin, the same way the http
stream wrapper does since GHSA-fpwc-w8rq-cr92. Digest authentication is
not attempted against the new origin either.
The Digest Authorization header was built from the login option and the
realm, nonce, opaque and algorithm values of the server challenge without
the newline check that the other SoapClient request headers go through
since f6fe838. A CR/LF in the login thus injected arbitrary request
headers, while Basic authentication base64-encodes it.

Emit these values through soap_smart_str_append_header_value() as well.
php_mysqlnd_rowp_read_binary_protocol() skipped the status byte and the
(field_count + 9) / 8 bytes of the NULL bitmap and then read the bitmap
without checking that the row packet is that long. A malicious server
could send a shorter row packet and make the client read past it.

Fail with a warning when the packet cannot hold the status byte and the
bitmap, as is done for the field values since GHSA-r6x9-5r99-36j7.
The full status page encoded the request URI and the query string of
the processes for the html, xml and json formats since
GHSA-7qg2-v9fj-4mwv, but still printed the request method, the user and
the script verbatim. The user is the name of the HTTP Basic credentials
sent by the client, so any client could inject markup into the page an
administrator views, or break the JSON output.

Encode these values the same way, through a helper shared by all of them.
The %e, %f, %m, %q, %r and %u access log tokens wrote values that come
from the request verbatim. A client could thus forge access log lines,
e.g. with a line break in the name of its HTTP Basic credentials.

Write the control characters of these values as \xHH, as httpd and nginx
do.
SoapClient stored a cookie with whatever Domain attribute the server
sent, and later sent it to the hosts of that domain. Any server the
client talks to, including a redirect target, could thus set cookies for
unrelated hosts, e.g. to fixate a session.

Ignore such cookies, as RFC 6265 section 5.3 requires.
…rapper

php_fopen_do_pasv() copied a fixed sizeof(ip) bytes from the start of
the address in the 512 byte response buffer. An FTP server could pad the
227 reply so that the address starts less than that before the end of
the buffer, making the copy read past it.

Copy the address, which is NUL-terminated at that point, with strlcpy().
…rapper

When looking for the timestamp in the MDTM reply, php_stream_ftp_url_stat()
skipped non-digits up to the end of the 512 byte response buffer instead
of the end of the reply, and the bounds check that followed was off by
one. A reply without digits thus made sscanf() read past the buffer.

Stop at the end of the reply.
@kamil-tekiela

Copy link
Copy Markdown
Member

What is this supposed to be?

@AmmaarBakshi

Copy link
Copy Markdown
Author

What is this supposed to be?

These three files are the regression tests for three of the fixes on your security-fixes branch, written in php-src's .phpt format: a --TEST-- title, a --SKIPIF-- check, the PHP code under --FILE--, and the exact output it must print under --EXPECT--. ftp_stat_mdtm_no_digits.phpt and ftp_pasv_address_at_line_end.phpt start the fake FTP server from ext/ftp/tests/server.inc. That server sends malformed replies: an MDTM time with no digits, and a padded PASV reply that puts the address at the very end of the buffer. These used to make PHP read past a 512-byte stack buffer. The tests now expect a clean result instead: an unknown mtime of -1, or a "Failed to set up data channel" warning. status-full-encode-request-values.phpt starts PHP-FPM and sends a request with <script> in its method and Basic-auth username. It then fetches the /status?full page as HTML, XML and JSON and checks that the script is escaped in each format, which is the XSS fix. Each test fails or shows the bug on the old code and passes with the fix, so the bugs can't come back unnoticed. If you meant a different file, ask in the main conversation, since I can't open files from a side question.

@php php locked as spam and limited conversation to collaborators Oct 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants