diff mbox series

[6/8] lib: crypto: export and enhance pkcs7_verify_one()

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

Commit Message

AKASHI Takahiro June 9, 2020, 5:13 a.m. UTC 
The function, pkcs7_verify_one(), will be utilized to rework signature
verification logic aiming to support intermediate certificates in
"chain of trust."

To do that, its function interface is expanded, adding an extra argument
which is expected to return the last certificate in trusted chain.
Then, this last one must further be verified with signature database, db
and/or dbx.

Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
---
 include/crypto/pkcs7.h    |  9 ++++++++-
 lib/crypto/pkcs7_verify.c | 36 +++++++++++++++++++++++++++++++-----
 2 files changed, 39 insertions(+), 6 deletions(-)

Comments

Heinrich Schuchardt June 10, 2020, 9:08 p.m. UTC | #1 
On 6/9/20 7:13 AM, AKASHI Takahiro wrote:
> The function, pkcs7_verify_one(), will be utilized to rework signature
> verification logic aiming to support intermediate certificates in
> "chain of trust."
>
> To do that, its function interface is expanded, adding an extra argument
> which is expected to return the last certificate in trusted chain.
> Then, this last one must further be verified with signature database, db
> and/or dbx.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> ---
>  include/crypto/pkcs7.h    |  9 ++++++++-
>  lib/crypto/pkcs7_verify.c | 36 +++++++++++++++++++++++++++++++-----
>  2 files changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
> index 8f5c8a7ee3b9..ca35df29f6fb 100644
> --- a/include/crypto/pkcs7.h
> +++ b/include/crypto/pkcs7.h
> @@ -27,7 +27,14 @@ extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,
>  				  const void **_data, size_t *_datalen,
>  				  size_t *_headerlen);
>
> -#ifndef __UBOOT__
> +#ifdef __UBOOT__
> +struct pkcs7_signed_info;
> +struct x509_certificate;
> +
> +int pkcs7_verify_one(struct pkcs7_message *pkcs7,
> +		     struct pkcs7_signed_info *sinfo,
> +		     struct x509_certificate **signer);
> +#else
>  /*
>   * pkcs7_trust.c
>   */
> diff --git a/lib/crypto/pkcs7_verify.c b/lib/crypto/pkcs7_verify.c
> index 8b5cd7a443ae..220ce9f77cf0 100644
> --- a/lib/crypto/pkcs7_verify.c
> +++ b/lib/crypto/pkcs7_verify.c
> @@ -295,8 +295,14 @@ static int pkcs7_find_key(struct pkcs7_message *pkcs7,
>  /*
>   * Verify the internal certificate chain as best we can.
Hello Takahiro,

please, add a function description. See
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

>   */
> +#ifdef __UBOOT__
> +static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> +				  struct pkcs7_signed_info *sinfo,
> +				  struct x509_certificate **signer)
> +#else
>  static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
>  				  struct pkcs7_signed_info *sinfo)

I would prefer if you could change this to a single signature with the
argument 'signer' which is required below anyway.

Please, remove all #ifdef __UBOOT__.

Functions that are not always used can be marked as __maybe_unused if
this is required to avoid compiler warnings.

