[gnutls-devel] [PATCH] Check for all error conditions when verifying a certificate

[Armin Burgmeier]

On Tue, 2014-09-16 at 13:32 +0200, Nikos Mavrogiannopoulos wrote:
> On Sat, Sep 13, 2014 at 5:46 PM, Armin Burgmeier <armin at arbur.net> wrote:
> > This allows to check for all possible flaws with a certificate chain with a
> > single call to gnutls_x509_crt_list_verify and friends.
> 
> > +               if (sigalg >= 0 &&
> > +                   is_level_acceptable(cert, issuer, sigalg, flags) == 0) {
> > +                       gnutls_assert();
> > +                       out |=
> > +                           GNUTLS_CERT_INSECURE_ALGORITHM |
> > +                           GNUTLS_CERT_INVALID;
> > +                       result = 0;
> > +               }
> 
> Hi,
>  The security level check will not be performed if there is no issuer.
> That means if the issuer wasn't found you'll not know whether
> GNUTLS_CERT_INSECURE_ALGORITHM would have been set for the specified
> security level. Would that be acceptable?

For my own use case yes, but I agree that while we are at it we should
consider this as well. I have changed the function so that it is allowed
to be called with a NULL issuer.

> Also you added the "if (sigalg >= 0)"... Why is that needed? Isn't
> sigalg always positive at this point?

It can be negative if _gnutls_x509_get_signature_algorithm returns an
error code.

> >         {
> > diff --git a/tests/test-chains.h b/tests/test-chains.h
> > index 28974e1..ff9086f 100644
> > --- a/tests/test-chains.h
> > +++ b/tests/test-chains.h
> > @@ -1366,9 +1366,11 @@ static struct
> >  } chains[] =
> >  {
> >    { "CVE-2014-0092", cve_2014_0092_check, &cve_2014_0092_check[1],
> > -    0, GNUTLS_CERT_SIGNER_NOT_CA | GNUTLS_CERT_INVALID },
> > +    GNUTLS_VERIFY_DISABLE_TIME_CHECKS,
> > +    GNUTLS_CERT_SIGNER_NOT_CA | GNUTLS_CERT_INVALID },
> >    { "CVE-2008-4989", cve_2008_4989_chain, &cve_2008_4989_chain[2],
> > -    0, GNUTLS_CERT_SIGNER_NOT_FOUND | GNUTLS_CERT_INVALID },
> > +    GNUTLS_VERIFY_DISABLE_TIME_CHECKS,
> > +    GNUTLS_CERT_SIGNER_NOT_FOUND | GNUTLS_CERT_INVALID },
> 
> Wouldn't here instead of adding GNUTLS_VERIFY_DISABLE_TIME_CHECKS, to
> add the GNUTLS_CERT_EXPIRED in the expected result? That way we can
> test that the results remain consistent from now on.

I agree, that makes sense.

> Other than these, it looks reasonable. btw. would you like to send a
> DCO on the list (as in http://www.gnutls.org/devel.html) ?

Yes, I'll do it. Will also work on a patch for the getter functions for
gnutls_certificate_credentials_t that we discussed about.

> regards,
> Nikos

Cheers,
Armin