From: Ricardo Ferreira Ribeiro <garb12@pm.me>
Date: Fri, 19 Jun 2026 04:41:42 +0000 (+0000)
Subject: Do not send FTP commands with embedded CRs or LFs (#2444)
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=51d1ddb225f19a79672a3cc8dc5de5423dbfac11;p=thirdparty%2Fsquid.git

Do not send FTP commands with embedded CRs or LFs (#2444)

FTP command syntax prohibits bare CRs and LFs except for command
termination. FTP servers treat embedded CRs inconsistently. FTP does not
have a generally accepted escape mechanism for safely encoding those
message-framing characters. Thus, Squid must not send FTP commands with
parameters containing embedded CRs or LFs.

Squid assembles FTP commands from a variety of information sources that
depend, in part, on whether the master transaction originated on an
`http(s)_port` or `ftp_port`. One can check for (and reject) embedded
CRs and LFs in many code locations. Our attempts to reject invalid
inputs earlier, before the assembly, resulted in significant
functionality changes going beyond this fix scope but still missed an
important injection point.

The approach used here covers all known code paths, has a decent chance
of remaining effective during significant code refactoring, and does not
alter Squid functionality beyond killing transactions that resulted in
problematic FTP command parameters (an in-scope change). This approach
also allows request adaptation services to "fix" problematic requests in
some cases.

Rejected transactions usually result in HTTP 502 ERR_FTP_FAILURE
responses logged as TCP_MISS_ABORTED. More work is needed to provide
better %err_detail for these cases.

This is a Measurement Factory project.
---

diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc
index eb63585344..66a5689553 100644
--- a/src/clients/FtpClient.cc
+++ b/src/clients/FtpClient.cc
@@ -844,6 +844,21 @@ Ftp::Client::dataClosed(const CommCloseCbParams &)
 void
 Ftp::Client::writeCommand(const char *buf)
 {
+    // The caller must supply a non-empty command followed by CRLF.
+    // TODO: Move CRLF appending code from callers to here.
+    const auto bufLen = strlen(buf);
+    Assure(bufLen > 2);
+    Assure(buf[bufLen-2] == '\r');
+    Assure(buf[bufLen-1] == '\n');
+
+    const auto crlfCharPosition = strcspn(buf, crlf);
+    if (crlfCharPosition != bufLen-2) {
+        const auto invalidCharName = buf[crlfCharPosition] == '\r' ? "CR" : "LF";
+        debugs(9, 2, "ERROR: Caller assembled a malformed FTP command. Found " << invalidCharName << " at position " << crlfCharPosition);
+        failed(ERR_FTP_FAILURE, 0);
+        return;
+    }
+
     char *ebuf;
     /* trace FTP protocol communications at level 2 */
     debugs(9, 2, "ftp<< " << buf);