From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Tue, 16 Jun 2020 10:23:03 +0900 Subject: [PATCH 6/8] lib: crypto: export and enhance pkcs7_verify_one() In-Reply-To: References: <20200609051401.18192-1-takahiro.akashi@linaro.org> <20200609051401.18192-7-takahiro.akashi@linaro.org> Message-ID: <20200616012303.GA18868@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 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 > > --- > > 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__ > > >