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

[Armin Burgmeier]

Thanks for the comments. I'll send an updated patch shortly.

Cheers,
Armin

On Sat, 2014-09-13 at 09:44 +0200, Nikos Mavrogiannopoulos wrote:
> On Fri, 2014-09-12 at 14:16 -0400, Armin Burgmeier wrote:
> 
> Thanks. Some comments inline.
> 
> > -	if (!(flags & GNUTLS_VERIFY_DISABLE_CA_SIGN) &&
> > -	    ((flags & GNUTLS_VERIFY_DO_NOT_ALLOW_X509_V1_CA_CRT)
> > -	     || issuer_version != 1)) {
> > +	} else if (issuer != NULL &&
> > +                   !(flags & GNUTLS_VERIFY_DISABLE_CA_SIGN) &&
> > +	           ((flags & GNUTLS_VERIFY_DO_NOT_ALLOW_X509_V1_CA_CRT)
> > +	            || issuer_version != 1)) {
> 
> After this point wouldn't it make sense to have one large block with an
> if (issuer != NULL)? (or at least one that is interrupted at some
> point). I'm worried about the multiple checks for if not NULL, as it is
> easy to miss one.

My motivation mostly was to avoid too many levels of indentation, to
maybe keep is easier to follow the logic of the code. But I don't have a
strong opinion. I have changed it to use a large issuer != NULL block in
the updated patch.

> >  	if (func) {
> >  		if (result == 0) {
> >  			out |= GNUTLS_CERT_INVALID;
> > @@ -1414,7 +1401,6 @@ gnutls_x509_crl_verify(gnutls_x509_crl_t crl,
> >  			*verify |=
> >  			    GNUTLS_CERT_SIGNER_NOT_FOUND |
> >  			    GNUTLS_CERT_INVALID;
> > -		return 0;
> >  	}
> >  
> >  	if (!(flags & GNUTLS_VERIFY_DISABLE_CA_SIGN)) {
> > @@ -1424,7 +1410,6 @@ gnutls_x509_crl_verify(gnutls_x509_crl_t crl,
> >  				*verify |=
> >  				    GNUTLS_CERT_SIGNER_NOT_CA |
> >  				    GNUTLS_CERT_INVALID;
> > -			return 0;
> >  		}
> 
> Shouldn't the if (issuer != NULL) be repeated here as well? Even if
> gnutls_x509_crt_get_ca_status() doesn't crash, you'll most probably get
> a flag GNUTLS_CERT_SIGNER_NOT_CA, which is misleading as there is no
> signer there.

Agreed. I added the check there.