diff mbox series

[v4,1/7] lib: crypto: add public_key_verify_signature()

Message ID 20200717071630.7363-2-takahiro.akashi@linaro.org
State Superseded
Headers show
Series efi_loader: secure boot: support intermediate certificates in signature | expand

Commit Message

AKASHI Takahiro July 17, 2020, 7:16 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     | 70 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 70 insertions(+), 2 deletions(-)

-- 
2.27.0

Comments

Heinrich Schuchardt July 19, 2020, 8:20 a.m. UTC | #1 
On 7/17/20 9:16 AM, 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     | 70 ++++++++++++++++++++++++++++++++++++-

>  2 files changed, 70 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..71a0e0356beb 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


Shouldn't we describe in the header of the file that the code is taken
(at least partially) from Linux' crypto/asymmetric_keys/public_key.c?

Please, also state the Linux version number.

>

> @@ -80,6 +83,71 @@ 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 a string

> +	 * argument like "<checksum>,<crypto>"

> +	 * TODO: support other hash algorithms

> +	 */

> +	if (strcmp(sig->pkey_algo, "rsa") || (sig->s_size * 8) != 2048) {

> +		pr_warn("Encryption is not RSA2048: %s%d\n",

> +			sig->pkey_algo, sig->s_size * 8);

> +		return -ENOPKG;

> +	}

> +	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);

> +	if (unlikely(IS_ERR(info.checksum) || IS_ERR(info.crypto)))

> +		return -ENOPKG;


scripts/checkpatch.pl complains:

WARNING: nested (un)?likely() calls, IS_ERR already uses unlikely()
internally
#104: FILE: lib/crypto/public_key.c:134:
+       if (unlikely(IS_ERR(info.checksum) || IS_ERR(info.crypto)))

I cannot find this unlikely in the Linux v5.8-rc4
crypto/asymmetric_keys/public_key.c file. From where did you copy this code?

Best regards

Heinrich

> +

> +	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.

>
AKASHI Takahiro July 20, 2020, 2:51 a.m. UTC | #2 
Heinrich,

On Sun, Jul 19, 2020 at 10:20:58AM +0200, Heinrich Schuchardt wrote:
> On 7/17/20 9:16 AM, 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     | 70 ++++++++++++++++++++++++++++++++++++-

> >  2 files changed, 70 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..71a0e0356beb 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

> 

> Shouldn't we describe in the header of the file that the code is taken

> (at least partially) from Linux' crypto/asymmetric_keys/public_key.c?

> 

> Please, also state the Linux version number.


Please take a look at the commit c4e961ecec99 ("lib: crypto: add public
key utility") as this file itself was imported in v2020-01. 

While I prefer to add such an information in a commit message,
we should have an explicit rule for inclusion from outer sources
for consistency over the tree if you want to see such an information
in a file.

> >

> > @@ -80,6 +83,71 @@ 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 a string

> > +	 * argument like "<checksum>,<crypto>"

> > +	 * TODO: support other hash algorithms

> > +	 */

> > +	if (strcmp(sig->pkey_algo, "rsa") || (sig->s_size * 8) != 2048) {

> > +		pr_warn("Encryption is not RSA2048: %s%d\n",

> > +			sig->pkey_algo, sig->s_size * 8);

> > +		return -ENOPKG;

> > +	}

> > +	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);

> > +	if (unlikely(IS_ERR(info.checksum) || IS_ERR(info.crypto)))

> > +		return -ENOPKG;

> 

> scripts/checkpatch.pl complains:

> 

> WARNING: nested (un)?likely() calls, IS_ERR already uses unlikely()

> internally

> #104: FILE: lib/crypto/public_key.c:134:

> +       if (unlikely(IS_ERR(info.checksum) || IS_ERR(info.crypto)))


Probably I missed it out when checking.

> I cannot find this unlikely in the Linux v5.8-rc4

> crypto/asymmetric_keys/public_key.c file. From where did you copy this code?


As I stated in the commit message, the whole code of this function
was written from the scratch rather than by re-using linux code.

-Takahiro Akashi


> Best regards

> 

> Heinrich

> 

> > +

> > +	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.

> >

>
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..71a0e0356beb 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,71 @@  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 a string
+	 * argument like "<checksum>,<crypto>"
+	 * TODO: support other hash algorithms
+	 */
+	if (strcmp(sig->pkey_algo, "rsa") || (sig->s_size * 8) != 2048) {
+		pr_warn("Encryption is not RSA2048: %s%d\n",
+			sig->pkey_algo, sig->s_size * 8);
+		return -ENOPKG;
+	}
+	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);
+	if (unlikely(IS_ERR(info.checksum) || IS_ERR(info.crypto)))
+		return -ENOPKG;
+
+	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.