diff mbox series

[v2,2/8] lib: crypto: add public_key_verify_signature()

Message ID 20200616052655.4845-3-takahiro.akashi@linaro.org
State Superseded
Delegated to: Heinrich Schuchardt
Headers show
Series efi_loader: secure boot: support intermediate certificates in signature | expand

Commit Message

AKASHI Takahiro June 16, 2020, 5:26 a.m. UTC
This function will be called from x509_check_for_self_signed() and
pkcs7_verify_one(), which will be imported from linux in a later patch.

While it does exist in linux code and has a similar functionality of
rsa_verify(), it calls further linux-specific interfaces inside.
That could lead to more files being imported from linux.

So simply re-implement it here instead of re-using the code.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/crypto/public_key.h |  2 +-
 lib/crypto/public_key.c     | 63 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 63 insertions(+), 2 deletions(-)

Comments

Heinrich Schuchardt July 7, 2020, 10:04 a.m. UTC | #1
On 16.06.20 07:26, AKASHI Takahiro wrote:
> This function will be called from x509_check_for_self_signed() and
> pkcs7_verify_one(), which will be imported from linux in a later patch.
>
> While it does exist in linux code and has a similar functionality of
> rsa_verify(), it calls further linux-specific interfaces inside.
> That could lead to more files being imported from linux.
>
> So simply re-implement it here instead of re-using the code.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/crypto/public_key.h |  2 +-
>  lib/crypto/public_key.c     | 63 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> index 436a1ee1ee64..3ba90fcc3483 100644
> --- a/include/crypto/public_key.h
> +++ b/include/crypto/public_key.h
> @@ -82,9 +82,9 @@ extern int decrypt_blob(struct kernel_pkey_params *, const void *, void *);
>  extern int create_signature(struct kernel_pkey_params *, const void *, void *);
>  extern int verify_signature(const struct key *,
>  			    const struct public_key_signature *);
> +#endif /* __UBOOT__ */
>
>  int public_key_verify_signature(const struct public_key *pkey,
>  				const struct public_key_signature *sig);
> -#endif /* !__UBOOT__ */
>
>  #endif /* _LINUX_PUBLIC_KEY_H */
> diff --git a/lib/crypto/public_key.c b/lib/crypto/public_key.c
> index e12ebbb3d0c5..013ea71a180f 100644
> --- a/lib/crypto/public_key.c
> +++ b/lib/crypto/public_key.c
> @@ -25,7 +25,10 @@
>  #include <keys/asymmetric-subtype.h>
>  #endif
>  #include <crypto/public_key.h>
> -#ifndef __UBOOT__
> +#ifdef __UBOOT__
> +#include <image.h>
> +#include <u-boot/rsa.h>
> +#else
>  #include <crypto/akcipher.h>
>  #endif
>
> @@ -80,6 +83,64 @@ void public_key_signature_free(struct public_key_signature *sig)
>  }
>  EXPORT_SYMBOL_GPL(public_key_signature_free);
>
> +/**
> + * public_key_verify_signature - Verify a signature using a public key.
> + *
> + * @pkey:	Public key
> + * @sig:	Signature
> + *
> + * Verify a signature, @sig, using a RSA public key, @pkey.
> + *
> + * Return:	0 - verified, non-zero error code - otherwise
> + */
> +int public_key_verify_signature(const struct public_key *pkey,
> +				const struct public_key_signature *sig)
> +{
> +	struct image_sign_info info;
> +	struct image_region region;
> +	int ret;
> +
> +	pr_devel("==>%s()\n", __func__);
> +
> +	if (!pkey || !sig)
> +		return -EINVAL;
> +
> +	if (pkey->key_is_private)
> +		return -EINVAL;
> +
> +	memset(&info, '\0', sizeof(info));
> +	info.padding = image_get_padding_algo("pkcs-1.5");
> +	/*
> +	 * Note: image_get_[checksum|crypto]_algo takes an string

nits

%/an string/a string/g

> +	 * argument like "<checksum>,<crypto>"
> +	 * TODO: support other hash algorithms
> +	 */
> +	if (!strcmp(sig->hash_algo, "sha1")) {

In our coding sig->key_algo can have values "rsa" or "ecrdsa". See
lib/crypto/x509_cert_parser.c:471. Please, check the value.

You can use sig->s_size * 8 to get the actual key size.

You should check the return value of image_get_checksum_algo().

if (!strcmp(sig->key_algo, "rsa")) {
	char algo[16];
	snprintf(algo, sizeof(algo), "%s,rsa%d", sig->hash_algo, 8 * sig->s_size);
	info.checksum = image_get_checksum_algo(algo);
}
if (!info.checksum)
	return -ENOPKG;
}

> +		info.checksum = image_get_checksum_algo("sha1,rsa2048");
> +		info.name = "sha1,rsa2048";
> +	} else if (!strcmp(sig->hash_algo, "sha256")) {
> +		info.checksum = image_get_checksum_algo("sha256,rsa2048");
> +		info.name = "sha256,rsa2048";
> +	} else {
> +		pr_warn("unknown msg digest algo: %s\n", sig->hash_algo);

Please, use log_warning().

> +		return -ENOPKG;
> +	}
> +	info.crypto = image_get_crypto_algo(info.name);
> +
> +	info.key = pkey->key;
> +	info.keylen = pkey->keylen;
> +
> +	region.data = sig->digest;
> +	region.size = sig->digest_size;
> +
> +	if (rsa_verify_with_pkey(&info, sig->digest, sig->s, sig->s_size))

