Hi,As a result of the last security vulnerability we had, I'm adding new code to checksrc that will alert us on uses of (v)sprintf, strcat and gets in the code base.
This is meant to be an additional tool to help us detect unsafe code easier, since all those functions are too easily use without careful considerations of all possible side-effects.
I also had to clean up some code so that this wouldn't immediately start complaining! =)
Comments or improvements? -- / daniel.haxx.se
From 64bc4ef6a6ddaf56422a13b68e67484d845e7d90 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg <[email protected]> Date: Wed, 6 Mar 2013 13:27:51 +0100 Subject: [PATCH] checksrc: ban unsafe functions The list of unsafe functions currently consists of sprintf, vsprintf, strcat and gets. Subsequently, some existing code needed updating to avoid warnings on this. --- include/curl/mprintf.h | 4 +- lib/checksrc.pl | 8 +++- lib/ftp.c | 17 +++---- lib/http_digest.c | 8 ++-- lib/mprintf.c | 117 ++++-------------------------------------------- src/tool_dirhie.c | 10 +++-- src/tool_operate.c | 14 +++--- src/tool_operhlp.c | 14 +++--- src/tool_parsecfg.c | 23 +++++----- src/tool_setopt.c | 8 ++-- 10 files changed, 64 insertions(+), 159 deletions(-) diff --git a/include/curl/mprintf.h b/include/curl/mprintf.h index de7dd2f..cc9e7f5 100644 --- a/include/curl/mprintf.h +++ b/include/curl/mprintf.h @@ -7,7 +7,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2006, Daniel Stenberg, <[email protected]>, et al. + * Copyright (C) 1998 - 2013, Daniel Stenberg, <[email protected]>, et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -58,7 +58,7 @@ CURL_EXTERN char *curl_mvaprintf(const char *format, va_list args); # define printf curl_mprintf # define fprintf curl_mfprintf #ifdef CURLDEBUG -/* When built with CURLDEBUG we define away the sprintf() functions since we +/* When built with CURLDEBUG we define away the sprintf functions since we don't want internal code to be using them */ # define sprintf sprintf_was_used # define vsprintf vsprintf_was_used diff --git a/lib/checksrc.pl b/lib/checksrc.pl index 9f5058d..eb9e38a 100755 --- a/lib/checksrc.pl +++ b/lib/checksrc.pl @@ -6,7 +6,7 @@ # | (__| |_| | _ <| |___ # \___|\___/|_| \_\_____| # -# Copyright (C) 2011, Daniel Stenberg, <[email protected]>, et al. +# Copyright (C) 2011 - 2013, Daniel Stenberg, <[email protected]>, et al. # # This software is licensed as described in the file COPYING, which # you should have received as part of this distribution. The terms @@ -153,6 +153,12 @@ sub scanfile { checkwarn($line, length($1)+1, $file, $l, "missing space after close paren"); } + # scan for use of banned functions + if($l =~ /^(.*\W)(sprintf|vsprintf|strcat|gets) *\(/) { + checkwarn($line, length($1), $file, $l, + "use of $2 is banned"); + } + # check for open brace first on line but not first column # only alert if previous line ended with a close paren and wasn't a cpp # line diff --git a/lib/ftp.c b/lib/ftp.c index dc9fc48..d9b8547 100644 --- a/lib/ftp.c +++ b/lib/ftp.c @@ -3978,16 +3978,11 @@ static CURLcode wc_statemach(struct connectdata *conn) /* filelist has at least one file, lets get first one */ struct ftp_conn *ftpc = &conn->proto.ftpc; struct curl_fileinfo *finfo = wildcard->filelist->head->ptr; - char *tmp_path = malloc(strlen(conn->data->state.path) + - strlen(finfo->filename) + 1); - if(!tmp_path) { + + char *tmp_path = aprintf("%s%s", wildcard->path, finfo->filename); + if(!tmp_path) return CURLE_OUT_OF_MEMORY; - } - tmp_path[0] = 0; - /* make full path to matched file */ - strcat(tmp_path, wildcard->path); - strcat(tmp_path, finfo->filename); /* switch default "state.pathbuffer" and tmp_path, good to see ftp_parse_url_path function to understand this trick */ Curl_safefree(conn->data->state.pathbuffer); @@ -4124,13 +4119,13 @@ CURLcode Curl_ftpsendf(struct connectdata *conn, va_list ap; va_start(ap, fmt); - vsnprintf(s, SBUF_SIZE-3, fmt, ap); + write_len = vsnprintf(s, SBUF_SIZE-3, fmt, ap); va_end(ap); - strcat(s, "\r\n"); /* append a trailing CRLF */ + strcpy(&s[write_len], "\r\n"); /* append a trailing CRLF */ + write_len +=2; bytes_written=0; - write_len = strlen(s); res = Curl_convert_to_network(conn->data, s, write_len); /* Curl_convert_to_network calls failf if unsuccessful */ diff --git a/lib/http_digest.c b/lib/http_digest.c index f9f20d4..4351396 100644 --- a/lib/http_digest.c +++ b/lib/http_digest.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2012, Daniel Stenberg, <[email protected]>, et al. + * Copyright (C) 1998 - 2013, Daniel Stenberg, <[email protected]>, et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -287,6 +287,7 @@ CURLcode Curl_output_digest(struct connectdata *conn, struct timeval now; char **allocuserpwd; + size_t userlen; const char *userp; const char *passwdp; struct auth *authp; @@ -533,10 +534,11 @@ CURLcode Curl_output_digest(struct connectdata *conn, } /* append CRLF + zero (3 bytes) to the userpwd header */ - tmp = realloc(*allocuserpwd, strlen(*allocuserpwd) + 3); + userlen = strlen(*allocuserpwd); + tmp = realloc(*allocuserpwd, userlen + 3); if(!tmp) return CURLE_OUT_OF_MEMORY; - strcat(tmp, "\r\n"); + strcpy(&tmp[userlen], "\r\n"); /* append the data */ *allocuserpwd = tmp; return CURLE_OK; diff --git a/lib/mprintf.c b/lib/mprintf.c index b5b8153..2ec4a75 100644 --- a/lib/mprintf.c +++ b/lib/mprintf.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1999 - 2011, Daniel Stenberg, <[email protected]>, et al. + * Copyright (C) 1999 - 2013, Daniel Stenberg, <[email protected]>, et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -203,101 +203,6 @@ static int dprintf_IsQualifierNoDollar(char c) } } -#ifdef DPRINTF_DEBUG2 -static void dprintf_Pass1Report(va_stack_t *vto, int max) -{ - int i; - char buffer[256]; - int bit; - int flags; - - for(i=0; i<max; i++) { - char *type; - switch(vto[i].type) { - case FORMAT_UNKNOWN: - type = "unknown"; - break; - case FORMAT_STRING: - type ="string"; - break; - case FORMAT_PTR: - type ="pointer"; - break; - case FORMAT_INT: - type = "int"; - break; - case FORMAT_INTPTR: - type = "intptr"; - break; - case FORMAT_LONG: - type = "long"; - break; - case FORMAT_LONGLONG: - type = "long long"; - break; - case FORMAT_DOUBLE: - type = "double"; - break; - case FORMAT_LONGDOUBLE: - type = "long double"; - break; - } - - - buffer[0]=0; - - for(bit=0; bit<31; bit++) { - flags = vto[i].flags & (1<<bit); - - if(flags & FLAGS_SPACE) - strcat(buffer, "space "); - else if(flags & FLAGS_SHOWSIGN) - strcat(buffer, "plus "); - else if(flags & FLAGS_LEFT) - strcat(buffer, "left "); - else if(flags & FLAGS_ALT) - strcat(buffer, "alt "); - else if(flags & FLAGS_SHORT) - strcat(buffer, "short "); - else if(flags & FLAGS_LONG) - strcat(buffer, "long "); - else if(flags & FLAGS_LONGLONG) - strcat(buffer, "longlong "); - else if(flags & FLAGS_LONGDOUBLE) - strcat(buffer, "longdouble "); - else if(flags & FLAGS_PAD_NIL) - strcat(buffer, "padnil "); - else if(flags & FLAGS_UNSIGNED) - strcat(buffer, "unsigned "); - else if(flags & FLAGS_OCTAL) - strcat(buffer, "octal "); - else if(flags & FLAGS_HEX) - strcat(buffer, "hex "); - else if(flags & FLAGS_UPPER) - strcat(buffer, "upper "); - else if(flags & FLAGS_WIDTH) - strcat(buffer, "width "); - else if(flags & FLAGS_WIDTHPARAM) - strcat(buffer, "widthparam "); - else if(flags & FLAGS_PREC) - strcat(buffer, "precision "); - else if(flags & FLAGS_PRECPARAM) - strcat(buffer, "precparam "); - else if(flags & FLAGS_CHAR) - strcat(buffer, "char "); - else if(flags & FLAGS_FLOATE) - strcat(buffer, "floate "); - else if(flags & FLAGS_FLOATG) - strcat(buffer, "floatg "); - } - printf("REPORT: %d. %s [%s]\n", i, type, buffer); - - } - - -} -#endif - /****************************************************************** * * Pass 1: @@ -537,10 +442,6 @@ static long dprintf_Pass1(const char *format, va_stack_t *vto, char **endpos, } } -#ifdef DPRINTF_DEBUG2 - dprintf_Pass1Report(vto, max_param); -#endif - /* Read the arg list parameters into our data list */ for(i=0; i<max_param; i++) { if((i + 1 < max_param) && (vto[i + 1].type == FORMAT_WIDTH)) { @@ -919,7 +820,7 @@ static int dprintf_formatf( case FORMAT_DOUBLE: { char formatbuf[32]="%"; - char *fptr; + char *fptr = &formatbuf[1]; size_t left = sizeof(formatbuf)-strlen(formatbuf); int len; @@ -936,15 +837,15 @@ static int dprintf_formatf( prec = (long)vto[p->precision].data.num.as_signed; if(p->flags & FLAGS_LEFT) - strcat(formatbuf, "-"); + *fptr++ = '-'; if(p->flags & FLAGS_SHOWSIGN) - strcat(formatbuf, "+"); + *fptr++ = '+'; if(p->flags & FLAGS_SPACE) - strcat(formatbuf, " "); + *fptr++ = ' '; if(p->flags & FLAGS_ALT) - strcat(formatbuf, "#"); + *fptr++ = '#'; - fptr=&formatbuf[strlen(formatbuf)]; + *fptr = 0; if(width >= 0) { /* RECURSIVE USAGE */ @@ -969,8 +870,8 @@ static int dprintf_formatf( *fptr = 0; /* and a final zero termination */ - /* NOTE NOTE NOTE!! Not all sprintf() implementations returns number - of output characters */ + /* NOTE NOTE NOTE!! Not all sprintf implementations return number of + output characters */ (sprintf)(work, formatbuf, p->data.dnum); for(fptr=work; *fptr; fptr++) diff --git a/src/tool_dirhie.c b/src/tool_dirhie.c index 4ba1c43..5965f7a 100644 --- a/src/tool_dirhie.c +++ b/src/tool_dirhie.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2012, Daniel Stenberg, <[email protected]>, et al. + * Copyright (C) 1998 - 2013, Daniel Stenberg, <[email protected]>, et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -98,12 +98,14 @@ CURLcode create_dir_hierarchy(const char *outfile, FILE *errors) char *outdup; char *dirbuildup; CURLcode result = CURLE_OK; + size_t outlen; + outlen = strlen(outfile); outdup = strdup(outfile); if(!outdup) return CURLE_OUT_OF_MEMORY; - dirbuildup = malloc(strlen(outfile) + 1); + dirbuildup = malloc(outlen + 1); if(!dirbuildup) { Curl_safefree(outdup); return CURLE_OUT_OF_MEMORY; @@ -119,12 +121,12 @@ CURLcode create_dir_hierarchy(const char *outfile, FILE *errors) if(tempdir2 != NULL) { size_t dlen = strlen(dirbuildup); if(dlen) - sprintf(&dirbuildup[dlen], "%s%s", DIR_CHAR, tempdir); + snprintf(&dirbuildup[dlen], outlen - dlen, "%s%s", DIR_CHAR, tempdir); else { if(0 != strncmp(outdup, DIR_CHAR, 1)) strcpy(dirbuildup, tempdir); else - sprintf(dirbuildup, "%s%s", DIR_CHAR, tempdir); + snprintf(dirbuildup, outlen, "%s%s", DIR_CHAR, tempdir); } if(access(dirbuildup, F_OK) == -1) { if(-1 == mkdir(dirbuildup,(mode_t)0000750)) { diff --git a/src/tool_operate.c b/src/tool_operate.c index 5e73d86..3151f41 100644 --- a/src/tool_operate.c +++ b/src/tool_operate.c @@ -805,18 +805,18 @@ int operate(struct Configurable *config, int argc, argv_item_t argv[]) /* * Then append ? followed by the get fields to the url. */ - urlbuffer = malloc(strlen(this_url) + strlen(httpgetfields) + 3); - if(!urlbuffer) { - res = CURLE_OUT_OF_MEMORY; - goto show_error; - } if(pc) - sprintf(urlbuffer, "%s%c%s", this_url, sep, httpgetfields); + urlbuffer = aprintf("%s%c%s", this_url, sep, httpgetfields); else /* Append / before the ? to create a well-formed url if the url contains a hostname only */ - sprintf(urlbuffer, "%s/?%s", this_url, httpgetfields); + urlbuffer = aprintf("%s/?%s", this_url, httpgetfields); + + if(!urlbuffer) { + res = CURLE_OUT_OF_MEMORY; + goto show_error; + } Curl_safefree(this_url); /* free previous URL */ this_url = urlbuffer; /* use our new URL instead! */ diff --git a/src/tool_operhlp.c b/src/tool_operhlp.c index 6314887..d3c1a88 100644 --- a/src/tool_operhlp.c +++ b/src/tool_operhlp.c @@ -123,22 +123,20 @@ char *add_file_name_to_url(CURL *curl, char *url, const char *filename) /* URL encode the file name */ encfile = curl_easy_escape(curl, filep, 0 /* use strlen */); if(encfile) { - char *urlbuffer = malloc(strlen(url) + strlen(encfile) + 3); - if(!urlbuffer) { - curl_free(encfile); - Curl_safefree(url); - return NULL; - } + char *urlbuffer; if(ptr) /* there is a trailing slash on the URL */ - sprintf(urlbuffer, "%s%s", url, encfile); + urlbuffer = aprintf("%s%s", url, encfile); else /* there is no trailing slash on the URL */ - sprintf(urlbuffer, "%s/%s", url, encfile); + urlbuffer = aprintf("%s/%s", url, encfile); curl_free(encfile); Curl_safefree(url); + if(!urlbuffer) + return NULL; + url = urlbuffer; /* use our new URL instead! */ } } diff --git a/src/tool_parsecfg.c b/src/tool_parsecfg.c index 561dada..680688a 100644 --- a/src/tool_parsecfg.c +++ b/src/tool_parsecfg.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2012, Daniel Stenberg, <[email protected]>, et al. + * Copyright (C) 1998 - 2013, Daniel Stenberg, <[email protected]>, et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -275,32 +275,33 @@ static char *my_get_line(FILE *fp) { char buf[4096]; char *nl = NULL; - char *retval = NULL; + char *line = NULL; do { if(NULL == fgets(buf, sizeof(buf), fp)) break; - if(!retval) { - retval = strdup(buf); - if(!retval) + if(!line) { + line = strdup(buf); + if(!line) return NULL; } else { char *ptr; - ptr = realloc(retval, strlen(retval) + strlen(buf) + 1); + size_t linelen = strlen(line); + ptr = realloc(line, linelen + strlen(buf) + 1); if(!ptr) { - Curl_safefree(retval); + Curl_safefree(line); return NULL; } - retval = ptr; - strcat(retval, buf); + line = ptr; + strcpy(&line[linelen], buf); } - nl = strchr(retval, '\n'); + nl = strchr(line, '\n'); } while(!nl); if(nl) *nl = '\0'; - return retval; + return line; } diff --git a/src/tool_setopt.c b/src/tool_setopt.c index 4014177..4493e5f 100644 --- a/src/tool_setopt.c +++ b/src/tool_setopt.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2012, Daniel Stenberg, <[email protected]>, et al. + * Copyright (C) 1998 - 2013, Daniel Stenberg, <[email protected]>, et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -199,7 +199,7 @@ static char *c_escape(const char *str) e += 2; } else if(! isprint(c)) { - sprintf(e, "\\%03o", c); + snprintf(e, 4, "\\%03o", c); e += 4; } else @@ -270,7 +270,7 @@ CURLcode tool_setopt_flags(CURL *curl, struct Configurable *config, if(!rest) break; /* handled them all */ /* replace with all spaces for continuation line */ - sprintf(preamble, "%*s", strlen(preamble), ""); + snprintf(preamble, sizeof(preamble), "%*s", strlen(preamble), ""); } } /* If any bits have no definition, output an explicit value. @@ -313,7 +313,7 @@ CURLcode tool_setopt_bitmask(CURL *curl, struct Configurable *config, if(!rest) break; /* handled them all */ /* replace with all spaces for continuation line */ - sprintf(preamble, "%*s", strlen(preamble), ""); + snprintf(preamble, sizeof(preamble), "%*s", strlen(preamble), ""); } } /* If any bits have no definition, output an explicit value. -- 1.7.10.4
------------------------------------------------------------------- List admin: http://cool.haxx.se/list/listinfo/curl-library Etiquette: http://curl.haxx.se/mail/etiquette.html