> +#endif
>  {
>  	struct public_key_signature *sig;
>  	struct x509_certificate *x509 = sinfo->signer, *p;
> @@ -305,6 +311,8 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
>
>  	kenter("");
>
> +	*signer = NULL;
> +
>  	for (p = pkcs7->certs; p; p = p->next)
>  		p->seen = false;
>
> @@ -322,6 +330,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
>  			for (p = sinfo->signer; p != x509; p = p->signer)
>  				p->blacklisted = true;
>  			pr_debug("- blacklisted\n");
> +#ifdef __UBOOT__
> +			*signer = x509;
> +#endif
>  			return 0;
>  		}
>
> @@ -347,6 +358,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
>  				goto unsupported_crypto_in_x509;
>  			x509->signer = x509;
>  			pr_debug("- self-signed\n");
> +#ifdef __UBOOT__
> +			*signer = x509;
> +#endif
>  			return 0;
>  		}
a>
> @@ -377,6 +391,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
>
>  		/* We didn't find the root of this chain */
>  		pr_debug("- top\n");
> +#ifdef __UBOOT__
> +		*signer = x509;
> +#endif
>  		return 0;
>
>  	found_issuer_check_skid:
> @@ -394,6 +411,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
>  		if (p->seen) {
>  			pr_warn("Sig %u: X.509 chain contains loop\n",
>  				sinfo->index);
> +#ifdef __UBOOT__
> +			*signer = p;
> +#endif
>  			return 0;
>  		}
>  		ret = public_key_verify_signature(p->pub, x509->sig);
> @@ -402,6 +422,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
>  		x509->signer = p;
>  		if (x509 == p) {
>  			pr_debug("- self-signed\n");
> +#ifdef __UBOOT__
> +			*signer = p;
> +#endif
>  			return 0;
>  		}
>  		x509 = p;
> @@ -423,11 +446,14 @@ unsupported_crypto_in_x509:
>  /*
>   * Verify one signed information block from a PKCS#7 message.

A proper function description is missing here.

>   */
> -#ifndef __UBOOT__
> -static
> -#endif
> +#ifdef __UBOOT__
>  int pkcs7_verify_one(struct pkcs7_message *pkcs7,
> -		     struct pkcs7_signed_info *sinfo)
> +		     struct pkcs7_signed_info *sinfo,
> +		     struct x509_certificate **signer)
> +#else
> +static int pkcs7_verify_one(struct pkcs7_message *pkcs7,
> +			    struct pkcs7_signed_info *sinfo)

Please, use a single function signature here too.

Best regards

Heinrich

> +#endif
>  {
>  	int ret;
>
> @@ -471,7 +497,7 @@ int pkcs7_verify_one(struct pkcs7_message *pkcs7,
>  	pr_devel("Verified signature %u\n", sinfo->index);
>
>  	/* Verify the internal certificate chain */
> -	return pkcs7_verify_sig_chain(pkcs7, sinfo);
> +	return pkcs7_verify_sig_chain(pkcs7, sinfo, signer);
>  }
>
>  #ifndef __UBOOT__
>
AKASHI Takahiro June 16, 2020, 1:23 a.m. UTC | #2 
Heinrich,

On Wed, Jun 10, 2020 at 11:08:18PM +0200, Heinrich Schuchardt wrote:
> On 6/9/20 7:13 AM, AKASHI Takahiro wrote:
> > The function, pkcs7_verify_one(), will be utilized to rework signature
> > verification logic aiming to support intermediate certificates in
> > "chain of trust."
> >
> > To do that, its function interface is expanded, adding an extra argument
> > which is expected to return the last certificate in trusted chain.
> > Then, this last one must further be verified with signature database, db
> > and/or dbx.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > ---
> >  include/crypto/pkcs7.h    |  9 ++++++++-
> >  lib/crypto/pkcs7_verify.c | 36 +++++++++++++++++++++++++++++++-----
> >  2 files changed, 39 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
> > index 8f5c8a7ee3b9..ca35df29f6fb 100644
> > --- a/include/crypto/pkcs7.h
> > +++ b/include/crypto/pkcs7.h
> > @@ -27,7 +27,14 @@ extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,
> >  				  const void **_data, size_t *_datalen,
> >  				  size_t *_headerlen);
> >
> > -#ifndef __UBOOT__
> > +#ifdef __UBOOT__
> > +struct pkcs7_signed_info;
> > +struct x509_certificate;
> > +
> > +int pkcs7_verify_one(struct pkcs7_message *pkcs7,
> > +		     struct pkcs7_signed_info *sinfo,
> > +		     struct x509_certificate **signer);
> > +#else
> >  /*
> >   * pkcs7_trust.c
> >   */
> > diff --git a/lib/crypto/pkcs7_verify.c b/lib/crypto/pkcs7_verify.c
> > index 8b5cd7a443ae..220ce9f77cf0 100644
> > --- a/lib/crypto/pkcs7_verify.c
> > +++ b/lib/crypto/pkcs7_verify.c
> > @@ -295,8 +295,14 @@ static int pkcs7_find_key(struct pkcs7_message *pkcs7,
> >  /*
> >   * Verify the internal certificate chain as best we can.
> Hello Takahiro,
> 
> please, add a function description. See
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

Sure

> >   */
> > +#ifdef __UBOOT__
> > +static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> > +				  struct pkcs7_signed_info *sinfo,
> > +				  struct x509_certificate **signer)
> > +#else
> >  static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >  				  struct pkcs7_signed_info *sinfo)
> 
> I would prefer if you could change this to a single signature with the
> argument 'signer' which is required below anyway.