Why warning above but no debug message here?

> +		ret = -EKEYREJECTED;
> +	else
> +		ret = 0;
> +
> +	pr_devel("<==%s() = %d\n", __func__, ret);

log_content()

Best regards

Heinrich


> +	return ret;
> +}
>  #else
>  /*
>   * Destroy a public key algorithm key.
>
AKASHI Takahiro July 8, 2020, 6:21 a.m. UTC | #2
Heinrich,

On Tue, Jul 07, 2020 at 12:04:29PM +0200, Heinrich Schuchardt wrote:
> On 16.06.20 07:26, AKASHI Takahiro wrote:
> > This function will be called from x509_check_for_self_signed() and
> > pkcs7_verify_one(), which will be imported from linux in a later patch.
> >
> > While it does exist in linux code and has a similar functionality of
> > rsa_verify(), it calls further linux-specific interfaces inside.
> > That could lead to more files being imported from linux.
> >
> > So simply re-implement it here instead of re-using the code.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  include/crypto/public_key.h |  2 +-
> >  lib/crypto/public_key.c     | 63 ++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 63 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> > index 436a1ee1ee64..3ba90fcc3483 100644
> > --- a/include/crypto/public_key.h
> > +++ b/include/crypto/public_key.h
> > @@ -82,9 +82,9 @@ extern int decrypt_blob(struct kernel_pkey_params *, const void *, void *);
> >  extern int create_signature(struct kernel_pkey_params *, const void *, void *);
> >  extern int verify_signature(const struct key *,
> >  			    const struct public_key_signature *);
> > +#endif /* __UBOOT__ */
> >
> >  int public_key_verify_signature(const struct public_key *pkey,
> >  				const struct public_key_signature *sig);
> > -#endif /* !__UBOOT__ */
> >
> >  #endif /* _LINUX_PUBLIC_KEY_H */
> > diff --git a/lib/crypto/public_key.c b/lib/crypto/public_key.c
> > index e12ebbb3d0c5..013ea71a180f 100644
> > --- a/lib/crypto/public_key.c
> > +++ b/lib/crypto/public_key.c
> > @@ -25,7 +25,10 @@
> >  #include <keys/asymmetric-subtype.h>
> >  #endif
> >  #include <crypto/public_key.h>
> > -#ifndef __UBOOT__
> > +#ifdef __UBOOT__
> > +#include <image.h>
> > +#include <u-boot/rsa.h>
> > +#else
> >  #include <crypto/akcipher.h>
> >  #endif
> >
> > @@ -80,6 +83,64 @@ void public_key_signature_free(struct public_key_signature *sig)
> >  }
> >  EXPORT_SYMBOL_GPL(public_key_signature_free);
> >
> > +/**
> > + * public_key_verify_signature - Verify a signature using a public key.
> > + *
> > + * @pkey:	Public key
> > + * @sig:	Signature
> > + *
> > + * Verify a signature, @sig, using a RSA public key, @pkey.
> > + *
> > + * Return:	0 - verified, non-zero error code - otherwise
> > + */
> > +int public_key_verify_signature(const struct public_key *pkey,
> > +				const struct public_key_signature *sig)
> > +{
> > +	struct image_sign_info info;
> > +	struct image_region region;
> > +	int ret;
> > +
> > +	pr_devel("==>%s()\n", __func__);
> > +
> > +	if (!pkey || !sig)
> > +		return -EINVAL;
> > +
> > +	if (pkey->key_is_private)
> > +		return -EINVAL;
> > +
> > +	memset(&info, '\0', sizeof(info));
> > +	info.padding = image_get_padding_algo("pkcs-1.5");
> > +	/*
> > +	 * Note: image_get_[checksum|crypto]_algo takes an string
> 
> nits
> 
> %/an string/a string/g

Fixed.

> > +	 * argument like "<checksum>,<crypto>"
> > +	 * TODO: support other hash algorithms
> > +	 */
> > +	if (!strcmp(sig->hash_algo, "sha1")) {
> 
> In our coding sig->key_algo can have values "rsa" or "ecrdsa". See
> lib/crypto/x509_cert_parser.c:471. Please, check the value.
> 
> You can use sig->s_size * 8 to get the actual key size.

I think that you are mentioning a check against encryption algo.
Yeah, I will add it although it will anyhow fail in rsa_verify_with_pkey().

> You should check the return value of image_get_checksum_algo().

I think it's nothing but an overhead as the arguments here
have "static" values, but will add it if you want.

> if (!strcmp(sig->key_algo, "rsa")) {
> 	char algo[16];
> 	snprintf(algo, sizeof(algo), "%s,rsa%d", sig->hash_algo, 8 * sig->s_size);
> 	info.checksum = image_get_checksum_algo(algo);
> }
> if (!info.checksum)
> 	return -ENOPKG;
> }
> 
> > +		info.checksum = image_get_checksum_algo("sha1,rsa2048");
> > +		info.name = "sha1,rsa2048";
> > +	} else if (!strcmp(sig->hash_algo, "sha256")) {
> > +		info.checksum = image_get_checksum_algo("sha256,rsa2048");
> > +		info.name = "sha256,rsa2048";
> > +	} else {
> > +		pr_warn("unknown msg digest algo: %s\n", sig->hash_algo);
> 
> Please, use log_warning().

No, I can't agree.
I prefer to call pr_xxx() very much more than log_xxx() as
this code is embedded in a file imported from linux.
Moreover, log_xxx() is seldom used even across U-Boot.

> > +		return -ENOPKG;
> > +	}
> > +	info.crypto = image_get_crypto_algo(info.name);
> > +
> > +	info.key = pkey->key;
> > +	info.keylen = pkey->keylen;
> > +
> > +	region.data = sig->digest;
> > +	region.size = sig->digest_size;
> > +
> > +	if (rsa_verify_with_pkey(&info, sig->digest, sig->s, sig->s_size))
> 
> Why warning above but no debug message here?

