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