I don't get your point here.

> Please, remove all #ifdef __UBOOT__.
> 
> Functions that are not always used can be marked as __maybe_unused if
> this is required to avoid compiler warnings.

The purpose of __UBOOT__ is to distinguish my changes from the original
code that was imported from linux. I believe that it would make
it much easier to catch up and apply the following commits in linux.
Since there is no explicit custodian for lib/crypto, it is crucial
to keep such a maintenance effort as simple as possible.

I adopted this style of import also in other files under lib/crypto.

Thanks,
-Takahiro Akashi

> > +#endif
> >  {
> >  	struct public_key_signature *sig;
> >  	struct x509_certificate *x509 = sinfo->signer, *p;
> > @@ -305,6 +311,8 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >
> >  	kenter("");
> >
> > +	*signer = NULL;
> > +
> >  	for (p = pkcs7->certs; p; p = p->next)
> >  		p->seen = false;
> >
> > @@ -322,6 +330,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >  			for (p = sinfo->signer; p != x509; p = p->signer)
> >  				p->blacklisted = true;
> >  			pr_debug("- blacklisted\n");
> > +#ifdef __UBOOT__
> > +			*signer = x509;
> > +#endif
> >  			return 0;
> >  		}
> >
> > @@ -347,6 +358,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >  				goto unsupported_crypto_in_x509;
> >  			x509->signer = x509;
> >  			pr_debug("- self-signed\n");
> > +#ifdef __UBOOT__
> > +			*signer = x509;
> > +#endif
> >  			return 0;
> >  		}
> a>
> > @@ -377,6 +391,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >
> >  		/* We didn't find the root of this chain */
> >  		pr_debug("- top\n");
> > +#ifdef __UBOOT__
> > +		*signer = x509;
> > +#endif
> >  		return 0;
> >
> >  	found_issuer_check_skid:
> > @@ -394,6 +411,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >  		if (p->seen) {
> >  			pr_warn("Sig %u: X.509 chain contains loop\n",
> >  				sinfo->index);
> > +#ifdef __UBOOT__
> > +			*signer = p;
> > +#endif
> >  			return 0;
> >  		}
> >  		ret = public_key_verify_signature(p->pub, x509->sig);
> > @@ -402,6 +422,9 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
> >  		x509->signer = p;
> >  		if (x509 == p) {
> >  			pr_debug("- self-signed\n");
> > +#ifdef __UBOOT__
> > +			*signer = p;
> > +#endif
> >  			return 0;
> >  		}
> >  		x509 = p;
> > @@ -423,11 +446,14 @@ unsupported_crypto_in_x509:
> >  /*
> >   * Verify one signed information block from a PKCS#7 message.
> 
> A proper function description is missing here.
> 
> >   */
> > -#ifndef __UBOOT__
> > -static
> > -#endif
> > +#ifdef __UBOOT__
> >  int pkcs7_verify_one(struct pkcs7_message *pkcs7,
> > -		     struct pkcs7_signed_info *sinfo)
> > +		     struct pkcs7_signed_info *sinfo,
> > +		     struct x509_certificate **signer)
> > +#else
> > +static int pkcs7_verify_one(struct pkcs7_message *pkcs7,
> > +			    struct pkcs7_signed_info *sinfo)
> 
> Please, use a single function signature here too.
> 
> Best regards
> 
> Heinrich
> 
> > +#endif
> >  {
> >  	int ret;
> >
> > @@ -471,7 +497,7 @@ int pkcs7_verify_one(struct pkcs7_message *pkcs7,
> >  	pr_devel("Verified signature %u\n", sinfo->index);
> >
> >  	/* Verify the internal certificate chain */
> > -	return pkcs7_verify_sig_chain(pkcs7, sinfo);
> > +	return pkcs7_verify_sig_chain(pkcs7, sinfo, signer);
> >  }
> >
> >  #ifndef __UBOOT__
> >
>
diff mbox series

Patch

diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
index 8f5c8a7ee3b9..ca35df29f6fb 100644
--- a/include/crypto/pkcs7.h
+++ b/include/crypto/pkcs7.h
@@ -27,7 +27,14 @@  extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,
 				  const void **_data, size_t *_datalen,
 				  size_t *_headerlen);
 
