ftp: reject PWD responses containing control characters

[flightlesstux]

What

The ftp_pwd_resp() function in lib/ftp.c parses the quoted directory name out of a server's 257 PWD response and stores it as ftpc->entrypath. On connection reuse that string is passed directly to Curl_pp_sendf() as:

result = Curl_pp_sendf(data, &ftpc->pp, "CWD %s", ftpc->entrypath);

Curl_pp_vsendf() performs no sanitization - it formats the string, appends \r\n, and sends it. Because the extraction loop accepted any byte except \0 and ", a server could embed bare \r or other control characters inside the quoted path. Those bytes survive into the CWD command sent on the next connection reuse.

Example malicious 257 response:

257 "/legitimate/path\rSTOR evil.txt" is current directory

Some FTP server implementations treat a bare \r as a command separator, which turns this into a second injected command.

Fix

Reject the entire path during extraction if any byte with value < 0x20 or == 0x7f is encountered. This is consistent with how lib/cookie.c handles control bytes in cookie values.

Impact

Requires connecting to a malicious or MITM'd FTP server. The injected command is limited to what the server's own credentials allow. Severity is moderate but the fix is a one-liner guard with no effect on legitimate servers.

[bagder]

It would also be cool to have a test case verifying this.

Something like this:

<?xml version="1.0" encoding="US-ASCII"?>
<testcase>
<info>
<keywords>
FTP
PASV
RETR
</keywords>
</info>
# Server-side
<reply>
<data>
we can still send data even if pwd fails!
</data>
<servercmd>
REPLY PWD 257 %hex["/%03"]hex%
</servercmd>
</reply>

# Client-side
<client>
<server>
ftp
</server>
<name>
FTP with control characters in PWD response
</name>
<command>
ftp://%HOSTIP:%FTPPORT/%TESTNUMBER
</command>
</client>

# Verify data after the test has been "shot"
<verify>
<protocol crlf="yes">
USER anonymous
PASS ftp@example.com
PWD
EPSV
TYPE I
SIZE %TESTNUMBER
RETR %TESTNUMBER
QUIT
</protocol>
</verify>
</testcase>

[flightlesstux]

Thanks for the suggestion! I've added test3217 that uses the hex encoding to inject a control character (0x03) into the PWD response and verifies curl bails out with error code 8. Pushed it in the second commit. Let me know if the test format needs any tweaking or if you'd like a different test number range.

[bagder]

You did not incorporate my other proposals though, one of which causes the build error. Which indicates you did not compile this locally?

[flightlesstux]

Thanks for catching that. I've applied both inline suggestions - switched to ISCNTRL() with the curl_ctype.h include, and changed it back to returning CURLE_WEIRD_SERVER_REPLY with proper cleanup before the return. Updated the test to expect error code 8 and the shortened protocol exchange. Should be good now.

[bagder]

augment review

[augmentcode]

🤖 Augment PR Summary

Summary: This PR hardens FTP handling by rejecting malicious/invalid PWD (257) responses that contain ASCII control characters, preventing them from being persisted and later reused in commands.

Changes:

• Updates ftp_pwd_resp() in lib/ftp.c to abort extraction if any control byte is seen in the quoted path (via ISCNTRL()), returning CURLE_WEIRD_SERVER_REPLY.
• Adds proper dynbuf cleanup on early error returns during path extraction.
• Adjusts existing FTP test test1152 to expect failure on an invalid/unclosed-quote PWD response.
• Adds new regression tests (test3217, test3218) that inject control bytes into the PWD path using hex encoding.
• Registers the new test cases in tests/data/Makefile.am.

Technical Notes: The new guard prevents control characters (e.g., CR/LF) from reaching later CWD formatting/sending logic, mitigating potential command-injection behavior on some FTP servers.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

[bagder]

Thanks!