Because
* the reason of EKEYREJECTED failure is trivial in this function.
* rsa_verify_With_pkey() outputs a bunch of verbose and useful
  messages instead.


> > +		ret = -EKEYREJECTED;
> > +	else
> > +		ret = 0;
> > +
> > +	pr_devel("<==%s() = %d\n", __func__, ret);
> 
> log_content()

See my opinion & policy above.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> 
> > +	return ret;
> > +}
> >  #else
> >  /*
> >   * Destroy a public key algorithm key.
> >
>
diff mbox series

Patch

diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index 436a1ee1ee64..3ba90fcc3483 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -82,9 +82,9 @@  extern int decrypt_blob(struct kernel_pkey_params *, const void *, void *);
 extern int create_signature(struct kernel_pkey_params *, const void *, void *);
 extern int verify_signature(const struct key *,
 			    const struct public_key_signature *);
+#endif /* __UBOOT__ */
 
 int public_key_verify_signature(const struct public_key *pkey,
 				const struct public_key_signature *sig);
-#endif /* !__UBOOT__ */
 
 #endif /* _LINUX_PUBLIC_KEY_H */
diff --git a/lib/crypto/public_key.c b/lib/crypto/public_key.c
index e12ebbb3d0c5..013ea71a180f 100644
--- a/lib/crypto/public_key.c
+++ b/lib/crypto/public_key.c
@@ -25,7 +25,10 @@ 
 #include <keys/asymmetric-subtype.h>
 #endif
 #include <crypto/public_key.h>
