Looks good to me. And coverity shows two issues fixed. So I'll push it.
On 12/09, Juaristi Álamos, Ander wrote:
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
-- Thanking You, Darshit Shah
signature.asc
Description: PGP signature
