[PATCH v2 3/8] lib: crypto: enable x509_check_for_self_signed()

[AKASHI Takahiro]

Heinrich,

On Tue, Jul 07, 2020 at 12:02:51PM +0200, Heinrich Schuchardt wrote:
> On 16.06.20 07:26, AKASHI Takahiro wrote:
> > When the file, x509_public_key.c, was imported from linux code in
> >     commit b4adf627d5b7 ("lib: crypto: add x509 parser"),
> > x509_check_for_self_signed() was commented out for simplicity.
> >
> > Now it need be enabled in order to make pkcs7_verify_one(), which will be
> > imported in a later patch, functional.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > ---
> >  lib/crypto/x509_cert_parser.c |  2 --
> >  lib/crypto/x509_public_key.c  | 33 +++++++++++++++++++++++++--------
> >  2 files changed, 25 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/crypto/x509_cert_parser.c b/lib/crypto/x509_cert_parser.c
> > index 5f984b9dfdae..eb24349460c2 100644
> > --- a/lib/crypto/x509_cert_parser.c
> > +++ b/lib/crypto/x509_cert_parser.c
> > @@ -142,12 +142,10 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
> >  	}
> >  	cert->id = kid;
> >
> > -#ifndef __UBOOT__
> >  	/* Detect self-signed certificates */
> >  	ret = x509_check_for_self_signed(cert);
> >  	if (ret < 0)
> >  		goto error_decode;
> > -#endif
> >
> >  	kfree(ctx);
> >  	return cert;
> > diff --git a/lib/crypto/x509_public_key.c b/lib/crypto/x509_public_key.c
> > index 571af9a0adf9..91810a864049 100644
> > --- a/lib/crypto/x509_public_key.c
> > +++ b/lib/crypto/x509_public_key.c
> > @@ -8,6 +8,7 @@
> >  #define pr_fmt(fmt) "X.509: "fmt
> >  #ifdef __UBOOT__
> >  #include <common.h>
> > +#include <image.h>
> >  #include <dm/devres.h>
> >  #include <linux/compat.h>
> >  #include <linux/err.h>
> > @@ -18,6 +19,7 @@
> >  #include <linux/kernel.h>
> >  #ifdef __UBOOT__
> >  #include <crypto/x509_parser.h>
> > +#include <u-boot/rsa-checksum.h>
> >  #else
> >  #include <linux/slab.h>
> >  #include <keys/asymmetric-subtype.h>
> > @@ -35,7 +37,9 @@
> >  int x509_get_sig_params(struct x509_certificate *cert)
> >  {
> >  	struct public_key_signature *sig = cert->sig;
> > -#ifndef __UBOOT__
> > +#ifdef __UBOOT__
> > +	struct image_region region;
> > +#else
> >  	struct crypto_shash *tfm;
> >  	struct shash_desc *desc;
> >  	size_t desc_size;
> > @@ -63,12 +67,25 @@ int x509_get_sig_params(struct x509_certificate *cert)
> >  	sig->s_size = cert->raw_sig_size;
> >
> >  #ifdef __UBOOT__
> > -	/*
> > -	 * Note:
> > -	 * This part (filling sig->digest) should be implemented if
> > -	 * x509_check_for_self_signed() is enabled x509_cert_parse().
> > -	 * Currently, this check won't affect UEFI secure boot.
> > -	 */
> > +	if (!sig->hash_algo)
> > +		return -ENOPKG;
> > +	if (!strcmp(sig->hash_algo, "sha256"))
> > +		sig->digest_size = SHA256_SUM_LEN;
> > +	else if (!strcmp(sig->hash_algo, "sha1"))
> > +		sig->digest_size = SHA1_SUM_LEN;
> > +	else
> > +		return -ENOPKG;
> 
> It would be preferable to call hash_lookup_algo() instead of hard coding
> hash sizes in multiple places.

Maybe it's a matter of taste, but I don't see any benefit of
calling hash_lookup_algo() here as we name an algo name
explicitly.
It's nothing but an overhead.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > +
> > +	sig->digest = calloc(1, sig->digest_size);
> > +	if (!sig->digest)
> > +		return -ENOMEM;
> > +
> > +	region.data = cert->tbs;
> > +	region.size = cert->tbs_size;
> > +	hash_calculate(sig->hash_algo, &region, 1, sig->digest);
> > +
> > +	/* TODO: is_hash_blacklisted()? */
> > +
> >  	ret = 0;
> >  #else
> >  	/* Allocate the hashing algorithm we're going to need and find out how
> > @@ -118,7 +135,6 @@ error:
> >  	return ret;
> >  }
> >
> > -#ifndef __UBOOT__
> >  /*
> >   * Check for self-signedness in an X.509 cert and if found, check the signature
> >   * immediately if we can.
> > @@ -175,6 +191,7 @@ not_self_signed:
> >  	return 0;
> >  }
> >
> > +#ifndef __UBOOT__
> >  /*
> >   * Attempt to parse a data blob for a key as an X509 certificate.
> >   */
> >
>