bug-wget
[Top][All Lists]
Advanced
[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: Ander Juaristi
Subject: Re: [Bug-wget] Fwd: New Defects reported by Coverity Scan for GNU Wget
Date: Sun, 13 Dec 2015 15:29:26 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 
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&token=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

Attachment: 0001-Fix-potential-leak.patch
Description: Text Data

reply via email to
[Prev in Thread] Current Thread [Next in Thread]