-#ifndef __UBOOT__
+#ifdef __UBOOT__
+#include <image.h>
+#include <u-boot/rsa.h>
+#else
 #include <crypto/akcipher.h>
 #endif
 
@@ -80,6 +83,64 @@  void public_key_signature_free(struct public_key_signature *sig)
 }
 EXPORT_SYMBOL_GPL(public_key_signature_free);
 
+/**
+ * public_key_verify_signature - Verify a signature using a public key.
+ *
+ * @pkey:	Public key
+ * @sig:	Signature
+ *
+ * Verify a signature, @sig, using a RSA public key, @pkey.
+ *
+ * Return:	0 - verified, non-zero error code - otherwise
+ */
+int public_key_verify_signature(const struct public_key *pkey,
+				const struct public_key_signature *sig)
+{
+	struct image_sign_info info;
+	struct image_region region;
+	int ret;
+
+	pr_devel("==>%s()\n", __func__);
+
+	if (!pkey || !sig)
+		return -EINVAL;
+
+	if (pkey->key_is_private)
+		return -EINVAL;
+
+	memset(&info, '\0', sizeof(info));
+	info.padding = image_get_padding_algo("pkcs-1.5");
+	/*
+	 * Note: image_get_[checksum|crypto]_algo takes an string
+	 * argument like "<checksum>,<crypto>"
+	 * TODO: support other hash algorithms
+	 */
+	if (!strcmp(sig->hash_algo, "sha1")) {
+		info.checksum = image_get_checksum_algo("sha1,rsa2048");
+		info.name = "sha1,rsa2048";
+	} else if (!strcmp(sig->hash_algo, "sha256")) {
+		info.checksum = image_get_checksum_algo("sha256,rsa2048");
+		info.name = "sha256,rsa2048";
+	} else {
+		pr_warn("unknown msg digest algo: %s\n", sig->hash_algo);
+		return -ENOPKG;
+	}
+	info.crypto = image_get_crypto_algo(info.name);
+
+	info.key = pkey->key;
+	info.keylen = pkey->keylen;
+
+	region.data = sig->digest;
+	region.size = sig->digest_size;
+
+	if (rsa_verify_with_pkey(&info, sig->digest, sig->s, sig->s_size))
+		ret = -EKEYREJECTED;
+	else
+		ret = 0;
+
+	pr_devel("<==%s() = %d\n", __func__, ret);
+	return ret;
+}
 #else
 /*
  * Destroy a public key algorithm key.