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

diff --git a/CHANGES b/CHANGES
index bedab831c9643531b928452ab307e99503c2b59f..5959147f66e5ffc95fe37361af1a08374116274a 100644 (file)
--- 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 1177b37e2d245d4739c541a51a6306da98be0cf6..1bfe25bc2ce0cd2e9df4eb1e10e620334537c343 100644 (file)
--- 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 dbda550509a984734fbeeddfbf0e2ea83343cb83..6a8b4799414ee176060234401be84f527eb0d1f8 100644 (file)
--- 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 0aa734e5b64c13858e25609e94ad9267f02ffdc8..ffec9c647d23c245bd69b92bfc7d2a6a7c2afc23 100644 (file)
--- 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 79ee0609b195add3e6fb5590393b7133f85059ad..c284b27ba822ca0c1ff21100e2c646cf48fae452 100644 (file)
--- 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 (file)
index 0000000..75d9dd9
--- /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 (file)
index 0000000..414438f
--- /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>