On Mon, Jun 10, 2024 at 05:33:19AM +0200, Theo Buehler wrote:
> x509_get_purpose() handles basic constraints and extended key usage.
> Key usage also belongs there. By moving it, we now also check it for
> BGPsec Router certs.
> 
> Add missing check that the extension is critical. Criticality is only a
> SHOULD in RFC 5280, so libcrypto doesn't enforce it and of course makes
> it annoying to check. It's a MUST in 6487, 4.8.4.
> 
> EKU for EE certs is handled in x509_get_purpose(), so zap that check in
> cert_parse_ee_cert() and move the one for CA/TA in cert_parse_pre()
> without the incorrect 'may be allowed' comment since that applies only
> to EE certs.
> 
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> diff -u -p -r1.144 cert.c
> --- cert.c	8 Jun 2024 13:33:49 -0000	1.144
> +++ cert.c	10 Jun 2024 02:46:53 -0000
> @@ -753,18 +753,6 @@ cert_parse_ee_cert(const char *fn, int t
>  		goto out;
>  	}
>  
> -	if (X509_get_key_usage(x) != KU_DIGITAL_SIGNATURE) {
> -		warnx("%s: RFC 6487 section 4.8.4: KU must be digitalSignature",
> -		    fn);
> -		goto out;
> -	}
> -
> -	/* EKU may be allowed for some purposes in the future. */
> -	if (X509_get_extended_key_usage(x) != UINT32_MAX) {
> -		warnx("%s: RFC 6487 section 4.8.5: EKU not allowed", fn);
> -		goto out;
> -	}
> -
>  	index = X509_get_ext_by_NID(x, NID_sbgp_ipAddrBlock, -1);
>  	if ((ext = X509_get_ext(x, index)) != NULL) {
>  		if (!sbgp_ipaddrblk(fn, cert, ext))
> @@ -976,19 +964,6 @@ cert_parse_pre(const char *fn, const uns
>  		}
>  		if (!valid_ca_pkey(fn, pkey))
>  			goto out;
> -
> -		if (X509_get_key_usage(x) != (KU_KEY_CERT_SIGN | KU_CRL_SIGN)) {
> -			warnx("%s: RFC 6487 section 4.8.4: key usage violation",
> -			    fn);
> -			goto out;
> -		}
> -
> -		/* EKU may be allowed for some purposes in the future. */
> -		if (X509_get_extended_key_usage(x) != UINT32_MAX) {
> -			warnx("%s: RFC 6487 section 4.8.5: EKU not allowed",
> -			    fn);
> -			goto out;
> -		}
>  
>  		if (cert->mft == NULL) {
>  			warnx("%s: RFC 6487 section 4.8.8: missing SIA", fn);
> Index: x509.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
> diff -u -p -r1.97 x509.c
> --- x509.c	8 Jun 2024 13:32:30 -0000	1.97
> +++ x509.c	10 Jun 2024 03:12:36 -0000
> @@ -267,15 +267,17 @@ x509_get_ski(X509 *x, const char *fn, ch
>  
>  /*
>   * Check the cert's purpose: the cA bit in basic constraints distinguishes
> - * between TA/CA and EE/BGPsec router. TAs are self-signed, CAs not self-issued,
> - * EEs have no extended key usage, BGPsec router have id-kp-bgpsec-router OID.
> + * between TA/CA and EE/BGPsec router and the key usage bits must match.
> + * TAs are self-signed, CAs not self-issued, EEs have no extended key usage,
> + * BGPsec router have id-kp-bgpsec-router OID.
>   */
>  enum cert_purpose
>  x509_get_purpose(X509 *x, const char *fn)
>  {
>  	BASIC_CONSTRAINTS		*bc = NULL;
>  	EXTENDED_KEY_USAGE		*eku = NULL;
> -	int				 crit, ext_flags, is_ca;
> +	const X509_EXTENSION		*ku;
> +	int				 crit, ext_flags, is_ca, ku_idx;
>  	enum cert_purpose		 purpose = CERT_PURPOSE_INVALID;
>  
>  	if (!x509_cache_extensions(x, fn))
> @@ -283,6 +285,20 @@ x509_get_purpose(X509 *x, const char *fn
>  
>  	ext_flags = X509_get_extension_flags(x);
>  
> +	/* Key usage must be present and critical. KU bits are checked below. */
> +	if ((ku_idx = X509_get_ext_by_NID(x, NID_key_usage, -1)) < 0) {
> +		warnx("%s: RFC 6487, section 4.8.4: missing KeyUsage", fn);
> +		goto out;
> +	}
> +	if ((ku = X509_get_ext(x, ku_idx)) == NULL) {
> +		warnx("%s: RFC 6487, section 4.8.4: missing KeyUsage", fn);
> +		goto out;
> +	}
> +	if (!X509_EXTENSION_get_critical(ku)) {
> +		warnx("%s: RFC 6487, section 4.8.4: KeyUsage not critical", fn);
> +		goto out;
> +	}
> +
>  	/* This weird API can return 0, 1, 2, 4, 5 but can't error... */
>  	if ((is_ca = X509_check_ca(x)) > 1) {
>  		if (is_ca == 4)
> @@ -314,6 +330,19 @@ x509_get_purpose(X509 *x, const char *fn
>  			    "Constraint must be absent", fn);
>  			goto out;
>  		}
> +
> +		if (X509_get_key_usage(x) != (KU_KEY_CERT_SIGN | KU_CRL_SIGN)) {
> +			warnx("%s: RFC 6487 section 4.8.4: key usage violation",
> +			    fn);
> +			goto out;
> +		}
> +
> +		if (X509_get_extended_key_usage(x) != UINT32_MAX) {
> +			warnx("%s: RFC 6487 section 4.8.5: EKU not allowed",
> +			    fn);
> +			goto out;
> +		}
> +
>  		/*
>  		 * EXFLAG_SI means that issuer and subject are identical.
>  		 * EXFLAG_SS is SI plus the AKI is absent or matches the SKI.
> @@ -332,6 +361,12 @@ x509_get_purpose(X509 *x, const char *fn
>  
>  	if ((ext_flags & EXFLAG_BCONS) != 0) {
>  		warnx("%s: Basic Constraints ext in non-CA cert", fn);
> +		goto out;
> +	}
> +
> +	if (X509_get_key_usage(x) != KU_DIGITAL_SIGNATURE) {
> +		warnx("%s: RFC 6487 section 4.8.4: KU must be digitalSignature",
> +		    fn);
>  		goto out;
>  	}
>  
> 

Diff is OK claudio@. I wonder if the name of the function is slowly
outdated since x509_get_purpose() does not a lot more then just selecting
the purpose. That's a different bikeshed and for a different diff.

-- 
:wq Claudio