FTP: Reject commands with parameters containing CR (#2453)

FTP command syntax treats CR and LF characters as command terminators.
All RFC 959 sightings of CR and LF either refer to the CRLF terminator
or treat both characters the same way. For example, RFC 959 defines an
FTP command parameter as a sequence of chars, where:

    <char> ::= any of the 128 ASCII characters except <CR> and <LF>

Squid should (and now does) restrict CR use to the command terminator
sequence, especially since some FTP servers treat CRs as command
delimiters -- if we continue to allow embedded CRs in FTP command
parameters than our FtpClient::writeCommand() will assert when trying to
forward those commands to the FTP server. Moreover, we already use that
CR treatment when _parsing_ FTP responses (see
Ftp::Client::parseControlReply()).

When it comes to command termination, CRs are still optional.

A new ban on CRs in FTP command parameter values means that Squid starts
treating some FTP commands as syntactically invalid, using
EarlyErrorKind::MalformedCommand for the first time since its inception
in 2014 commit eacfca83. For example, `PWD\rQUIT` input is now invalid.

This is a Measurement Factory project.

diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc
index bb236a3bc6e44a4dde9e9aefb387ac982cfd7dc7..1a851ad73cb735336f054c97274a91025cb70ebf 100644 (file)
--- a/src/servers/FtpServer.cc
+++ b/src/servers/FtpServer.cc
@@ -641,15 +641,31 @@ Ftp::Server::parseOneRequest()
 {
     flags.readMore = false; // common for all but one case below
 
-    // OWS <command> [ RWS <parameter> ] OWS LF
-
-    // InlineSpaceChars are isspace(3) or RFC 959 Section 3.1.1.5.2, except
-    // for the LF character that we must exclude here (but see FullWhiteSpace).
-    static const char * const InlineSpaceChars = " \f\r\t\v";
+    // FTP command syntax is specified in RFC 959 Section 5.3. We generalize
+    // that grammar to parse all commands using the same code. We also relax the
+    // rules a little in hope to accommodate more real world use cases:
+    // * We allow ASCII HT, VT, and NP characters in various delimiters (in addition to SP).
+    // * We allow zero or more CR characters in command terminator (instead of exactly one).
+    // * We allow space characters before any command.
+    //
+    // command = BWS code [ RWS parameter ] OWS *CR LF
+    // code = 1*code_char
+    // parameter = 1*parameter_char ; without leading and trailing inner_space_chars
+    // code_char = ; any ASCII character other than space_char
+    // parameter_char = ; any ASCII character other than CR or LF
+    // BWS = 0*space_char ; optional "bad" space before the command code
+    // RWS = 1*inner_space_char ; required space before the command parameter
+    // OWS = 0*inner_space_char ; optional space after the command parameter
+    // inner_space_char = SP / HT / VT / NP ; space_char without CR and LF
+    // space_char = SP / HT / VT / NP / CR / LF; any isspace(3) character in "C" locale
+
+    static const auto InlineSpaceChars = " \f\t\v";
     static const CharacterSet InlineSpace = CharacterSet("Ftp::Inline", InlineSpaceChars);
-    static const CharacterSet FullWhiteSpace = (InlineSpace + CharacterSet::LF).rename("Ftp::FWS");
+    static const CharacterSet CrLfChars = (CharacterSet::CR + CharacterSet::LF).rename("CRLF");
+    static const CharacterSet FullWhiteSpace = (InlineSpace + CrLfChars).rename("Ftp::FWS");
     static const CharacterSet CommandChars = FullWhiteSpace.complement("Ftp::Command");
-    static const CharacterSet TailChars = CharacterSet::LF.complement("Ftp::Tail");
+    // RFC 959 Section 5.3.2 excludes both CR and LF from <char> and <pr-char> definitions
+    static const CharacterSet TailChars = CrLfChars.complement("Ftp::Tail");
 
     // This set is used to ignore empty commands without allowing an attacker
     // to keep us endlessly busy by feeding us whitespace or empty commands.
@@ -663,7 +679,7 @@ Ftp::Server::parseOneRequest()
     (void)tok.skipAll(LeadingSpace); // leading OWS and empty commands
     const bool parsed = tok.prefix(cmd, CommandChars); // required command
 
-    // note that the condition below will eat either RWS or trailing OWS
+    // note that the condition below eats leading RWS and trailing OWS, if any
     if (parsed && tok.skipAll(InlineSpace) && tok.prefix(params, TailChars)) {
         // now params may include trailing OWS
         // TODO: Support right-trimming using CharacterSet in Tokenizer instead
@@ -682,8 +698,21 @@ Ftp::Server::parseOneRequest()
         return earlyError(EarlyErrorKind::HugeRequest);
     }
 
+    if (parsed)
+        (void)tok.skipAll(CharacterSet::CR);
+
     // technically, we may skip multiple NLs below, but that is OK
     if (!parsed || !tok.skipAll(CharacterSet::LF)) { // did not find terminating LF yet
+
+        if (!tok.remaining().isEmpty()) {
+            // We always consume all valid input, so any leftovers imply that we
+            // found something that we cannot parse now and will never parse if
+            // more input becomes available later (e.g., `PWD\rQUIT\n`).
+            changeState(fssError, "bad FTP command syntax");
+            quitAfterError(nullptr);
+            return earlyError(EarlyErrorKind::MalformedCommand);
+        }
+
         // we need more data, but can we buffer more?
         if (inBuf.length() >= Config.maxRequestHeaderSize) {
             changeState(fssError, "huge req");