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 eb635853445c33ba6b5a4bbc83932b236396ecfb..66a56895530f6a53e171b93cf0354e4645c67db7 100644 (file)
--- 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);