From 76f91d8b2da6501558cd92620c9656c283db8adf Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Tue, 4 Oct 2016 18:56:45 +0200 Subject: [PATCH 1/2] unescape: avoid integer overflow CVE-2016-8622 Bug: https://curl.haxx.se/docs/adv_20161102H.html Reported-by: Cure53 Upstream-commit: 53e71e47d6b81650d26ec33a58d0dca24c7ffb2c Signed-off-by: Kamil Dudka --- docs/libcurl/curl_easy_unescape.3 | 7 +++++-- lib/dict.c | 10 +++++----- lib/escape.c | 10 ++++++++-- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/docs/libcurl/curl_easy_unescape.3 b/docs/libcurl/curl_easy_unescape.3 index 06fd6fc..50ce97d 100644 --- a/docs/libcurl/curl_easy_unescape.3 +++ b/docs/libcurl/curl_easy_unescape.3 @@ -5,7 +5,7 @@ .\" * | (__| |_| | _ <| |___ .\" * \___|\___/|_| \_\_____| .\" * -.\" * Copyright (C) 1998 - 2015, Daniel Stenberg, , et al. +.\" * Copyright (C) 1998 - 2016, Daniel Stenberg, , et al. .\" * .\" * This software is licensed as described in the file COPYING, which .\" * you should have received as part of this distribution. The terms @@ -40,7 +40,10 @@ will use strlen() on the input \fIurl\fP string to find out the size. If \fBoutlength\fP is non-NULL, the function will write the length of the returned string in the integer it points to. This allows an escaped string -containing %00 to still get used properly after unescaping. +containing %00 to still get used properly after unescaping. Since this is a +pointer to an \fIint\fP type, it can only return a value up to INT_MAX so no +longer string can be unescaped if the string length is returned in this +parameter. You must \fIcurl_free(3)\fP the returned string when you're done with it. .SH AVAILABILITY diff --git a/lib/dict.c b/lib/dict.c index 2e7cb47..e1475ca 100644 --- a/lib/dict.c +++ b/lib/dict.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2015, Daniel Stenberg, , et al. + * Copyright (C) 1998 - 2016, Daniel Stenberg, , et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -52,7 +52,7 @@ #include #include "transfer.h" #include "sendf.h" - +#include "escape.h" #include "progress.h" #include "strequal.h" #include "dict.h" @@ -96,12 +96,12 @@ static char *unescape_word(struct SessionHandle *data, const char *inputbuff) char *newp; char *dictp; char *ptr; - int len; + size_t len; char ch; int olen=0; - newp = curl_easy_unescape(data, inputbuff, 0, &len); - if(!newp) + CURLcode result = Curl_urldecode(data, inputbuff, 0, &newp, &len, FALSE); + if(!newp || result) return NULL; dictp = malloc(((size_t)len)*2 + 1); /* add one for terminating zero */ diff --git a/lib/escape.c b/lib/escape.c index 808ac6c..4091097 100644 --- a/lib/escape.c +++ b/lib/escape.c @@ -224,8 +224,14 @@ char *curl_easy_unescape(CURL *handle, const char *string, int length, FALSE); if(res) return NULL; - if(olen) - *olen = curlx_uztosi(outputlen); + + if(olen) { + if(outputlen <= (size_t) INT_MAX) + *olen = curlx_uztosi(outputlen); + else + /* too large to return in an int, fail! */ + Curl_safefree(str); + } } return str; } -- 2.7.4 From e24b3f19b11fa4bf01ae4bce34d6aa96b27ec6ab Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Sat, 8 Oct 2016 11:21:38 +0200 Subject: [PATCH 2/2] escape: avoid using curl_easy_unescape() internally Since the internal Curl_urldecode() function has a better API. Upstream-commit: 46133aa536f7f5bf552b83369e3851b6f811299e Signed-off-by: Kamil Dudka --- lib/file.c | 9 +++++---- lib/ftp.c | 60 +++++++++++++++++++++++++++++------------------------------- lib/gopher.c | 5 +++-- lib/ldap.c | 26 +++++++++++++++----------- lib/ssh.c | 12 ++++++------ lib/tftp.c | 9 +++++---- lib/url.c | 55 +++++++++++++++++++++++++++---------------------------- 7 files changed, 90 insertions(+), 86 deletions(-) diff --git a/lib/file.c b/lib/file.c index d47bda1..1afdb7b 100644 --- a/lib/file.c +++ b/lib/file.c @@ -194,11 +194,12 @@ static CURLcode file_connect(struct connectdata *conn, bool *done) int i; char *actual_path; #endif - int real_path_len; + size_t real_path_len; - real_path = curl_easy_unescape(data, data->state.path, 0, &real_path_len); - if(!real_path) - return CURLE_OUT_OF_MEMORY; + CURLcode result = Curl_urldecode(data, data->state.path, 0, &real_path, + &real_path_len, FALSE); + if(result) + return result; #ifdef DOS_FILESYSTEM /* If the first character is a slash, and there's diff --git a/lib/ftp.c b/lib/ftp.c index b5a7f55..d7e3dbb 100644 --- a/lib/ftp.c +++ b/lib/ftp.c @@ -3298,8 +3298,8 @@ 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) @@ -4291,6 +4291,7 @@ CURLcode ftp_parse_url_path(struct connectdata *conn) slash_pos=strrchr(cur_pos, '/'); if(slash_pos || !*cur_pos) { size_t dirlen = slash_pos-cur_pos; + CURLcode result; ftpc->dirs = calloc(1, sizeof(ftpc->dirs[0])); if(!ftpc->dirs) @@ -4299,12 +4300,13 @@ CURLcode ftp_parse_url_path(struct connectdata *conn) if(!dirlen) dirlen++; - ftpc->dirs[0] = curl_easy_unescape(conn->data, slash_pos ? cur_pos : "/", - slash_pos ? curlx_uztosi(dirlen) : 1, - NULL); - if(!ftpc->dirs[0]) { + result = Curl_urldecode(conn->data, slash_pos ? cur_pos : "/", + slash_pos ? dirlen : 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 */ @@ -4339,18 +4341,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 { @@ -4386,15 +4385,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 @@ -4412,15 +4408,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/lib/gopher.c b/lib/gopher.c index 19f2f5a..03c79f7 100644 --- a/lib/gopher.c +++ b/lib/gopher.c @@ -35,6 +35,7 @@ #include "rawstr.h" #include "select.h" #include "url.h" +#include "escape.h" #include "warnless.h" #include "curl_memory.h" /* The last #include file should be: */ @@ -83,7 +84,7 @@ static CURLcode gopher_do(struct connectdata *conn, bool *done) char *sel; char *sel_org = NULL; ssize_t amount, k; - int len; + size_t len; *done = TRUE; /* unconditionally */ @@ -107,7 +108,7 @@ static CURLcode gopher_do(struct connectdata *conn, bool *done) newp[i] = '\x09'; /* ... and finally unescape */ - sel = curl_easy_unescape(data, newp, 0, &len); + result = Curl_urldecode(data, newp, 0, &sel, &len, FALSE); if(!sel) return CURLE_OUT_OF_MEMORY; sel_org = sel; diff --git a/lib/ldap.c b/lib/ldap.c index 4f4c707..57061bb 100644 --- a/lib/ldap.c +++ b/lib/ldap.c @@ -768,7 +768,7 @@ static bool split_str(char *str, char ***out, size_t *count) * * Defined in RFC4516 section 2. */ -static int _ldap_url_parse2 (const struct connectdata *conn, LDAPURLDesc *ludp) +static int _ldap_url_parse2(const struct connectdata *conn, LDAPURLDesc *ludp) { int rc = LDAP_SUCCESS; char *path; @@ -799,12 +799,13 @@ static int _ldap_url_parse2 (const struct connectdata *conn, LDAPURLDesc *ludp) if(*p) { char *dn = p; char *unescaped; + CURLcode result; LDAP_TRACE (("DN '%s'\n", dn)); /* Unescape the DN */ - unescaped = curl_easy_unescape(conn->data, dn, 0, NULL); - if(!unescaped) { + result = Curl_urldecode(conn->data, dn, 0, &unescaped, NULL, FALSE); + if(result) { rc = LDAP_NO_MEMORY; goto quit; @@ -863,12 +864,14 @@ static int _ldap_url_parse2 (const struct connectdata *conn, LDAPURLDesc *ludp) for(i = 0; i < count; i++) { char *unescaped; + CURLcode result; LDAP_TRACE (("attr[%d] '%s'\n", i, attributes[i])); /* Unescape the attribute */ - unescaped = curl_easy_unescape(conn->data, attributes[i], 0, NULL); - if(!unescaped) { + result = Curl_urldecode(conn->data, attributes[i], 0, &unescaped, NULL, + FALSE); + if(result) { free(attributes); rc = LDAP_NO_MEMORY; @@ -931,12 +934,13 @@ static int _ldap_url_parse2 (const struct connectdata *conn, LDAPURLDesc *ludp) if(*p) { char *filter = p; char *unescaped; + CURLcode result; LDAP_TRACE (("filter '%s'\n", filter)); /* Unescape the filter */ - unescaped = curl_easy_unescape(conn->data, filter, 0, NULL); - if(!unescaped) { + result = Curl_urldecode(conn->data, filter, 0, &unescaped, NULL, FALSE); + if(result) { rc = LDAP_NO_MEMORY; goto quit; @@ -972,8 +976,8 @@ quit: return rc; } -static int _ldap_url_parse (const struct connectdata *conn, - LDAPURLDesc **ludpp) +static int _ldap_url_parse(const struct connectdata *conn, + LDAPURLDesc **ludpp) { LDAPURLDesc *ludp = calloc(1, sizeof(*ludp)); int rc; @@ -982,7 +986,7 @@ static int _ldap_url_parse (const struct connectdata *conn, if(!ludp) return LDAP_NO_MEMORY; - rc = _ldap_url_parse2 (conn, ludp); + rc = _ldap_url_parse2(conn, ludp); if(rc != LDAP_SUCCESS) { _ldap_free_urldesc(ludp); ludp = NULL; @@ -991,7 +995,7 @@ static int _ldap_url_parse (const struct connectdata *conn, return (rc); } -static void _ldap_free_urldesc (LDAPURLDesc *ludp) +static void _ldap_free_urldesc(LDAPURLDesc *ludp) { size_t i; diff --git a/lib/ssh.c b/lib/ssh.c index 10bf546..2d9b6a2 100644 --- a/lib/ssh.c +++ b/lib/ssh.c @@ -407,12 +407,12 @@ static CURLcode ssh_getworkingpath(struct connectdata *conn, struct SessionHandle *data = conn->data; char *real_path = NULL; char *working_path; - int working_path_len; - - working_path = curl_easy_unescape(data, data->state.path, 0, - &working_path_len); - if(!working_path) - return CURLE_OUT_OF_MEMORY; + size_t working_path_len; + CURLcode result = + Curl_urldecode(data, data->state.path, 0, &working_path, + &working_path_len, FALSE); + if(result) + return result; /* Check for /~/ , indicating relative to the user's home directory */ if(conn->handler->protocol & CURLPROTO_SCP) { diff --git a/lib/tftp.c b/lib/tftp.c index 040d967..274a724 100644 --- a/lib/tftp.c +++ b/lib/tftp.c @@ -59,6 +59,7 @@ #include "speedcheck.h" #include "curl_printf.h" #include "select.h" +#include "escape.h" /* The last #include files should be: */ #include "curl_memory.h" @@ -484,10 +485,10 @@ static CURLcode tftp_send_first(tftp_state_data_t *state, tftp_event_t event) /* As RFC3617 describes the separator slash is not actually part of the file name so we skip the always-present first letter of the path string. */ - filename = curl_easy_unescape(data, &state->conn->data->state.path[1], 0, - NULL); - if(!filename) - return CURLE_OUT_OF_MEMORY; + result = Curl_urldecode(data, &state->conn->data->state.path[1], 0, + &filename, NULL, FALSE); + if(result) + return result; snprintf((char *)state->spacket.data+2, state->blksize, diff --git a/lib/url.c b/lib/url.c index ff14dad..a88903c 100644 --- a/lib/url.c +++ b/lib/url.c @@ -4652,21 +4652,24 @@ static CURLcode parse_proxy(struct SessionHandle *data, them. */ Curl_safefree(conn->proxyuser); if(proxyuser && strlen(proxyuser) < MAX_CURL_USER_LENGTH) - conn->proxyuser = curl_easy_unescape(data, proxyuser, 0, NULL); - else + result = Curl_urldecode(data, proxyuser, 0, &conn->proxyuser, NULL, + FALSE); + else { conn->proxyuser = strdup(""); + if(!conn->proxyuser) + result = CURLE_OUT_OF_MEMORY; + } - if(!conn->proxyuser) - result = CURLE_OUT_OF_MEMORY; - else { + if(!result) { Curl_safefree(conn->proxypasswd); if(proxypasswd && strlen(proxypasswd) < MAX_CURL_PASSWORD_LENGTH) - conn->proxypasswd = curl_easy_unescape(data, proxypasswd, 0, NULL); - else + result = Curl_urldecode(data, proxypasswd, 0, + &conn->proxypasswd, NULL, FALSE); + else { conn->proxypasswd = strdup(""); - - if(!conn->proxypasswd) - result = CURLE_OUT_OF_MEMORY; + if(!conn->proxypasswd) + result = CURLE_OUT_OF_MEMORY; + } } if(!result) { @@ -4773,6 +4776,7 @@ static CURLcode parse_proxy_auth(struct SessionHandle *data, { char proxyuser[MAX_CURL_USER_LENGTH]=""; char proxypasswd[MAX_CURL_PASSWORD_LENGTH]=""; + CURLcode result; if(data->set.str[STRING_PROXYUSERNAME] != NULL) { strncpy(proxyuser, data->set.str[STRING_PROXYUSERNAME], @@ -4785,15 +4789,11 @@ static CURLcode parse_proxy_auth(struct SessionHandle *data, proxypasswd[MAX_CURL_PASSWORD_LENGTH-1] = '\0'; /*To be on safe side*/ } - conn->proxyuser = curl_easy_unescape(data, proxyuser, 0, NULL); - if(!conn->proxyuser) - return CURLE_OUT_OF_MEMORY; - - conn->proxypasswd = curl_easy_unescape(data, proxypasswd, 0, NULL); - if(!conn->proxypasswd) - return CURLE_OUT_OF_MEMORY; - - return CURLE_OK; + result = Curl_urldecode(data, proxyuser, 0, &conn->proxyuser, NULL, FALSE); + if(!result) + result = Curl_urldecode(data, proxypasswd, 0, &conn->proxypasswd, NULL, + FALSE); + return result; } #endif /* CURL_DISABLE_PROXY */ @@ -4867,9 +4867,8 @@ static CURLcode parse_url_login(struct SessionHandle *data, conn->bits.user_passwd = TRUE; /* enable user+password */ /* Decode the user */ - newname = curl_easy_unescape(data, userp, 0, NULL); - if(!newname) { - result = CURLE_OUT_OF_MEMORY; + result = Curl_urldecode(data, userp, 0, &newname, NULL, FALSE); + if(result) { goto out; } @@ -4879,9 +4878,9 @@ static CURLcode parse_url_login(struct SessionHandle *data, if(passwdp) { /* We have a password in the URL so decode it */ - char *newpasswd = curl_easy_unescape(data, passwdp, 0, NULL); - if(!newpasswd) { - result = CURLE_OUT_OF_MEMORY; + char *newpasswd; + result = Curl_urldecode(data, passwdp, 0, &newpasswd, NULL, FALSE); + if(result) { goto out; } @@ -4891,9 +4890,9 @@ static CURLcode parse_url_login(struct SessionHandle *data, if(optionsp) { /* We have an options list in the URL so decode it */ - char *newoptions = curl_easy_unescape(data, optionsp, 0, NULL); - if(!newoptions) { - result = CURLE_OUT_OF_MEMORY; + char *newoptions; + result = Curl_urldecode(data, optionsp, 0, &newoptions, NULL, FALSE); + if(result) { goto out; } -- 2.7.4