From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Wed, 8 Jul 2020 15:24:07 +0900 Subject: [PATCH v2 3/8] lib: crypto: enable x509_check_for_self_signed() In-Reply-To: References: <20200616052655.4845-1-takahiro.akashi@linaro.org> <20200616052655.4845-4-takahiro.akashi@linaro.org> Message-ID: <20200708062407.GB19234@laputa> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > --- > > 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 > > +#include > > #include > > #include > > #include > > @@ -18,6 +19,7 @@ > > #include > > #ifdef __UBOOT__ > > #include > > +#include > > #else > > #include > > #include > > @@ -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, ®ion, 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. > > */ > > >