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

[Nikos Mavrogiannopoulos]

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.

>  	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.

regards,
Nikos