Stephan Bergmann made libcurl return CURLE_URL_MALFORMAT if an FTP URL

contains %0a or %0d in the user, password or CWD parts. (A future fix would
include doing it for %00 as well - see KNOWN_BUGS for details.) Test case 225
and 226 were added to verify this

7 files changed, 80 insertions, 2 deletions

diff --git a/CHANGES b/CHANGES
index bedab831c..5959147f6 100644
--- a/CHANGES
+++ b/CHANGES
@@ -8,12 +8,18 @@
 
 
 Daniel (19 January 2005)
+- Stephan Bergmann made libcurl return CURLE_URL_MALFORMAT if an FTP URL
+  contains %0a or %0d in the user, password or CWD parts. (A future fix would
+  include doing it for %00 as well - see KNOWN_BUGS for details.) Test case
+  225 and 226 were added to verify this
+
 - Stephan Bergmann pointed out two flaws in libcurl built with HTTP disabled:
 
   1) the proxy environment variables are still read and used to set HTTP proxy
 
   2) you couldn't disable http proxy with CURLOPT_PROXY (since the option was
-     disabled)
+     disabled). This is important since apps may want to disable HTTP proxy
+     without actually knowing if libcurl was built to disable HTTP or not.
 
   Based on Stephan's patch, both these issues should now be fixed.
 
diff --git a/docs/KNOWN_BUGS b/docs/KNOWN_BUGS
index 1177b37e2..1bfe25bc2 100644
--- a/docs/KNOWN_BUGS
+++ b/docs/KNOWN_BUGS
@@ -3,6 +3,16 @@ join in and help us correct one or more of these! Also be sure to check the
 changelog of the current development status, as one or more of these problems
 may have been fixed since this was written!
 
+* FTP URLs passed to curl may contain NUL (0x00) in the RFC 1738 <user>,
+  <password>, and <fpath> components, encoded as "%00".  The problem is that
+  curl_unescape does not detect this, but instead returns a shortened C
+  string.  From a strict FTP protocol standpoint, NUL is a valid character
+  within RFC 959 <string>, so the way to handle this correctly in curl would
+  be to use a data structure other than a plain C string, one that can handle
+  embedded NUL characters.  From a practical standpoint, most FTP servers
+  would not meaningfully support NUL characters within RFC 959 <string>,
+  anyway (e.g., UNIX pathnames may not contain NUL).
+
 * Test case 241 fails on all systems that support IPv6 but that don't have the
   host name 'ip6-localhost' in /etc/hosts (or similar) since the test case
   uses that host name to test the IPv6 name to address resolver.
diff --git a/docs/TODO b/docs/TODO
index dbda55050..6a8b47994 100644
--- a/docs/TODO
+++ b/docs/TODO
@@ -65,6 +65,9 @@ TODO
 
  FTP
 
+ * Make the detection of (bad) %0d and %0a codes in FTP url parts earlier in
+   the process to avoid doing a resolve and connect in vain.
+
  * Code overhaul to make it more state-machine like and to _never_ block on
    waiting for server responses when used with the multi interface.
 
diff --git a/lib/ftp.c b/lib/ftp.c
index 0aa734e5b..ffec9c647 100644
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -149,6 +149,14 @@ static void freedirs(struct FTP *ftp)
   }
 }
 
+/* Returns non-zero iff the given string contains CR (0x0D) or LF (0x0A), which
+   are not allowed within RFC 959 <string>.
+ */
+static bool isBadFtpString(const char *string)
+{
+  return strchr(string, 0x0D) != NULL || strchr(string, 0x0A) != NULL;
+}
+
 /***********************************************************************
  *
  * AllowServerConnect()
@@ -474,6 +482,9 @@ CURLcode Curl_ftp_connect(struct connectdata *conn)
   /* no need to duplicate them, this connectdata struct won't change */
   ftp->user = conn->user;
   ftp->passwd = conn->passwd;
+  if (isBadFtpString(ftp->user) || isBadFtpString(ftp->passwd)) {
+    return CURLE_URL_MALFORMAT;
+  }
   ftp->response_time = 3600; /* set default response time-out */
 
 #ifndef CURL_DISABLE_HTTP
@@ -2738,6 +2749,10 @@ CURLcode ftp_parse_url_path(struct connectdata *conn)
         freedirs(ftp);
         return CURLE_OUT_OF_MEMORY;
       }
+      if (isBadFtpString(ftp->dirs[ftp->dirdepth])) {
+        freedirs(ftp);
+        return CURLE_URL_MALFORMAT;
+      }
     }
     else {
       cur_pos = slash_pos + 1; /* jump to the rest of the string */
@@ -2769,6 +2784,10 @@ CURLcode ftp_parse_url_path(struct connectdata *conn)
       failf(data, "no memory");
       return CURLE_OUT_OF_MEMORY;
     }
+    if (isBadFtpString(ftp->file)) {
+      freedirs(ftp);
+      return CURLE_URL_MALFORMAT;
+    }
   }
   else
     ftp->file=NULL; /* instead of point to a zero byte, we make it a NULL
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 79ee0609b..c284b27ba 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -31,7 +31,7 @@ EXTRA_DIST = test1 test108 test117 test127 test20 test27 test34 test46	\
  test517 test518 test210 test211 test212 test220 test221 test222	\
  test223 test224 test206 test207 test208 test209 test213 test240	\
  test241 test242 test519 test214 test215 test216 test217 test218	\
- test199
+ test199 test225
 
 # The following tests have been removed from the dist since they no longer
 # work. We need to fix the test suite's FTPS server first, then bring them
diff --git a/tests/data/test225 b/tests/data/test225
new file mode 100644
index 000000000..75d9dd90a
--- /dev/null
+++ b/tests/data/test225
@@ -0,0 +1,20 @@
+# Client-side
+<client>
+<server>
+ftp
+</server>
+ <name>
+FTP %0a-code in URL's name part
+ </name>
+ <command>
+ftp://bad%0auser:passwd@%HOSTIP:%FTPPORT/225%0a
+</command>
+</client>
+
+# Verify data after the test has been "shot"
+<verify>
+# 3 == CURLE_URL_MALFORMAT
+<errorcode>
+3
+</errorcode>
+</verify>
diff --git a/tests/data/test226 b/tests/data/test226
new file mode 100644
index 000000000..414438feb
--- /dev/null
+++ b/tests/data/test226
@@ -0,0 +1,20 @@
+# Client-side
+<client>
+<server>
+ftp
+</server>
+ <name>
+FTP %0d-code in URL's CWD part
+ </name>
+ <command>
+ftp://%HOSTIP:%FTPPORT/226%0d
+</command>
+</client>
+
+# Verify data after the test has been "shot"
+<verify>
+# 3 == CURLE_URL_MALFORMAT
+<errorcode>
+3
+</errorcode>
+</verify>