-#ifndef __UBOOT__
+#ifdef __UBOOT__
+struct pkcs7_signed_info;
+struct x509_certificate;
+
+int pkcs7_verify_one(struct pkcs7_message *pkcs7,
+		     struct pkcs7_signed_info *sinfo,
+		     struct x509_certificate **signer);
+#else
 /*
  * pkcs7_trust.c
  */
diff --git a/lib/crypto/pkcs7_verify.c b/lib/crypto/pkcs7_verify.c
index 8b5cd7a443ae..220ce9f77cf0 100644
--- a/lib/crypto/pkcs7_verify.c
+++ b/lib/crypto/pkcs7_verify.c
@@ -295,8 +295,14 @@  static int pkcs7_find_key(struct pkcs7_message *pkcs7,
 /*
  * Verify the internal certificate chain as best we can.
  */
+#ifdef __UBOOT__
+static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
+				  struct pkcs7_signed_info *sinfo,
+				  struct x509_certificate **signer)
+#else
 static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 				  struct pkcs7_signed_info *sinfo)
+#endif
 {
 	struct public_key_signature *sig;
 	struct x509_certificate *x509 = sinfo->signer, *p;
@@ -305,6 +311,8 @@  static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 
 	kenter("");
 
+	*signer = NULL;
+
 	for (p = pkcs7->certs; p; p = p->next)
 		p->seen = false;
 
@@ -322,6 +330,9 @@  static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 			for (p = sinfo->signer; p != x509; p = p->signer)
 				p->blacklisted = true;
 			pr_debug("- blacklisted\n");
+#ifdef __UBOOT__
+			*signer = x509;
+#endif
 			return 0;
 		}
 
@@ -347,6 +358,9 @@  static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 				goto unsupported_crypto_in_x509;
 			x509->signer = x509;
 			pr_debug("- self-signed\n");
+#ifdef __UBOOT__
+			*signer = x509;
+#endif
 			return 0;
 		}
 
@@ -377,6 +391,9 @@  static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 
 		/* We didn't find the root of this chain */
 		pr_debug("- top\n");
+#ifdef __UBOOT__
+		*signer = x509;
+#endif
 		return 0;
 
 	found_issuer_check_skid:
@@ -394,6 +411,9 @@  static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 		if (p->seen) {
 			pr_warn("Sig %u: X.509 chain contains loop\n",
 				sinfo->index);
+#ifdef __UBOOT__
+			*signer = p;
+#endif
 			return 0;
 		}
 		ret = public_key_verify_signature(p->pub, x509->sig);
@@ -402,6 +422,9 @@  static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 		x509->signer = p;
 		if (x509 == p) {
 			pr_debug("- self-signed\n");
+#ifdef __UBOOT__
+			*signer = p;
+#endif
 			return 0;
 		}
 		x509 = p;
@@ -423,11 +446,14 @@  unsupported_crypto_in_x509:
 /*
  * Verify one signed information block from a PKCS#7 message.
  */
-#ifndef __UBOOT__
-static
-#endif
+#ifdef __UBOOT__
 int pkcs7_verify_one(struct pkcs7_message *pkcs7,
-		     struct pkcs7_signed_info *sinfo)
+		     struct pkcs7_signed_info *sinfo,
+		     struct x509_certificate **signer)
+#else
+static int pkcs7_verify_one(struct pkcs7_message *pkcs7,
+			    struct pkcs7_signed_info *sinfo)
+#endif
 {
 	int ret;
 
@@ -471,7 +497,7 @@  int pkcs7_verify_one(struct pkcs7_message *pkcs7,
 	pr_devel("Verified signature %u\n", sinfo->index);
 
 	/* Verify the internal certificate chain */
-	return pkcs7_verify_sig_chain(pkcs7, sinfo);
+	return pkcs7_verify_sig_chain(pkcs7, sinfo, signer);
 }
 
 #ifndef __UBOOT__