| Message ID | 20200616052655.4845-4-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: secure boot: support intermediate certificates in signature | expand |
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@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. 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. > */ >
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@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, ®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. > > */ > > >
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; + + 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. */
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@linaro.org> --- lib/crypto/x509_cert_parser.c | 2 -- lib/crypto/x509_public_key.c | 33 +++++++++++++++++++++++++-------- 2 files changed, 25 insertions(+), 10 deletions(-)