Repository navigation
Fix out-of-bounds reads parsing ftp:// wrapper server replies - #24340
Open
alepacheco wants to merge 1 commit into
Open
alepacheco wants to merge 1 commit into
alepacheco wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ftp:// wrapper reads server replies into a 512-byte
tmp_lineand 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 completedddstatus line. The callers then parse fromtmp_line + 4(PASV/EPSV,SIZE,MDTM), which for a short line such as213is past its terminating NUL.php_fopen_do_pasv()copies a fixedip_sizebytes from the dotted address insidetmp_line, so an address near the end of the buffer is read pasttmp_line, and an over-long one is silently truncated to a different address.MDTMhandler inphp_stream_ftp_url_stat()skips non-digits without stopping at the NUL, and its bound check uses>instead of>=, sosscanf()can start attmp_line + sizeof(tmp_line).This change makes
get_ftp_result()return a code only for a complete status line (and 0 otherwise), which keeps everytmp_line + 4reader inside the received line;php_fopen_do_pasv()now copies exactly the parsed address and rejects one that does not fit; and theMDTMscan 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 bare226, which RFC 959 does not allow; that reply used to be accepted from the stale buffer after the read timed out.#13717 enlarges the
ipbuffers; this change bounds the copy itself, so it is independent of the buffer size.The tests add
server.increplies for two cases: aPASVreply whose address ends at the buffer limit, whichfopen()now rejects (the old read past the buffer shows under ASan); and anMDTMreply without digits following a longerSIZEreply, for whichstat()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.