On Wed, Feb 14, 2024 at 09:24:34AM +0100, Theo Buehler wrote:
> > This is indeed much nicer. Should md use EVP_MAX_MD_SIZE? But then I
> > guess the code needs to pass a len param to X509_pubkey_digest() so
> > that the memcmp() can use it. Also why did you move the
> > 	if (os->length != SHA_DIGEST_LENGTH) {
> > check after X509_pubkey_digest()? It is a cheap check that is currently
> > done before.
> 
> I moved the check since it made it more explicit that the lengths in the
> memcmp() are the same. If we want to use EVP_MAX_MD_SIZE, I think the
> check should stay where I moved it, otherwise we have to check lengths
> twice, which seems a bit much.

I see.
 
> Using EVP_MAX_MD_SIZE and a length parameter is a bit annoying since we
> run into signedness issues due to API inconsistency.

These APIs are a pain. Why is the ASN1_OCTET_STRING lenght field a signed
int? (no answer needed)

I'm OK with either diff.
 
> Index: x509.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
> diff -u -p -r1.78 x509.c
> --- x509.c	13 Feb 2024 20:37:15 -0000	1.78
> +++ x509.c	14 Feb 2024 08:20:03 -0000
> @@ -191,7 +191,7 @@ out:
>  }
>  
>  /*
> - * Parse X509v3 subject key identifier (SKI), RFC 6487 section 4.8.2:
> + * Validate the X509v3 subject key identifier (SKI), RFC 6487 section 4.8.2:
>   * "The SKI is a SHA-1 hash of the value of the DER-encoded ASN.1 BIT STRING of
>   * the Subject Public Key, as described in Section 4.2.1.2 of RFC 5280."
>   * Returns the SKI formatted as hex string, or NULL if it couldn't be parsed.
> @@ -199,11 +199,10 @@ out:
>  int
>  x509_get_ski(X509 *x, const char *fn, char **ski)
>  {
> -	const unsigned char	*d, *spk;
>  	ASN1_OCTET_STRING	*os;
> -	X509_PUBKEY		*pubkey;
> -	unsigned char		 spkd[SHA_DIGEST_LENGTH];
> -	int			 crit, dsz, spkz, rc = 0;
> +	unsigned char		 md[EVP_MAX_MD_SIZE];
> +	unsigned int		 md_len = EVP_MAX_MD_SIZE;
> +	int			 crit, rc = 0;
>  
>  	*ski = NULL;
>  	os = X509_get_ext_d2i(x, NID_subject_key_identifier, &crit, NULL);
> @@ -221,36 +220,24 @@ x509_get_ski(X509 *x, const char *fn, ch
>  		goto out;
>  	}
>  
> -	d = os->data;
> -	dsz = os->length;
> -
> -	if (dsz != SHA_DIGEST_LENGTH) {
> -		warnx("%s: RFC 6487 section 4.8.2: SKI: "
> -		    "want %d bytes SHA1 hash, have %d bytes",
> -		    fn, SHA_DIGEST_LENGTH, dsz);
> -		goto out;
> -	}
> -
> -	if ((pubkey = X509_get_X509_PUBKEY(x)) == NULL) {
> -		warnx("%s: X509_get_X509_PUBKEY", fn);
> -		goto out;
> -	}
> -	if (!X509_PUBKEY_get0_param(NULL, &spk, &spkz, NULL, pubkey)) {
> -		warnx("%s: X509_PUBKEY_get0_param", fn);
> +	if (!X509_pubkey_digest(x, EVP_sha1(), md, &md_len)) {
> +		warnx("%s: X509_pubkey_digest", fn);
>  		goto out;
>  	}
>  
> -	if (!EVP_Digest(spk, spkz, spkd, NULL, EVP_sha1(), NULL)) {
> -		warnx("%s: EVP_Digest failed", fn);
> +	if (os->length < 0 || md_len != (size_t)os->length) {
> +		warnx("%s: RFC 6487 section 4.8.2: SKI: "
> +		    "want %d bytes SHA1 hash, have %d bytes",
> +		    fn, md_len, os->length);
>  		goto out;
>  	}
>  
> -	if (memcmp(spkd, d, dsz) != 0) {
> +	if (memcmp(os->data, md, md_len) != 0) {
>  		warnx("%s: SKI does not match SHA1 hash of SPK", fn);
>  		goto out;
>  	}
>  
> -	*ski = hex_encode(d, dsz);
> +	*ski = hex_encode(md, md_len);
>  	rc = 1;
>   out:
>  	ASN1_OCTET_STRING_free(os);
> 

-- 
:wq Claudio