ftp: reject control bytes in ACCT and alternative-to-user - #22301
Closed
alhudz wants to merge 1 commit into
Closed
Conversation
A CR or LF in the CURLOPT_FTP_ACCOUNT or CURLOPT_FTP_ALTERNATIVE_TO_USER string split the control-channel command line and smuggled a second FTP command. Reject a byte below 0x20 in both values before the command is built.
bagder
approved these changes
Jul 12, 2026
There was a problem hiding this comment.
Pull request overview
This PR closes an FTP command-injection gap by rejecting control bytes (e.g., CR/LF) in two option-sourced strings that are incorporated into FTP control commands in ftp_state_user_resp().
Changes:
- Add a small helper to detect control bytes in strings used in FTP control commands and apply it to
CURLOPT_FTP_ACCOUNT(ACCT) andCURLOPT_FTP_ALTERNATIVE_TO_USER. - Return
CURLE_BAD_FUNCTION_ARGUMENTwith an explanatory error when either value contains a control byte. - Add new tests to ensure embedded CRLF in these option values is rejected and does not reach the server.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/ftp.c | Reject control bytes in ftp-account and ftp-alternative-to-user values before assembling/sending FTP control commands. |
| tests/data/test2115 | New regression test ensuring CRLF injection via ftp-account is rejected and the injected command is not sent. |
| tests/data/test2116 | New regression test ensuring CRLF injection via ftp-alternative-to-user is rejected and the injected command is not sent. |
| tests/data/Makefile.am | Register new tests 2115 and 2116 in the test data makefile list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
ftp_state_user_resp()writesCURLOPT_FTP_ACCOUNTinto anACCTcommand andCURLOPT_FTP_ALTERNATIVE_TO_USERas a replacement for theUSERline, both straight throughCurl_pp_sendf()with no control-byte check. Once a server answersUSERwith332, an account ofone\r\nDELE fgoes out asACCT onefollowed by a separateDELE fline; the alternative-to-user string smuggles a command the same way when the server rejectsUSER. Tests2115and2116drive both paths and show the injectedDELEreaching the server on the current code.The URL path and the
USER/PASScredentials already reject control octets (REJECT_CTRL/str_has_ctrl); these two option-sourced command fields were the gap. Reject a byte below0x20in both values inftp_state_user_resp()before the command line is assembled, so the guard sits where the command is built rather than in each option setter.