Repository navigation
Fix out-of-bounds reads parsing ftp:// wrapper server replies - #24340
alepacheco wants to merge 1 commit into
Conversation
|
Friendly ping: this and its companion #24341 (http/ftp wrapper userinfo decoding, against PHP-8.5) are still waiting for a first look. Both still merge cleanly into their base branches; the WINDOWS_X64_ZTS failure is the proc_open quoting tests fixed by #24343, not this change. @bukka, @ndossche, could one of you take a look when you have time? Thanks! |
| @@ -77,10 +77,14 @@ typedef struct _php_ftp_dirstream_data { | |||
| static inline int get_ftp_result(php_stream *stream, char *buffer, size_t buffer_size) | |||
There was a problem hiding this comment.
Would be good if there's a doc block comment stating that the buffer is always NUL terminated, as that's what the calling code depends on for safety.
The reason is that either the read fails and then the 1st line puts a NUL; or it doesn't fail and php_stream_gets will put a NUL.
There was a problem hiding this comment.
Done: added a comment above get_ftp_result() stating the buffer is always NUL-terminated on return and why (folded into the same commit). Thanks for the review!
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.
1594905 to
cd2b268
Compare
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.