From ff50fe0348466cae1a9f9f759b362c03f7060c34 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Mon, 14 Aug 2017 23:33:23 +0200 Subject: [PATCH] strtoofft: reduce integer overflow risks globally ... make sure we bail out on overflows. Reported-by: Brian Carpenter Closes #1758 --- lib/cookie.c | 23 +++++++++++------ lib/file.c | 24 +++++++++-------- lib/ftp.c | 33 +++++++++++++----------- lib/ftplistparser.c | 36 ++++++++++---------------- lib/http.c | 56 +++++++++++++++++++++------------------- lib/http_chunks.c | 6 ++--- lib/http_proxy.c | 4 +-- lib/imap.c | 9 ++++--- lib/ssh.c | 17 ++++++++---- lib/strtoofft.c | 63 ++++++++++++++++++++++++++++++++++++++++++--- lib/strtoofft.h | 35 +++++++------------------ src/tool_getparam.c | 11 ++++++-- src/tool_paramhlp.c | 10 ++++--- 13 files changed, 196 insertions(+), 131 deletions(-) diff --git a/lib/cookie.c b/lib/cookie.c index 6b678aeb8..eba2536ef 100644 --- a/lib/cookie.c +++ b/lib/cookie.c @@ -593,14 +593,20 @@ Curl_cookie_add(struct Curl_easy *data, } while(semiptr); if(co->maxage) { - co->expires = - curlx_strtoofft((*co->maxage=='\"')? - &co->maxage[1]:&co->maxage[0], NULL, 10); - if(CURL_OFF_T_MAX - now < co->expires) - /* avoid overflow */ + CURLofft offt; + offt = curlx_strtoofft((*co->maxage=='\"')? + &co->maxage[1]:&co->maxage[0], NULL, 10, + &co->expires); + if(offt == CURL_OFFT_FLOW) + /* overflow, used max value */ co->expires = CURL_OFF_T_MAX; - else - co->expires += now; + else if(!offt) { + if(CURL_OFF_T_MAX - now < co->expires) + /* would overflow */ + co->expires = CURL_OFF_T_MAX; + else + co->expires += now; + } } else if(co->expirestr) { /* Note that if the date couldn't get parsed for whatever reason, @@ -753,7 +759,8 @@ Curl_cookie_add(struct Curl_easy *data, co->secure = strcasecompare(ptr, "TRUE")?TRUE:FALSE; break; case 4: - co->expires = curlx_strtoofft(ptr, NULL, 10); + if(curlx_strtoofft(ptr, NULL, 10, &co->expires)) + badcookie = TRUE; break; case 5: co->name = strdup(ptr); diff --git a/lib/file.c b/lib/file.c index 666cbe75b..a8f29f2ca 100644 --- a/lib/file.c +++ b/lib/file.c @@ -139,26 +139,28 @@ static CURLcode file_range(struct connectdata *conn) struct Curl_easy *data = conn->data; if(data->state.use_range && data->state.range) { - from=curlx_strtoofft(data->state.range, &ptr, 0); + CURLofft from_t; + CURLofft to_t; + from_t = curlx_strtoofft(data->state.range, &ptr, 0, &from); + if(from_t == CURL_OFFT_FLOW) + return CURLE_RANGE_ERROR; while(*ptr && (ISSPACE(*ptr) || (*ptr=='-'))) ptr++; - to=curlx_strtoofft(ptr, &ptr2, 0); - if(ptr == ptr2) { - /* we didn't get any digit */ - to=-1; - } - if((-1 == to) && (from>=0)) { + to_t = curlx_strtoofft(ptr, &ptr2, 0, &to); + if(to_t == CURL_OFFT_FLOW) + return CURLE_RANGE_ERROR; + if((to_t == CURL_OFFT_INVAL) && !from_t) { /* X - */ data->state.resume_from = from; DEBUGF(infof(data, "RANGE %" CURL_FORMAT_CURL_OFF_T " to end of file\n", from)); } - else if(from < 0) { + else if((from_t == CURL_OFFT_INVAL) && !to_t) { /* -Y */ - data->req.maxdownload = -from; - data->state.resume_from = from; + data->req.maxdownload = to; + data->state.resume_from = -to; DEBUGF(infof(data, "RANGE the last %" CURL_FORMAT_CURL_OFF_T " bytes\n", - -from)); + to)); } else { /* X-Y */ diff --git a/lib/ftp.c b/lib/ftp.c index 6e86e5386..481b14a88 100644 --- a/lib/ftp.c +++ b/lib/ftp.c @@ -2260,11 +2260,13 @@ static CURLcode ftp_state_size_resp(struct connectdata *conn, { CURLcode result = CURLE_OK; struct Curl_easy *data=conn->data; - curl_off_t filesize; + curl_off_t filesize = -1; char *buf = data->state.buffer; /* get the size from the ascii string: */ - filesize = (ftpcode == 213)?curlx_strtoofft(buf+4, NULL, 0):-1; + if(ftpcode == 213) + /* ignores parsing errors, which will make the size remain unknown */ + (void)curlx_strtoofft(buf+4, NULL, 0, &filesize); if(instate == FTP_SIZE) { #ifdef CURL_FTP_HTTPSTYLE_HEAD @@ -2435,7 +2437,7 @@ static CURLcode ftp_state_get_resp(struct connectdata *conn, /* if we have nothing but digits: */ if(bytes++) { /* get the number! */ - size = curlx_strtoofft(bytes, NULL, 0); + (void)curlx_strtoofft(bytes, NULL, 0, &size); } } } @@ -3466,31 +3468,32 @@ static CURLcode ftp_range(struct connectdata *conn) { curl_off_t from, to; char *ptr; - char *ptr2; struct Curl_easy *data = conn->data; struct ftp_conn *ftpc = &conn->proto.ftpc; if(data->state.use_range && data->state.range) { - from=curlx_strtoofft(data->state.range, &ptr, 0); + CURLofft from_t; + CURLofft to_t; + from_t = curlx_strtoofft(data->state.range, &ptr, 0, &from); + if(from_t == CURL_OFFT_FLOW) + return CURLE_RANGE_ERROR; while(*ptr && (ISSPACE(*ptr) || (*ptr=='-'))) ptr++; - to=curlx_strtoofft(ptr, &ptr2, 0); - if(ptr == ptr2) { - /* we didn't get any digit */ - to=-1; - } - if((-1 == to) && (from>=0)) { + to_t = curlx_strtoofft(ptr, NULL, 0, &to); + if(to_t == CURL_OFFT_FLOW) + return CURLE_RANGE_ERROR; + if((to_t == CURL_OFFT_INVAL) && !from_t) { /* X - */ data->state.resume_from = from; DEBUGF(infof(conn->data, "FTP RANGE %" CURL_FORMAT_CURL_OFF_T " to end of file\n", from)); } - else if(from < 0) { + else if(!to_t && (from_t == CURL_OFFT_INVAL)) { /* -Y */ - data->req.maxdownload = -from; - data->state.resume_from = from; + data->req.maxdownload = to; + data->state.resume_from = -to; DEBUGF(infof(conn->data, "FTP RANGE the last %" CURL_FORMAT_CURL_OFF_T - " bytes\n", -from)); + " bytes\n", to)); } else { /* X-Y */ diff --git a/lib/ftplistparser.c b/lib/ftplistparser.c index 2acce31d8..47fe1733b 100644 --- a/lib/ftplistparser.c +++ b/lib/ftplistparser.c @@ -609,16 +609,18 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb, char *p; curl_off_t fsize; finfo->b_data[parser->item_offset + parser->item_length - 1] = 0; - fsize = curlx_strtoofft(finfo->b_data+parser->item_offset, &p, 10); - if(p[0] == '\0' && fsize != CURL_OFF_T_MAX && - fsize != CURL_OFF_T_MIN) { - parser->file_data->info.flags |= CURLFINFOFLAG_KNOWN_SIZE; - parser->file_data->info.size = fsize; + if(!curlx_strtoofft(finfo->b_data+parser->item_offset, + &p, 10, &fsize)) { + if(p[0] == '\0' && fsize != CURL_OFF_T_MAX && + fsize != CURL_OFF_T_MIN) { + parser->file_data->info.flags |= CURLFINFOFLAG_KNOWN_SIZE; + parser->file_data->info.size = fsize; + } + parser->item_length = 0; + parser->item_offset = 0; + parser->state.UNIX.main = PL_UNIX_TIME; + parser->state.UNIX.sub.time = PL_UNIX_TIME_PREPART1; } - parser->item_length = 0; - parser->item_offset = 0; - parser->state.UNIX.main = PL_UNIX_TIME; - parser->state.UNIX.sub.time = PL_UNIX_TIME_PREPART1; } else if(!ISDIGIT(c)) { PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST); @@ -935,19 +937,9 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb, } else { char *endptr; - finfo->size = curlx_strtoofft(finfo->b_data + - parser->item_offset, - &endptr, 10); - if(!*endptr) { - if(finfo->size == CURL_OFF_T_MAX || - finfo->size == CURL_OFF_T_MIN) { - if(errno == ERANGE) { - PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST); - return bufflen; - } - } - } - else { + if(curlx_strtoofft(finfo->b_data + + parser->item_offset, + &endptr, 10, &finfo->size)) { PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST); return bufflen; } diff --git a/lib/http.c b/lib/http.c index d66b8482f..35c7c3d43 100644 --- a/lib/http.c +++ b/lib/http.c @@ -3486,28 +3486,32 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data, /* Check for Content-Length: header lines to get size */ if(!k->ignorecl && !data->set.ignorecl && checkprefix("Content-Length:", k->p)) { - curl_off_t contentlength = curlx_strtoofft(k->p+15, NULL, 10); - if(data->set.max_filesize && - contentlength > data->set.max_filesize) { - failf(data, "Maximum file size exceeded"); - return CURLE_FILESIZE_EXCEEDED; - } - if(contentlength >= 0) { - k->size = contentlength; - k->maxdownload = k->size; - /* we set the progress download size already at this point - just to make it easier for apps/callbacks to extract this - info as soon as possible */ - Curl_pgrsSetDownloadSize(data, k->size); - } - else { - /* Negative Content-Length is really odd, and we know it - happens for example when older Apache servers send large - files */ - streamclose(conn, "negative content-length"); - infof(data, "Negative content-length: %" CURL_FORMAT_CURL_OFF_T - ", closing after transfer\n", contentlength); + curl_off_t contentlength; + if(!curlx_strtoofft(k->p+15, NULL, 10, &contentlength)) { + if(data->set.max_filesize && + contentlength > data->set.max_filesize) { + failf(data, "Maximum file size exceeded"); + return CURLE_FILESIZE_EXCEEDED; + } + if(contentlength >= 0) { + k->size = contentlength; + k->maxdownload = k->size; + /* we set the progress download size already at this point + just to make it easier for apps/callbacks to extract this + info as soon as possible */ + Curl_pgrsSetDownloadSize(data, k->size); + } + else { + /* Negative Content-Length is really odd, and we know it + happens for example when older Apache servers send large + files */ + streamclose(conn, "negative content-length"); + infof(data, "Negative content-length: %" CURL_FORMAT_CURL_OFF_T + ", closing after transfer\n", contentlength); + } } + else + infof(data, "Illegal Content-Length: header\n"); } /* check for Content-Type: header lines to get the MIME-type */ else if(checkprefix("Content-Type:", k->p)) { @@ -3682,11 +3686,11 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data, /* if it truly stopped on a digit */ if(ISDIGIT(*ptr)) { - k->offset = curlx_strtoofft(ptr, NULL, 10); - - if(data->state.resume_from == k->offset) - /* we asked for a resume and we got it */ - k->content_range = TRUE; + if(!curlx_strtoofft(ptr, NULL, 10, &k->offset)) { + if(data->state.resume_from == k->offset) + /* we asked for a resume and we got it */ + k->content_range = TRUE; + } } else data->state.resume_from = 0; /* get everything */ diff --git a/lib/http_chunks.c b/lib/http_chunks.c index 1bdf6974c..fce00c21e 100644 --- a/lib/http_chunks.c +++ b/lib/http_chunks.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2016, Daniel Stenberg, , et al. + * Copyright (C) 1998 - 2017, 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 @@ -158,9 +158,7 @@ CHUNKcode Curl_httpchunk_read(struct connectdata *conn, return CHUNKE_ILLEGAL_HEX; } - ch->datasize=curlx_strtoofft(ch->hexbuffer, &endptr, 16); - if((ch->datasize == CURL_OFF_T_MAX) && (errno == ERANGE)) - /* overflow is an error */ + if(curlx_strtoofft(ch->hexbuffer, &endptr, 16, &ch->datasize)) return CHUNKE_ILLEGAL_HEX; ch->state = CHUNK_LF; /* now wait for the CRLF */ } diff --git a/lib/http_proxy.c b/lib/http_proxy.c index 36567d36f..5c5f8fb2c 100644 --- a/lib/http_proxy.c +++ b/lib/http_proxy.c @@ -523,8 +523,8 @@ static CURLcode CONNECT(struct connectdata *conn, k->httpcode); } else { - s->cl = curlx_strtoofft(s->line_start + - strlen("Content-Length:"), NULL, 10); + (void)curlx_strtoofft(s->line_start + + strlen("Content-Length:"), NULL, 10, &s->cl); } } else if(Curl_compareheader(s->line_start, "Connection:", "close")) diff --git a/lib/imap.c b/lib/imap.c index 48af2902a..bc20110c2 100644 --- a/lib/imap.c +++ b/lib/imap.c @@ -1070,10 +1070,11 @@ static CURLcode imap_state_fetch_resp(struct connectdata *conn, int imapcode, if(*ptr == '{') { char *endptr; - size = curlx_strtoofft(ptr + 1, &endptr, 10); - if(endptr - ptr > 1 && endptr[0] == '}' && - endptr[1] == '\r' && endptr[2] == '\0') - parsed = TRUE; + if(!curlx_strtoofft(ptr + 1, &endptr, 10, &size)) { + if(endptr - ptr > 1 && endptr[0] == '}' && + endptr[1] == '\r' && endptr[2] == '\0') + parsed = TRUE; + } } if(parsed) { diff --git a/lib/ssh.c b/lib/ssh.c index 9443e24b9..cc5178fe7 100644 --- a/lib/ssh.c +++ b/lib/ssh.c @@ -2233,18 +2233,25 @@ static CURLcode ssh_statemach_act(struct connectdata *conn, bool *block) curl_off_t from, to; char *ptr; char *ptr2; + CURLofft to_t; + CURLofft from_t; - from=curlx_strtoofft(conn->data->state.range, &ptr, 0); + from_t = curlx_strtoofft(conn->data->state.range, &ptr, 0, &from); + if(from_t == CURL_OFFT_FLOW) + return CURLE_RANGE_ERROR; while(*ptr && (ISSPACE(*ptr) || (*ptr=='-'))) ptr++; - to=curlx_strtoofft(ptr, &ptr2, 0); - if((ptr == ptr2) /* no "to" value given */ + to_t = curlx_strtoofft(ptr, &ptr2, 0, &to); + if(to_t == CURL_OFFT_FLOW) + return CURLE_RANGE_ERROR; + if((to_t == CURL_OFFT_INVAL) /* no "to" value given */ || (to >= size)) { to = size - 1; } - if(from < 0) { + if(from_t) { /* from is relative to end of file */ - from += size; + from = size - to; + to = size - 1; } if(from > size) { failf(data, "Offset (%" diff --git a/lib/strtoofft.c b/lib/strtoofft.c index c2adc7280..f057bb1d6 100644 --- a/lib/strtoofft.c +++ b/lib/strtoofft.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2016, Daniel Stenberg, , et al. + * Copyright (C) 1998 - 2017, 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 @@ -20,6 +20,7 @@ * ***************************************************************************/ +#include #include "curl_setup.h" #include "strtoofft.h" @@ -32,6 +33,30 @@ * https://www.opengroup.org/onlinepubs/009695399/functions/strtoimax.html */ +#if (CURL_SIZEOF_CURL_OFF_T > CURL_SIZEOF_LONG) +# ifdef HAVE_STRTOLL +# define strtooff strtoll +# else +# if defined(_MSC_VER) && (_MSC_VER >= 1300) && (_INTEGRAL_MAX_BITS >= 64) +# if defined(_SAL_VERSION) + _Check_return_ _CRTIMP __int64 __cdecl _strtoi64( + _In_z_ const char *_String, + _Out_opt_ _Deref_post_z_ char **_EndPtr, _In_ int _Radix); +# else + _CRTIMP __int64 __cdecl _strtoi64(const char *_String, + char **_EndPtr, int _Radix); +# endif +# define strtooff _strtoi64 +# else + curl_off_t curlx_strtoll(const char *nptr, char **endptr, int base); +# define strtooff curlx_strtoll +# define NEED_CURL_STRTOLL 1 +# endif +# endif +#else +# define strtooff strtol +#endif + #ifdef NEED_CURL_STRTOLL /* Range tests can be used for alphanum decoding if characters are consecutive, @@ -48,11 +73,10 @@ static const char valchars[] = static int get_char(char c, int base); /** - * Emulated version of the strtoll function. This extracts a long long + * Custom version of the strtooff function. This extracts a curl_off_t * value from the given input string and returns it. */ -curl_off_t -curlx_strtoll(const char *nptr, char **endptr, int base) +static curl_off_t strtooff(const char *nptr, char **endptr, int base) { char *end; int is_negative = 0; @@ -186,3 +210,34 @@ static int get_char(char c, int base) return value; } #endif /* Only present if we need strtoll, but don't have it. */ + +/* + * Parse a *positive* up to 64 bit number written in ascii. + */ +CURLofft curlx_strtoofft(const char *str, char **endp, int base, + curl_off_t *num) +{ + char *end; + curl_off_t number; + errno = 0; + *num = 0; /* clear by default */ + while(str && *str && ISSPACE(*str)) + str++; + if('-' == *str) { + if(endp) + *endp = (char *)str; /* didn't actually move */ + return CURL_OFFT_INVAL; /* nothing parsed */ + } + number = strtooff(str, &end, base); + if(endp) + *endp = end; + if(errno == ERANGE) + /* overflow/underflow */ + return CURL_OFFT_FLOW; + else if(str == end) + /* nothing parsed */ + return CURL_OFFT_INVAL; + + *num = number; + return CURL_OFFT_OK; +} diff --git a/lib/strtoofft.h b/lib/strtoofft.h index f4039f3a3..ac1fa4aa4 100644 --- a/lib/strtoofft.h +++ b/lib/strtoofft.h @@ -7,7 +7,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2014, Daniel Stenberg, , et al. + * Copyright (C) 1998 - 2017, 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,30 +40,6 @@ * of 'long' the conversion function to use is strtol(). */ -#if (CURL_SIZEOF_CURL_OFF_T > CURL_SIZEOF_LONG) -# ifdef HAVE_STRTOLL -# define curlx_strtoofft strtoll -# else -# if defined(_MSC_VER) && (_MSC_VER >= 1300) && (_INTEGRAL_MAX_BITS >= 64) -# if defined(_SAL_VERSION) - _Check_return_ _CRTIMP __int64 __cdecl _strtoi64( - _In_z_ const char *_String, - _Out_opt_ _Deref_post_z_ char **_EndPtr, _In_ int _Radix); -# else - _CRTIMP __int64 __cdecl _strtoi64(const char *_String, - char **_EndPtr, int _Radix); -# endif -# define curlx_strtoofft _strtoi64 -# else - curl_off_t curlx_strtoll(const char *nptr, char **endptr, int base); -# define curlx_strtoofft curlx_strtoll -# define NEED_CURL_STRTOLL 1 -# endif -# endif -#else -# define curlx_strtoofft strtol -#endif - #if (CURL_SIZEOF_CURL_OFF_T == 4) # define CURL_OFF_T_MAX CURL_OFF_T_C(0x7FFFFFFF) #else @@ -72,4 +48,13 @@ #endif #define CURL_OFF_T_MIN (-CURL_OFF_T_MAX - CURL_OFF_T_C(1)) +typedef enum { + CURL_OFFT_OK, /* parsed fine */ + CURL_OFFT_FLOW, /* over or underflow */ + CURL_OFFT_INVAL /* nothing was parsed */ +} CURLofft; + +CURLofft curlx_strtoofft(const char *str, char **endp, int base, + curl_off_t *num); + #endif /* HEADER_CURL_STRTOOFFT_H */ diff --git a/src/tool_getparam.c b/src/tool_getparam.c index b7ee519b3..40b39a8aa 100644 --- a/src/tool_getparam.c +++ b/src/tool_getparam.c @@ -590,7 +590,11 @@ ParameterError getparameter(const char *flag, /* f or -long-flag */ { /* We support G, M, K too */ char *unit; - curl_off_t value = curlx_strtoofft(nextarg, &unit, 0); + curl_off_t value; + if(curlx_strtoofft(nextarg, &unit, 0, &value)) { + warnf(global, "unsupported rate\n"); + return PARAM_BAD_USE; + } if(!*unit) unit = (char *)"b"; @@ -1843,10 +1847,13 @@ ParameterError getparameter(const char *flag, /* f or -long-flag */ if(ISDIGIT(*nextarg) && !strchr(nextarg, '-')) { char buffer[32]; curl_off_t off; + if(curlx_strtoofft(nextarg, NULL, 10, &off)) { + warnf(global, "unsupported range point\n"); + return PARAM_BAD_USE; + } warnf(global, "A specified range MUST include at least one dash (-). " "Appending one for you!\n"); - off = curlx_strtoofft(nextarg, NULL, 10); snprintf(buffer, sizeof(buffer), "%" CURL_FORMAT_CURL_OFF_T "-", off); Curl_safefree(config->range); config->range = strdup(buffer); diff --git a/src/tool_paramhlp.c b/src/tool_paramhlp.c index 85c5e79a7..86a3fe6b0 100644 --- a/src/tool_paramhlp.c +++ b/src/tool_paramhlp.c @@ -400,9 +400,13 @@ ParameterError str2offset(curl_off_t *val, const char *str) return PARAM_NEGATIVE_NUMERIC; #if(CURL_SIZEOF_CURL_OFF_T > CURL_SIZEOF_LONG) - *val = curlx_strtoofft(str, &endptr, 0); - if((*val == CURL_OFF_T_MAX || *val == CURL_OFF_T_MIN) && (errno == ERANGE)) - return PARAM_NUMBER_TOO_LARGE; + { + CURLofft offt = curlx_strtoofft(str, &endptr, 0, val); + if(CURL_OFFT_FLOW == offt) + return PARAM_NUMBER_TOO_LARGE; + else if(CURL_OFFT_INVAL == offt) + return PARAM_BAD_NUMERIC; + } #else errno = 0; *val = strtol(str, &endptr, 0);