From 5452fdc5ae93f3571074c591fdf28cdf630796a0 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg 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 --- 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 @@ + + + +FTP +PASV +CWD +--ftp-method +nocwd + + +# +# Server-side + + +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 + + + +# Client-side + + +ftp + + +FTP dir listing with nocwd and URL encoded path + + +--ftp-method nocwd ftp://%HOSTIP:%FTPPORT/fir%23t/th%69rd/244/ + + + +# Verify data after the test has been "shot" + + +USER anonymous +PASS ftp@example.com +PWD +EPSV +TYPE A +LIST fir#t/third/244/ +QUIT + + + -- 2.14.3 From 295fc8b0dc5c94a1cbf6688bfba768128b13cde6 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg 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 --- 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 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 --- 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 @@ + + + +FTP +PASV +CWD +--ftp-method +singlecwd + + +# +# Server-side + + + +# Client-side + + +ftp + + +FTP using %00 in path with singlecwd + + +--ftp-method singlecwd ftp://%HOSTIP:%FTPPORT/%00first/second/third/340 + + + +# Verify data after the test has been "shot" + + +USER anonymous +PASS ftp@example.com +PWD + + +3 + + + -- 2.14.3