Skip to content

ext/standard: check the from address for control characters before PASS - #22770

Open
iliaal wants to merge 1 commit into
php:masterfrom
iliaal:fix/ftp-pass-from-address-cntrl
Open

ext/standard: check the from address for control characters before PASS#22770
iliaal wants to merge 1 commit into
php:masterfrom
iliaal:fix/ftp-pass-from-address-cntrl

Conversation

@iliaal

@iliaal iliaal commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

GH-17976 sanitized from and user_agent at the HTTP wrapper's header-emit point and deferred the FTP wrapper. php_ftp_fopen_connect() still sends FG(from_address) as the anonymous password unchecked, so with ini_set('from', "evil\r\nSITE INJECT") the server receives a literal SITE INJECT on the control channel. The URL-supplied-password branch a few lines above already calls PHP_FTP_CNTRL_CHK; this uses the same macro over ZSTR_LEN, so the two branches now read identically.

@iliaal
iliaal requested a review from bukka as a code owner July 16, 2026 17:00
iliaal added a commit to iliaal/php-src that referenced this pull request Jul 16, 2026
phpGH-17976 sanitized from and user_agent where the HTTP wrapper emits headers,
and left the FTP wrapper for a follow-up. php_ftp_fopen_connect() sends
FG(from_address) as the anonymous password without a check, so an ini_set()
value carrying CRLF puts extra commands on the control channel. Apply
PHP_FTP_CNTRL_CHK, as the branch that sends a URL-supplied password already
does.

Closes phpGH-22770
@iliaal
iliaal force-pushed the fix/ftp-pass-from-address-cntrl branch from b967f41 to 2796857 Compare July 16, 2026 17:00

@Sjord Sjord left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It indeed makes sense to check from. I left some comments with possible improvements.

Comment thread ext/standard/tests/streams/ftp_from_address_cntrl.phpt Outdated
Comment thread ext/standard/ftp_fopen_wrapper.c Outdated
Comment thread ext/standard/tests/streams/ftp_from_address_cntrl.phpt
Comment thread ext/standard/tests/streams/ftp_from_address_cntrl.phpt
phpGH-17976 sanitized from and user_agent where the HTTP wrapper emits headers,
and left the FTP wrapper for a follow-up. php_ftp_fopen_connect() sends
FG(from_address) as the anonymous password without a check, so an ini_set()
value carrying CRLF puts extra commands on the control channel. Apply
PHP_FTP_CNTRL_CHK, as the branch that sends a URL-supplied password already
does.

Closes phpGH-22770
@iliaal
iliaal force-pushed the fix/ftp-pass-from-address-cntrl branch from 2796857 to d241ddc Compare August 24, 2026 12:40
@iliaal
iliaal requested a review from Sjord August 24, 2026 12:49

@Sjord Sjord left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

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.

2 participants