Blob Blame Raw
From 5452fdc5ae93f3571074c591fdf28cdf630796a0 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Tue, 12 Sep 2017 09:29:01 +0200
Subject: [PATCH 1/3] FTP: URL decode path for dir listing in nocwd mode

Reported-by: Zenju on github

Test 244 added to verify
Fixes #1974
Closes #1976

Upstream-commit: ecf21c551fa3426579463abe34b623111b8d487c
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 lib/ftp.c              | 93 +++++++++++++++++++++++---------------------------
 tests/data/Makefile.am |  3 +-
 tests/data/test244     | 54 +++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 51 deletions(-)
 create mode 100644 tests/data/test244

diff --git a/lib/ftp.c b/lib/ftp.c
index bcba6bb..fb3a716 100644
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -1003,7 +1003,7 @@ static CURLcode ftp_state_use_port(struct connectdata *conn,
     char *port_start = NULL;
     char *port_sep = NULL;
 
-    addr = calloc(addrlen+1, 1);
+    addr = calloc(addrlen + 1, 1);
     if(!addr)
       return CURLE_OUT_OF_MEMORY;
 
@@ -1041,7 +1041,7 @@ static CURLcode ftp_state_use_port(struct connectdata *conn,
     /* parse the port */
     if(ip_end != NULL) {
       if((port_start = strchr(ip_end, ':')) != NULL) {
-        port_min = curlx_ultous(strtoul(port_start+1, NULL, 10));
+        port_min = curlx_ultous(strtoul(port_start + 1, NULL, 10));
         if((port_sep = strchr(port_start, '-')) != NULL) {
           port_max = curlx_ultous(strtoul(port_sep + 1, NULL, 10));
         }
@@ -1469,25 +1469,22 @@ static CURLcode ftp_state_post_listtype(struct connectdata *conn)
      then just do LIST (in that case: nothing to do here)
   */
   char *cmd,*lstArg,*slashPos;
+  const char *inpath = data->state.path;
 
   lstArg = NULL;
   if((data->set.ftp_filemethod == FTPFILE_NOCWD) &&
-     data->state.path &&
-     data->state.path[0] &&
-     strchr(data->state.path,'/')) {
-
-    lstArg = strdup(data->state.path);
-    if(!lstArg)
-      return CURLE_OUT_OF_MEMORY;
+     inpath && inpath[0] && strchr(inpath, '/')) {
+    size_t n = strlen(inpath);
 
     /* Check if path does not end with /, as then we cut off the file part */
-    if(lstArg[strlen(lstArg) - 1] != '/')  {
-
+    if(inpath[n - 1] != '/') {
       /* chop off the file part if format is dir/dir/file */
-      slashPos = strrchr(lstArg,'/');
-      if(slashPos)
-        *(slashPos+1) = '\0';
+      slashPos = strrchr(inpath, '/');
+      n = slashPos - inpath;
     }
+    result = Curl_urldecode(data, inpath, n, &lstArg, NULL, FALSE);
+    if(result)
+      return result;
   }
 
   cmd = aprintf( "%s%s%s",
@@ -3327,12 +3324,10 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status,
   }
 
   /* get the "raw" path */
-  path = curl_easy_unescape(data, path_to_use, 0, NULL);
-  if(!path) {
+  result = Curl_urldecode(data, path_to_use, 0, &path, NULL, FALSE);
+  if(result) {
     /* out of memory, but we can limp along anyway (and should try to
      * since we may already be in the out of memory cleanup path) */
-    if(!result)
-      result = CURLE_OUT_OF_MEMORY;
     ftpc->ctl_valid = FALSE; /* mark control connection as bad */
     conn->bits.close = TRUE; /* mark for connection closure */
     ftpc->prevpath = NULL; /* no path remembering */
@@ -3643,7 +3638,7 @@ static CURLcode ftp_range(struct connectdata *conn)
     }
     else {
       /* X-Y */
-      data->req.maxdownload = (to-from)+1; /* include last byte */
+      data->req.maxdownload = (to - from) + 1; /* include last byte */
       data->state.resume_from = from;
       DEBUGF(infof(conn->data, "FTP RANGE from %" FORMAT_OFF_T
                    " getting %" FORMAT_OFF_T " bytes\n",
@@ -4332,20 +4327,22 @@ CURLcode ftp_parse_url_path(struct connectdata *conn)
     }
     slash_pos=strrchr(cur_pos, '/');
     if(slash_pos || !*cur_pos) {
+      CURLcode result;
       ftpc->dirs = calloc(1, sizeof(ftpc->dirs[0]));
       if(!ftpc->dirs)
         return CURLE_OUT_OF_MEMORY;
 
-      ftpc->dirs[0] = curl_easy_unescape(conn->data, slash_pos ? cur_pos : "/",
-                                         slash_pos ?
-                                         curlx_sztosi(slash_pos-cur_pos) : 1,
-                                         NULL);
-      if(!ftpc->dirs[0]) {
+      result = Curl_urldecode(conn->data, slash_pos ? cur_pos : "/",
+                              slash_pos ?
+                              curlx_sztosi(slash_pos-cur_pos) : 1,
+                              &ftpc->dirs[0], NULL,
+                              FALSE);
+      if(result) {
         freedirs(ftpc);
-        return CURLE_OUT_OF_MEMORY;
+        return result;
       }
       ftpc->dirdepth = 1; /* we consider it to be a single dir */
-      filename = slash_pos ? slash_pos+1 : cur_pos; /* rest is file name */
+      filename = slash_pos ? slash_pos + 1 : cur_pos; /* rest is file name */
     }
     else
       filename = cur_pos;  /* this is a file name only */
@@ -4377,18 +4374,15 @@ CURLcode ftp_parse_url_path(struct connectdata *conn)
           /* we skip empty path components, like "x//y" since the FTP command
              CWD requires a parameter and a non-existent parameter a) doesn't
              work on many servers and b) has no effect on the others. */
-          int len = curlx_sztosi(slash_pos - cur_pos + absolute_dir);
-          ftpc->dirs[ftpc->dirdepth] =
-            curl_easy_unescape(conn->data, cur_pos - absolute_dir, len, NULL);
-          if(!ftpc->dirs[ftpc->dirdepth]) { /* run out of memory ... */
-            failf(data, "no memory");
-            freedirs(ftpc);
-            return CURLE_OUT_OF_MEMORY;
-          }
-          if(isBadFtpString(ftpc->dirs[ftpc->dirdepth])) {
+          size_t len = slash_pos - cur_pos + absolute_dir;
+          CURLcode result =
+            Curl_urldecode(conn->data, cur_pos - absolute_dir, len,
+                           &ftpc->dirs[ftpc->dirdepth], NULL,
+                           TRUE);
+          if(result) {
             free(ftpc->dirs[ftpc->dirdepth]);
             freedirs(ftpc);
-            return CURLE_URL_MALFORMAT;
+            return result;
           }
         }
         else {
@@ -4415,15 +4409,12 @@ CURLcode ftp_parse_url_path(struct connectdata *conn)
   } /* switch */
 
   if(filename && *filename) {
-    ftpc->file = curl_easy_unescape(conn->data, filename, 0, NULL);
-    if(NULL == ftpc->file) {
-      freedirs(ftpc);
-      failf(data, "no memory");
-      return CURLE_OUT_OF_MEMORY;
-    }
-    if(isBadFtpString(ftpc->file)) {
+    CURLcode result =
+      Curl_urldecode(conn->data, filename, 0,  &ftpc->file, NULL, TRUE);
+
+    if(result) {
       freedirs(ftpc);
-      return CURLE_URL_MALFORMAT;
+      return result;
     }
   }
   else
@@ -4441,15 +4432,17 @@ CURLcode ftp_parse_url_path(struct connectdata *conn)
   if(ftpc->prevpath) {
     /* prevpath is "raw" so we convert the input path before we compare the
        strings */
-    int dlen;
-    char *path = curl_easy_unescape(conn->data, data->state.path, 0, &dlen);
-    if(!path) {
+    size_t dlen;
+    char *path;
+    CURLcode result =
+      Curl_urldecode(conn->data, data->state.path, 0, &path, &dlen, FALSE);
+    if(result) {
       freedirs(ftpc);
-      return CURLE_OUT_OF_MEMORY;
+      return result;
     }
 
-    dlen -= ftpc->file?curlx_uztosi(strlen(ftpc->file)):0;
-    if((dlen == curlx_uztosi(strlen(ftpc->prevpath))) &&
+    dlen -= ftpc->file?strlen(ftpc->file):0;
+    if((dlen == strlen(ftpc->prevpath)) &&
        strnequal(path, ftpc->prevpath, dlen)) {
       infof(data, "Request has same path as previous transfer\n");
       ftpc->cwddone = TRUE;
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 56cb286..e7955ee 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -28,7 +28,8 @@ test200 test201 test202 test203 test204 test205 test206 test207 test208	\
 test209 test210 test211 test212 test213 test214 test215 test216 test217	\
 test218 test220 test221 test222 test223 test224 test225 test226 test227	\
 test228 test229 test231 test233 test234 test235 test236 test237 test238	\
-test239 test240 test241 test242 test243 test245 test246 test247 test248	\
+test239 test240 test241 test242 test243	\
+test244 test245 test246 test247 test248	\
 test249 test250 test251 test252 test253 test254 test255 test256 test257	\
 test258 test259 test260 test261 test262 test263 test264 test265 test266	\
 test267 test268 test269 test270 test271 test272 test273 test274 test275	\
diff --git a/tests/data/test244 b/tests/data/test244
new file mode 100644
index 0000000..8ce4b63
--- /dev/null
+++ b/tests/data/test244
@@ -0,0 +1,54 @@
+<testcase>
+<info>
+<keywords>
+FTP
+PASV
+CWD
+--ftp-method
+nocwd
+</keywords>
+</info>
+#
+# Server-side
+<reply>
+<data mode="text">
+total 20
+drwxr-xr-x   8 98       98           512 Oct 22 13:06 .
+drwxr-xr-x   8 98       98           512 Oct 22 13:06 ..
+drwxr-xr-x   2 98       98           512 May  2  1996 .NeXT
+-r--r--r--   1 0        1             35 Jul 16  1996 README
+lrwxrwxrwx   1 0        1              7 Dec  9  1999 bin -> usr/bin
+dr-xr-xr-x   2 0        1            512 Oct  1  1997 dev
+drwxrwxrwx   2 98       98           512 May 29 16:04 download.html
+dr-xr-xr-x   2 0        1            512 Nov 30  1995 etc
+drwxrwxrwx   2 98       1            512 Oct 30 14:33 pub
+dr-xr-xr-x   5 0        1            512 Oct  1  1997 usr
+</data>
+</reply>
+
+# Client-side
+<client>
+<server>
+ftp
+</server>
+ <name>
+FTP dir listing with nocwd and URL encoded path
+ </name>
+ <command>
+--ftp-method nocwd ftp://%HOSTIP:%FTPPORT/fir%23t/th%69rd/244/
+</command>
+</client>
+
+# Verify data after the test has been "shot"
+<verify>
+<protocol>
+USER anonymous
+PASS ftp@example.com
+PWD
+EPSV
+TYPE A
+LIST fir#t/third/244/
+QUIT
+</protocol>
+</verify>
+</testcase>
-- 
2.14.3


From 295fc8b0dc5c94a1cbf6688bfba768128b13cde6 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Wed, 2 Nov 2016 07:22:27 +0100
Subject: [PATCH 2/3] ftp_done: don't clobber the passed in error code

Coverity CID 1374359 pointed out the unused result value.

Upstream-commit: f81a8364618caf99b4691ffd494a9b2d4c9fb1f6
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 lib/ftp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/ftp.c b/lib/ftp.c
index 9da5a24..0259a14 100644
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -3323,11 +3323,12 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status,
     ftpc->known_filesize = -1;
   }
 
-  /* get the "raw" path */
-  result = Curl_urldecode(data, path_to_use, 0, &path, NULL, FALSE);
+  if(!result)
+    /* get the "raw" path */
+    result = Curl_urldecode(data, path_to_use, 0, &path, NULL, FALSE);
   if(result) {
-    /* out of memory, but we can limp along anyway (and should try to
-     * since we may already be in the out of memory cleanup path) */
+    /* We can limp along anyway (and should try to since we may already be in
+     * the error path) */
     ftpc->ctl_valid = FALSE; /* mark control connection as bad */
     conn->bits.close = TRUE; /* mark for connection closure */
     ftpc->prevpath = NULL; /* no path remembering */
-- 
2.14.4


From 9534442aae1da4e6cf2ce815e47dbcd82695c3d4 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Wed, 31 Jan 2018 08:40:11 +0100
Subject: [PATCH 3/3] FTP: reject path components with control codes

Refuse to operate when given path components featuring byte values lower
than 32.

Previously, inserting a %00 sequence early in the directory part when
using the 'singlecwd' ftp method could make curl write a zero byte
outside of the allocated buffer.

Test case 340 verifies.

CVE-2018-1000120
Reported-by: Duy Phan Thanh
Bug: https://curl.haxx.se/docs/adv_2018-9cd6.html

Upstream-commit: 535432c0adb62fe167ec09621500470b6fa4eb0f
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 lib/ftp.c              |  8 ++++----
 tests/data/Makefile.am |  1 +
 tests/data/test340     | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 4 deletions(-)
 create mode 100644 tests/data/test340

diff --git a/lib/ftp.c b/lib/ftp.c
index fb3a716..268efdd 100644
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -1482,7 +1482,7 @@ static CURLcode ftp_state_post_listtype(struct connectdata *conn)
       slashPos = strrchr(inpath, '/');
       n = slashPos - inpath;
     }
-    result = Curl_urldecode(data, inpath, n, &lstArg, NULL, FALSE);
+    result = Curl_urldecode(data, inpath, n, &lstArg, NULL, TRUE);
     if(result)
       return result;
   }
@@ -3325,7 +3325,7 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status,
 
   if(!result)
     /* get the "raw" path */
-    result = Curl_urldecode(data, path_to_use, 0, &path, NULL, FALSE);
+    result = Curl_urldecode(data, path_to_use, 0, &path, NULL, TRUE);
   if(result) {
     /* We can limp along anyway (and should try to since we may already be in
      * the error path) */
@@ -4337,7 +4337,7 @@ CURLcode ftp_parse_url_path(struct connectdata *conn)
                               slash_pos ?
                               curlx_sztosi(slash_pos-cur_pos) : 1,
                               &ftpc->dirs[0], NULL,
-                              FALSE);
+                              TRUE);
       if(result) {
         freedirs(ftpc);
         return result;
@@ -4436,7 +4436,7 @@ CURLcode ftp_parse_url_path(struct connectdata *conn)
     size_t dlen;
     char *path;
     CURLcode result =
-      Curl_urldecode(conn->data, data->state.path, 0, &path, &dlen, FALSE);
+      Curl_urldecode(conn->data, data->state.path, 0, &path, &dlen, TRUE);
     if(result) {
       freedirs(ftpc);
       return result;
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index e7955ee..910db5b 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -39,6 +39,7 @@ test294 test295 test296 test297 test298 test299 test300 test301 test302	\
 test303 test304 test305 test306 test307 test308 test309 test310 test311	\
 test312 test313 test317 test318 \
 test320 test321 test322 test323 test324 test350 test351	\
+test340	\
 test352 test353 test354 test400 test401 test402 test403 test404 test405	\
 test406 test407 test408 test409 test500 test501 test502 test503 test504	\
 test505 test506 test507 test508 test510 test511 test512 test513 test514	\
diff --git a/tests/data/test340 b/tests/data/test340
new file mode 100644
index 0000000..d834d76
--- /dev/null
+++ b/tests/data/test340
@@ -0,0 +1,40 @@
+<testcase>
+<info>
+<keywords>
+FTP
+PASV
+CWD
+--ftp-method
+singlecwd
+</keywords>
+</info>
+#
+# Server-side
+<reply>
+</reply>
+
+# Client-side
+<client>
+<server>
+ftp
+</server>
+ <name>
+FTP using %00 in path with singlecwd
+ </name>
+ <command>
+--ftp-method singlecwd ftp://%HOSTIP:%FTPPORT/%00first/second/third/340
+</command>
+</client>
+
+# Verify data after the test has been "shot"
+<verify>
+<protocol>
+USER anonymous
+PASS ftp@example.com
+PWD
+</protocol>
+<errorcode>
+3
+</errorcode>
+</verify>
+</testcase>
-- 
2.14.3