Skip to content

Fix out-of-bounds reads parsing ftp:// wrapper server replies - #24340

Open
alepacheco wants to merge 1 commit into
php:PHP-8.4from
alepacheco:fix-ftp-wrapper-reply-bounds
Open

alepacheco wants to merge 1 commit into
php:PHP-8.4from
alepacheco:fix-ftp-wrapper-reply-bounds

Conversation

@alepacheco

Copy link
Copy Markdown

The ftp:// wrapper reads server replies into a 512-byte tmp_line and then parses them at fixed offsets and sizes that are not tied to the line actually received:

  • get_ftp_result() returns the code of whatever line it read last when the stream ends, even if that line is not a complete ddd status line. The callers then parse from tmp_line + 4 (PASV/EPSV, SIZE, MDTM), which for a short line such as 213 is past its terminating NUL.
  • php_fopen_do_pasv() copies a fixed ip_size bytes from the dotted address inside tmp_line, so an address near the end of the buffer is read past tmp_line, and an over-long one is silently truncated to a different address.
  • The MDTM handler in php_stream_ftp_url_stat() skips non-digits without stopping at the NUL, and its bound check uses > instead of >=, so sscanf() can start at tmp_line + sizeof(tmp_line).

This change makes get_ftp_result() return a code only for a complete status line (and 0 otherwise), which keeps every tmp_line + 4 reader inside the received line; php_fopen_do_pasv() now copies exactly the parsed address and rejects one that does not fit; and the MDTM scan stops at the end of the line.

Every caller already treats 0 as a failure (it is what get_ftp_result() returned when nothing could be read at all). The only visible change is for a server that ends the connection after a reply without the space after the code, such as a bare 226, which RFC 959 does not allow; that reply used to be accepted from the stale buffer after the read timed out.

#13717 enlarges the ip buffers; this change bounds the copy itself, so it is independent of the buffer size.

The tests add server.inc replies for two cases: a PASV reply whose address ends at the buffer limit, which fopen() now rejects (the old read past the buffer shows under ASan); and an MDTM reply without digits following a longer SIZE reply, for which stat() now reports an unknown mtime instead of the timestamp left in the buffer by the previous reply.

Targeting PHP-8.4 as the oldest branch with the bug in active support.

The ftp:// wrapper parses replies in a 512-byte buffer at fixed offsets
and sizes that are not tied to the line actually received:

- get_ftp_result() returns the code of the last line read when the
  stream ends, even if it is not a complete "ddd " status line, while
  the callers parse the reply text from tmp_line + 4.
- php_fopen_do_pasv() copies a fixed ip_size bytes from the dotted
  address inside the buffer, reading past its end when the address is
  near the end of the line, and silently truncating an over-long one.
- The MDTM parsing in php_stream_ftp_url_stat() skips non-digits past
  the terminating NUL, and its bound check allows sscanf() to start one
  past the end of the buffer.

Only return a code from get_ftp_result() for a complete status line,
copy exactly the parsed address and reject one that does not fit, and
stop the MDTM scan at the end of the line.
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