Re: [Bug-wget] Fwd: New Defects reported by Coverity Scan for GNU Wget

[Darshit Shah]

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:  
Date: 6 December 2015 at 22:39
Subject: New Defects reported by Coverity Scan for GNU Wget
To: dar...@gmail.com



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, ) == 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 

Re: [Bug-wget] --no-check-cert does not avoid cert warning

[Giuseppe Scrivano]

Ángel González  writes:

> Giuseppe Scrivano wrote:
>> Hi Ángel,
>>
>>
>> thanks for the suggestion, it looks fine to me.  I already pushed the
>> patch, could you prepare a new one that adds this part?
>>
>> Regards,
>> Giuseppe
> Sure. This was actually a diff against the file in master, so if you
> are ok with the contents,
> it only needed a commit log.

thanks, recommendations for users are never enough :)  I have shortened
the commit message and pushed it.

Giuseppe

Re: [Bug-wget] --no-check-cert does not avoid cert warning

[Ángel González]

Giuseppe Scrivano writes:

Ángel González  writes:

Sure. This was actually a diff against the file in master, so if you
are ok with the contents,
it only needed a commit log.

thanks, recommendations for users are never enough :)  I have shortened
the commit message and pushed it.

Giuseppe

:)

No problem. We also need to do our part in having them do the Right Thing™
Thanks Giuseppe.

Best

Re: [Bug-wget] Marking Release v1.17.1?

[Jernej Simončič]

On Tuesday, December 8, 2015, 21:45:31, Darshit Shah wrote:

> With my last set of patches, we have fixed all the issues reported / 
> identified
> after the 1.17 release. Hence, maybe we should consider releasing a 1.17.1
> bugfix release?

Sorry about reporting this late, it forgot about it a bit: when I was
preparing my Windows release, I noticed that metalink support first
didn't want to compile (due to missing sys/errno.h - just #ifdefing it
out didn't seem to break anything), and then that hash validation
failed, because the file was being opened in text mode. I did a quick
and dirty fix for my build
,
but this probably isn't correct for non-Windows systems.

Another thing that would be nice to include is the taskbar progressbar
patch:

This one is based on patch by Ángel González and tbprogress.c from
Gisle Vanem.

-- 
< Jernej Simončič ><><><><>< http://eternallybored.org/ >

When you are over the hill, you pick up speed.
   -- Baker's Byroad

Re: [Bug-wget] Windows cert store support

[Gisle Vanem]

Random Coder wrote:

> I'm not sure if the wget maintainers would be interested, but I've
> been carrying this patch around in my private builds of wget for a
> while.  It allows wget to load SSL certs from the default Windows cert
> store.

I've applied your patch. It seems to work fine. Nice!

But in a message like:
  X509 certificate successfully verified and matches host
  www.ssllabs.com

it would be nice to know if it succeeded because of WinCrypt or
OpenSSL.

> +  /* Loop through all the certs in the Windows cert store */
> +  for ( pCertCtx = Local_CertEnumCertificatesInStore(hStore, NULL);
> +  pCertCtx != NULL;
> +  pCertCtx = Local_CertEnumCertificatesInStore(hStore, pCertCtx) )
> +  {
> +if (!((pCertCtx->dwCertEncodingType & PKCS_7_ASN_ENCODING) == 
> PKCS_7_ASN_ENCODING))
> +{
> +  /* Add all certs we find to OpenSSL's store */

How does this prevent an expired Cert to be used?
I see in the 'CERT_INFO' structure a 'NotAfter' member. But this
struct seems to support for WINAPI_PARTITION_APP only :-(
I assume this could be used to check expired certificates.

-- 
--gv