ftp: reject control bytes in ACCT and alternative-to-user

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.

Closes #22301

diff --git a/lib/ftp.c b/lib/ftp.c
index b3da888b44e990d49fd9e0794c341b1e4c4b8542..f3c07d672296a2a7c651e83f8f07abffb5272b0b 100644 (file)
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -2927,6 +2927,20 @@ static CURLcode ftp_state_loggedin(struct Curl_easy *data,
   return result;
 }
 
+/* A value that becomes part of an FTP control command must not carry a
+   control byte: a CR or LF would end the command line and let a second
+   command be smuggled onto the control connection. */
+static bool ftp_has_ctrl(const char *string)
+{
+  const unsigned char *s = (const unsigned char *)string;
+  while(*s) {
+    if(*s < 0x20)
+      return TRUE;
+    s++;
+  }
+  return FALSE;
+}
+
 /* for USER and PASS responses */
 static CURLcode ftp_state_user_resp(struct Curl_easy *data,
                                     struct ftp_conn *ftpc,
@@ -2949,16 +2963,20 @@ static CURLcode ftp_state_user_resp(struct Curl_easy *data,
     result = ftp_state_loggedin(data, ftpc);
   }
   else if(ftpcode == 332) {
-    if(data->set.str[STRING_FTP_ACCOUNT]) {
-      result = Curl_pp_sendf(data, &ftpc->pp, "ACCT %s",
-                             data->set.str[STRING_FTP_ACCOUNT]);
-      if(!result)
-        ftp_state(data, ftpc, FTP_ACCT);
-    }
-    else {
+    const char *account = data->set.str[STRING_FTP_ACCOUNT];
+    if(!account) {
       failf(data, "ACCT requested but none available");
       result = CURLE_LOGIN_DENIED;
     }
+    else if(ftp_has_ctrl(account)) {
+      failf(data, "Control byte in FTP account");
+      result = CURLE_BAD_FUNCTION_ARGUMENT;
+    }
+    else {
+      result = Curl_pp_sendf(data, &ftpc->pp, "ACCT %s", account);
+      if(!result)
+        ftp_state(data, ftpc, FTP_ACCT);
+    }
   }
   else {
     /* All other response codes, like:
@@ -2966,14 +2984,19 @@ static CURLcode ftp_state_user_resp(struct Curl_easy *data,
     530 User ... access denied
     (the server denies to log the specified user) */
 
-    if(data->set.str[STRING_FTP_ALTERNATIVE_TO_USER] &&
-       !ftpc->ftp_trying_alternative) {
+    const char *alt = data->set.str[STRING_FTP_ALTERNATIVE_TO_USER];
+    if(alt && !ftpc->ftp_trying_alternative) {
       /* Ok, USER failed. Let's try the supplied command. */
-      result = Curl_pp_sendf(data, &ftpc->pp, "%s",
-                             data->set.str[STRING_FTP_ALTERNATIVE_TO_USER]);
-      if(!result) {
-        ftpc->ftp_trying_alternative = TRUE;
-        ftp_state(data, ftpc, FTP_USER);
+      if(ftp_has_ctrl(alt)) {
+        failf(data, "Control byte in FTP alternative-to-user command");
+        result = CURLE_BAD_FUNCTION_ARGUMENT;
+      }
+      else {
+        result = Curl_pp_sendf(data, &ftpc->pp, "%s", alt);
+        if(!result) {
+          ftpc->ftp_trying_alternative = TRUE;
+          ftp_state(data, ftpc, FTP_USER);
+        }
       }
     }
     else {

diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index e1249588745cb2919e02c25820b804adeed9d5d4..45df54464959a405603a0a3182dd49f2a51f7512 100644 (file)
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -254,7 +254,7 @@ test2072 test2073 test2074 test2075 test2076 test2077 test2078 test2079 \
 test2080 test2081 test2082 test2083 test2084 test2085 test2086 test2087 \
 test2088 test2089 test2090 test2091 test2092 \
 test2100 test2101 test2102 test2103 test2104 test2105 test2106 test2107 \
-test2108 test2109 test2110 test2113 test2114 \
+test2108 test2109 test2110 test2113 test2114 test2115 test2116 \
 \
 test2200 test2201 test2202 test2203 test2204 test2205 test2206 test2207 \
 test2208 \

diff --git a/tests/data/test2115 b/tests/data/test2115
new file mode 100644 (file)
index 0000000..d9c8b05
--- /dev/null
+++ b/tests/data/test2115
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="US-ASCII"?>
+<testcase>
+<info>
+<keywords>
+FTP
+ACCT
+</keywords>
+</info>
+# Server-side
+<reply>
+<servercmd>
+REPLY PASS 332 please provide account name
+</servercmd>
+</reply>
+
+# Client-side
+<client>
+<server>
+ftp
+</server>
+<name>
+FTP --ftp-account with embedded CRLF is rejected
+</name>
+# read the account from a config file so the raw CR LF reaches curl intact
+<file name="%LOGDIR/test%TESTNUMBER.config">
+ftp-account = "one\r\nDELE %TESTNUMBER"
+</file>
+<command>
+ftp://%HOSTIP:%FTPPORT/%TESTNUMBER -K %LOGDIR/test%TESTNUMBER.config
+</command>
+</client>
+
+# Verify data after the test has been "shot"
+<verify>
+# 43 - CURLE_BAD_FUNCTION_ARGUMENT
+<errorcode>
+43
+</errorcode>
+# the account is rejected before ACCT is built, so the injected DELE command
+# must never reach the server
+<protocol crlf="yes">
+USER anonymous
+PASS ftp@example.com
+</protocol>
+</verify>
+</testcase>

diff --git a/tests/data/test2116 b/tests/data/test2116
new file mode 100644 (file)
index 0000000..8ecd679
--- /dev/null
+++ b/tests/data/test2116
@@ -0,0 +1,45 @@
+<?xml version="1.0" encoding="US-ASCII"?>
+<testcase>
+<info>
+<keywords>
+FTP
+--ftp-alternative-to-user
+</keywords>
+</info>
+# Server-side
+<reply>
+<servercmd>
+REPLY USER 530 go away
+</servercmd>
+</reply>
+
+# Client-side
+<client>
+<server>
+ftp
+</server>
+<name>
+FTP --ftp-alternative-to-user with embedded CRLF is rejected
+</name>
+# read the command from a config file so the raw CR LF reaches curl intact
+<file name="%LOGDIR/test%TESTNUMBER.config">
+ftp-alternative-to-user = "USER me\r\nDELE %TESTNUMBER"
+</file>
+<command>
+ftp://%HOSTIP:%FTPPORT/%TESTNUMBER/ -K %LOGDIR/test%TESTNUMBER.config
+</command>
+</client>
+
+# Verify data after the test has been "shot"
+<verify>
+# 43 - CURLE_BAD_FUNCTION_ARGUMENT
+<errorcode>
+43
+</errorcode>
+# the replacement command is rejected before it is sent, so the injected DELE
+# command must never reach the server
+<protocol crlf="yes">
+USER anonymous
+</protocol>
+</verify>
+</testcase>