Darshit, could you test if these fixes pass the Coverity tests? I'm not particularly sure of the HSTS fix.
Regards, - AJ On Sun, 2015-12-06 at 22:45 +0100, Darshit Shah wrote: > ---------- Forwarded message ---------- > From: <[email protected]> > Date: 6 December 2015 at 22:39 > Subject: New Defects reported by Coverity Scan for GNU Wget > To: [email protected] > > > > Hi, > > Please find the latest report on new defect(s) introduced to GNU Wget > found with Coverity Scan. > > 6 new defect(s) introduced to GNU Wget found with Coverity Scan. > > > New defect(s) Reported-by: Coverity Scan > Showing 6 of 6 defect(s) > > > ** CID 1341706: (RESOURCE_LEAK) > /src/ftp.c: 1518 in getftp() > /src/ftp.c: 1528 in getftp() > /src/ftp.c: 1518 in getftp() > /src/ftp.c: 1518 in getftp() > > > ________________________________________________________________________________________________________ > *** CID 1341706: (RESOURCE_LEAK) > /src/ftp.c: 1518 in getftp() > 1512 logputs (LOG_NOTQUIET, "Server does not want to > resume the SSL session. Trying with a new one.\n"); > 1513 if (!ssl_connect_wget (dtsock, u->host, NULL)) > 1514 { > 1515 fd_close (csock); > 1516 fd_close (dtsock); > 1517 logputs (LOG_NOTQUIET, "Could not perform SSL > handshake.\n"); > >>> CID 1341706: (RESOURCE_LEAK) > >>> Variable "fp" going out of scope leaks the storage it points to. > 1518 return CONERROR; > 1519 } > 1520 } > 1521 else > 1522 logputs (LOG_NOTQUIET, "Resuming SSL session in data > connection.\n"); > 1523 > /src/ftp.c: 1528 in getftp() > 1522 logputs (LOG_NOTQUIET, "Resuming SSL session in data > connection.\n"); > 1523 > 1524 if (!ssl_check_certificate (dtsock, u->host)) > 1525 { > 1526 fd_close (csock); > 1527 fd_close (dtsock); > >>> CID 1341706: (RESOURCE_LEAK) > >>> Variable "fp" going out of scope leaks the storage it points to. > 1528 return CONERROR; > 1529 } > 1530 } > 1531 #endif > 1532 > 1533 /* Get the contents of the document. */ > /src/ftp.c: 1518 in getftp() > 1512 logputs (LOG_NOTQUIET, "Server does not want to > resume the SSL session. Trying with a new one.\n"); > 1513 if (!ssl_connect_wget (dtsock, u->host, NULL)) > 1514 { > 1515 fd_close (csock); > 1516 fd_close (dtsock); > 1517 logputs (LOG_NOTQUIET, "Could not perform SSL > handshake.\n"); > >>> CID 1341706: (RESOURCE_LEAK) > >>> Variable "fp" going out of scope leaks the storage it points to. > 1518 return CONERROR; > 1519 } > 1520 } > 1521 else > 1522 logputs (LOG_NOTQUIET, "Resuming SSL session in data > connection.\n"); > 1523 > /src/ftp.c: 1518 in getftp() > 1512 logputs (LOG_NOTQUIET, "Server does not want to > resume the SSL session. Trying with a new one.\n"); > 1513 if (!ssl_connect_wget (dtsock, u->host, NULL)) > 1514 { > 1515 fd_close (csock); > 1516 fd_close (dtsock); > 1517 logputs (LOG_NOTQUIET, "Could not perform SSL > handshake.\n"); > >>> CID 1341706: (RESOURCE_LEAK) > >>> Variable "fp" going out of scope leaks the storage it points to. > 1518 return CONERROR; > 1519 } > 1520 } > 1521 else > 1522 logputs (LOG_NOTQUIET, "Resuming SSL session in data > connection.\n"); > 1523 > > ** CID 1341705: Security best practices violations (TOCTOU) > /src/hsts.c: 479 in hsts_store_open() > > > ________________________________________________________________________________________________________ > *** CID 1341705: Security best practices violations (TOCTOU) > /src/hsts.c: 479 in hsts_store_open() > 473 > 474 if (file_exists_p (filename)) > 475 { > 476 if (stat (filename, &st) == 0) > 477 store->last_mtime = st.st_mtime; > 478 > >>> CID 1341705: Security best practices violations (TOCTOU) > >>> Calling function "fopen" that uses "filename" after a check function. > >>> This can cause a time-of-check, time-of-use race condition. > 479 fp = fopen (filename, "r"); > 480 if (!fp || !hsts_read_database (store, fp, false)) > 481 { > 482 /* abort! */ > 483 hsts_store_close (store); > 484 xfree (store); > > ** CID 1273467: API usage errors (BUFFER_SIZE) > /lib/md5.c: 291 in md5_process_bytes() > > > ________________________________________________________________________________________________________ > *** CID 1273467: API usage errors (BUFFER_SIZE) > /lib/md5.c: 291 in md5_process_bytes() > 285 memcpy (&((char *) ctx->buffer)[left_over], buffer, len); > 286 left_over += len; > 287 if (left_over >= 64) > 288 { > 289 md5_process_block (ctx->buffer, 64, ctx); > 290 left_over -= 64; > >>> CID 1273467: API usage errors (BUFFER_SIZE) > >>> The source buffer "&ctx->buffer[16]" potentially overlaps with the > >>> destination buffer "ctx->buffer", which results in undefined behavior for > >>> memcpy. > 291 memcpy (ctx->buffer, &ctx->buffer[16], left_over); > 292 } > 293 ctx->buflen = left_over; > 294 } > 295 } > 296 > > ** CID 1273466: API usage errors (BUFFER_SIZE) > /lib/sha256.c: 411 in sha256_process_bytes() > > > ________________________________________________________________________________________________________ > *** CID 1273466: API usage errors (BUFFER_SIZE) > /lib/sha256.c: 411 in sha256_process_bytes() > 405 memcpy (&((char *) ctx->buffer)[left_over], buffer, len); > 406 left_over += len; > 407 if (left_over >= 64) > 408 { > 409 sha256_process_block (ctx->buffer, 64, ctx); > 410 left_over -= 64; > >>> CID 1273466: API usage errors (BUFFER_SIZE) > >>> The source buffer "&ctx->buffer[16]" potentially overlaps with the > >>> destination buffer "ctx->buffer", which results in undefined behavior for > >>> memcpy. > 411 memcpy (ctx->buffer, &ctx->buffer[16], left_over); > 412 } > 413 ctx->buflen = left_over; > 414 } > 415 } > 416 > > ** CID 1273463: API usage errors (BUFFER_SIZE) > /lib/sha1.c: 278 in sha1_process_bytes() > > > ________________________________________________________________________________________________________ > *** CID 1273463: API usage errors (BUFFER_SIZE) > /lib/sha1.c: 278 in sha1_process_bytes() > 272 memcpy (&((char *) ctx->buffer)[left_over], buffer, len); > 273 left_over += len; > 274 if (left_over >= 64) > 275 { > 276 sha1_process_block (ctx->buffer, 64, ctx); > 277 left_over -= 64; > >>> CID 1273463: API usage errors (BUFFER_SIZE) > >>> The source buffer "&ctx->buffer[16]" potentially overlaps with the > >>> destination buffer "ctx->buffer", which results in undefined behavior for > >>> memcpy. > 278 memcpy (ctx->buffer, &ctx->buffer[16], left_over); > 279 } > 280 ctx->buflen = left_over; > 281 } > 282 } > 283 > > ** CID 420711: Insecure data handling (INTEGER_OVERFLOW) > /lib/str-two-way.h: 221 in critical_factorization() > > > ________________________________________________________________________________________________________ > *** CID 420711: Insecure data handling (INTEGER_OVERFLOW) > /lib/str-two-way.h: 221 in critical_factorization() > 215 lexicographic suffix of 'a' works for 'bba', but not 'ab' for > 216 'aab'. The shorter suffix of the two will always be a critical > 217 factorization. */ > 218 if (max_suffix_rev + 1 < max_suffix + 1) > 219 return max_suffix + 1; > 220 *period = p; > >>> CID 420711: Insecure data handling (INTEGER_OVERFLOW) > >>> Overflowed or truncated value (or a value computed from an overflowed > >>> or truncated value) "max_suffix_rev + 1UL" used as return value. > 221 return max_suffix_rev + 1; > 222 } > 223 > 224 /* Return the first location of non-empty NEEDLE within HAYSTACK, or > 225 NULL. HAYSTACK_LEN is the minimum known length of HAYSTACK. This > 226 method is optimized for NEEDLE_LEN < LONG_NEEDLE_THRESHOLD. > > > ________________________________________________________________________________________________________ > To view the defects in Coverity Scan visit, > https://scan.coverity.com/projects/gnu-wget?tab=overview > > To manage Coverity Scan email notifications for "[email protected]", > click > https://scan.coverity.com/subscriptions/edit?email=darnir%40gmail.com&token=a247cf0e017fe1ea3e52680a7e0c1fcf > > >
From c78aee99ba099a61af26c9df4c072e6e7a65cb03 Mon Sep 17 00:00:00 2001 From: Ander Juaristi <[email protected]> Date: Wed, 9 Dec 2015 17:12:51 +0100 Subject: [PATCH] Fix Coverity issues * src/ftp.c (getftp): on error, close the file and attempt to remove it before exiting. * src/hsts.c (hsts_store_open): update modification time in the end. --- src/ftp.c | 16 +++++++++++++--- src/hsts.c | 16 +++++++++------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/ftp.c b/src/ftp.c index 5394b71..002842e 100644 --- a/src/ftp.c +++ b/src/ftp.c @@ -321,7 +321,8 @@ getftp (struct url *u, wgint passed_expected_bytes, wgint *qtyread, { int csock, dtsock, local_sock, res; uerr_t err = RETROK; /* appease the compiler */ - FILE *fp; + FILE *fp = NULL; + struct_fstat st; char *respline, *tms; const char *user, *passwd, *tmrate; int cmd = con->cmd; @@ -1514,8 +1515,9 @@ Error in server response, closing control connection.\n")); { fd_close (csock); fd_close (dtsock); + err = CONERROR; logputs (LOG_NOTQUIET, "Could not perform SSL handshake.\n"); - return CONERROR; + goto exit_error; } } else @@ -1525,7 +1527,8 @@ Error in server response, closing control connection.\n")); { fd_close (csock); fd_close (dtsock); - return CONERROR; + err = CONERROR; + goto exit_error; } } #endif @@ -1762,6 +1765,13 @@ Error in server response, closing control connection.\n")); } } while (try_again); return RETRFINISHED; + +exit_error: + + /* If fp is a regular file, close and try to remove it */ + if (fp && !output_stream) + fclose (fp); + return err; } /* A one-file FTP loop. This is the part where FTP retrieval is diff --git a/src/hsts.c b/src/hsts.c index 3ddbf72..1583659 100644 --- a/src/hsts.c +++ b/src/hsts.c @@ -464,7 +464,7 @@ hsts_store_t hsts_store_open (const char *filename) { hsts_store_t store = NULL; - struct stat st; + struct_stat st; FILE *fp = NULL; store = xnew0 (struct hsts_store); @@ -473,27 +473,29 @@ hsts_store_open (const char *filename) if (file_exists_p (filename)) { - if (stat (filename, &st) == 0) - store->last_mtime = st.st_mtime; - fp = fopen (filename, "r"); + if (!fp || !hsts_read_database (store, fp, false)) { /* abort! */ hsts_store_close (store); xfree (store); + goto out; } - if (fp) - fclose (fp); + + if (fstat (fileno (fp), &st) == 0) + store->last_mtime = st.st_mtime; + fclose (fp); } +out: return store; } void hsts_store_save (hsts_store_t store, const char *filename) { - struct stat st; + struct_stat st; FILE *fp = NULL; int fd = 0; -- 2.1.4
