Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501

[Darshit Shah]

On 11/11, Tim Rühsen wrote:
> On Tuesday 11 November 2014 11:58:26 Giuseppe Scrivano wrote:
> > Tim Ruehsen <address@hidden> writes:
> > > On Saturday 08 November 2014 13:00:13 Giuseppe Scrivano wrote:
> > >> Tim Ruehsen <address@hidden> writes:
> > >> > On Friday 07 November 2014 09:26:58 Giuseppe Scrivano wrote:
> > >> >> Tim Ruehsen <address@hidden> writes:
> > >> >> > Here is a first patch (GnuTLS only) for review and comments (and
> > >> >> > playing
> > >> >> > around).
> > >> >>
> > >> >> I think we should fail and avoid any connection instead of printing
> > >> >> just
> > >> >> a warning as it seems from the code now.  Have you tested it with some
> > >> >> crl file?  Would be good to add some automatic tests for this new
> > >> >> feature.
> > >> >>
> > >> >> > - Should we support complete directories ?
> > >> >> > - Should we allow more than one --crl-file option ?
> > >> >>
> > >> >> We can add this later, but we need to ensure that wget fails now if
> > >> >> more
> > >> >> --crl-file are passed so that the user knows it is not supported now.
> > >> >
> > >> > Amended patch.
> > >>
> > >> thanks, the patch looks fine to me.
> > >
> > > I just moved a block of code (loading of --ca-certificate) to the right
> > > place and added output on failure and success.
> > >
> > > To made up a test, I had to recreate testenv/certs. The former CN
> > > component
> > > did not have the correct name, which would allow us to generate a CRL
> > > file.
> > > This also allows us to use the CA cert (--ca-certificate=) and remove the
> > > very general --no-check-certificate from the Wget command line within
> > > Test-- https.py.
> > >
> > > The testenv/certs directory now seems somehow cleaner and better to
> > > understand (to me). I documented the cert/key/crl creation steps (using
> > > certtool) in testenv/certs/README.
> > >
> > > Review and comments appreciated.
> >
> > great work, it looks fine to me.  Feel free to push it.
>
> It has been pushed.
>
> Tim
Hi, 

I just looked at the patch.

diff --git a/src/openssl.c b/src/openssl.c
index 6685ee7..371913c 100644
--- a/src/openssl.c
+++ b/src/openssl.c
@@ -254,6 +254,22 @@ ssl_init (void)
  SSL_CTX_set_default_verify_paths (ssl_ctx);
  SSL_CTX_load_verify_locations (ssl_ctx, opt.ca_cert, opt.ca_directory);

+  if (opt.crl_file)
+    {
+      X509_STORE *store = SSL_CTX_get_cert_store (ssl_ctx);
+      X509_LOOKUP *lookup;
+      int rc;
Variable rc is declared here...

+
+      if (!(lookup = X509_STORE_add_lookup (store, X509_LOOKUP_file ()))
+          || (!(rc = X509_load_crl_file (lookup, opt.crl_file, 
X509_FILETYPE_PEM))))
rc is initialized **only** if the first condition is false.
+        {
+          logprintf (LOG_NOTQUIET, _("ERROR: Failed to load CRL file '%s': 
(%d)\n"), opt.crl_file, rc);
rc is used uninitialized in case the first condition is true.

Based on the error being printed, I'm going to assume that rc needs to be 
initialized in both cases. Or the error message needs to be changed.


--
Thanking You,
Darshit Shah

pgpf9fR2g3qiQ.pgp
Description: PGP signature