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",

@@ -3328,12 +3325,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 */

@@ -3644,7 +3639,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",

@@ -4333,20 +4328,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 */

@@ -4378,18 +4375,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 {

@@ -4416,15 +4410,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

@@ -4442,15 +4433,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

@@ -3324,11 +3324,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;

   }

@@ -3326,7 +3326,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) */

@@ -4338,7 +4338,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;

@@ -4437,7 +4437,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