[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-wget] Fwd: New Defects reported by Coverity Scan for GNU Wget
|
From: |
Tim Rühsen |
|
Subject: |
Re: [Bug-wget] Fwd: New Defects reported by Coverity Scan for GNU Wget |
|
Date: |
Sun, 13 Dec 2015 16:42:16 +0100 |
|
User-agent: |
KMail/4.14.10 (Linux/4.2.0-1-amd64; KDE/4.14.14; x86_64; ; ) |
Thanks, Ander.
I splitted your patch into two and pushed them.
Regards, Tim
Am Sonntag, 13. Dezember 2015, 15:29:26 schrieb Ander Juaristi:
> Hi,
>
> Sorry I came out late, but I've just spotted a code path where fp is not
> closed.
>
> Regards,
> - AJ
>
> On 12/10/2015 11:14 PM, Darshit Shah wrote:
> > 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: <address@hidden>
> >>> Date: 6 December 2015 at 22:39
> >>> Subject: New Defects reported by Coverity Scan for GNU Wget
> >>> To: address@hidden
> >>>
> >>>
> >>>
> >>> 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 "address@hidden",
> >>> click
> >>> https://scan.coverity.com/subscriptions/edit?email=darnir%40gmail.com&t
> >>> oken=a247cf0e017fe1ea3e52680a7e0c1fcf>>
> >> From c78aee99ba099a61af26c9df4c072e6e7a65cb03 Mon Sep 17 00:00:00 2001
> >> From: Ander Juaristi <address@hidden>
> >> 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