Re: [PATCH 06/13] efi_loader: image_loader: verification for all signatures should pass

2020-06-01 Thread AKASHI Takahiro 
Heinrich,

On Sat, May 30, 2020 at 09:01:53AM +0200, Heinrich Schuchardt wrote:
> On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> > A signed image may have multiple signatures in
> >   - each WIN_CERTIFICATE in authenticode, and/or
> >   - each SignerInfo in pkcs7 SignedData (of WIN_CERTIFICATE)
> >
> > In the initial implementation of efi_image_authenticate(), the criteria
> > of verification check for multiple signatures case is a bit ambiguous
> > and it may cause inconsistent result.
> >
> > With this patch, we will make sure that verification check in
> > efi_image_authenticate() should pass against all the signatures.
> > The only exception would be
> >   - the case where a digest algorithm used in signature is not supported by
> > U-Boot, or
> >   - the case where parsing some portion of authenticode has failed
> > In those cases, we don't know how the signature be handled and should
> > just ignore them.
> >
> > Please note that, due to this change, efi_signature_verify_with_sigdb()'s
> > function prototype will be modified, taking "dbx" as well as "db"
> > instead of outputing a "certificate." If "dbx" is null, the behavior would
> > be the exact same as before.
> > The function's name will be changed to efi_signature_verify() once
> > current efi_signature_verify() has gone due to further improvement
> > in intermedaite certificates support.
> >
> > Signed-off-by: AKASHI Takahiro 
> > ---
> >  include/efi_loader.h  |  10 +-
> >  lib/efi_loader/efi_image_loader.c |  37 +++--
> >  lib/efi_loader/efi_signature.c| 266 ++
> >  3 files changed, 146 insertions(+), 167 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 9533df26dc9e..2cbd52e273d4 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -761,14 +761,12 @@ struct efi_signature_store {
> >  struct x509_certificate;
> >  struct pkcs7_message;
> >
> > -bool efi_signature_verify_cert(struct x509_certificate *cert,
> > -  struct efi_signature_store *dbx);
> > -bool efi_signature_verify_signers(struct pkcs7_message *msg,
> > - struct efi_signature_store *dbx);
> >  bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
> >  struct pkcs7_message *msg,
> > - struct efi_signature_store *db,
> > - struct x509_certificate **cert);
> > +struct efi_signature_store *db,
> > +struct efi_signature_store *dbx);
> > +bool efi_signature_check_signers(struct pkcs7_message *msg,
> > +struct efi_signature_store *dbx);
> >
> >  efi_status_t efi_image_region_add(struct efi_image_regions *regs,
> >   const void *start, const void *end,
> > diff --git a/lib/efi_loader/efi_image_loader.c 
> > b/lib/efi_loader/efi_image_loader.c
> > index 5cdaa93519e7..33ffb43f3886 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -492,11 +492,12 @@ static bool efi_image_authenticate(void *efi, size_t 
> > efi_size)
> > size_t wincerts_len;
> > struct pkcs7_message *msg = NULL;
> > struct efi_signature_store *db = NULL, *dbx = NULL;
> > -   struct x509_certificate *cert = NULL;
> > void *new_efi = NULL, *auth, *wincerts_end;
> > size_t new_efi_size, auth_size;
> > bool ret = false;
> >
> > +   debug("%s: Enter, %d\n", __func__, ret);
> > +
> > if (!efi_secure_boot_enabled())
> > return true;
> >
> > @@ -542,7 +543,17 @@ static bool efi_image_authenticate(void *efi, size_t 
> > efi_size)
> > goto err;
> > }
> >
> > -   /* go through WIN_CERTIFICATE list */
> > +   /*
> > +* go through WIN_CERTIFICATE list
> > +* NOTE:
> > +* We may have multiple signatures either as WIN_CERTIFICATE's
> > +* in PE header, or as pkcs7 SignerInfo's in SignedData.
> > +* So the verification policy here is:
> > +*   - Success if, at least, one of signatures is verified
> > +*   - unless
> > +*   any of signatures is rejected explicitly, or
> 
> If one signature is good and the others are rejected (e.g. due to the
> revocation list), why won't you be happy with the one good signature? Is
> this a requirement in the UEFI spec?

Actually, I don't know the right answer.
UEFI specification doesn't say anything here and as far as I correctly
understand EDK2 code, it behaves in the same way.
Since there is no conformance test available (in UEFI SCT), it is
difficult to confirm that.
# FYI, I have been requesting such test cases in SCT since last October:
  https://bugzilla.tianocore.org/show_bug.cgi?id=2230

I also think that applying more strict rules and then easing them
later would be much easier and safer than the reverse(?).

Thanks,
-Takahiro Akashi



> Best regards
> 
> Heinrich
> 
> > +*

Re: [PATCH 06/13] efi_loader: image_loader: verification for all signatures should pass

2020-05-30 Thread Heinrich Schuchardt 
On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> A signed image may have multiple signatures in
>   - each WIN_CERTIFICATE in authenticode, and/or
>   - each SignerInfo in pkcs7 SignedData (of WIN_CERTIFICATE)
>
> In the initial implementation of efi_image_authenticate(), the criteria
> of verification check for multiple signatures case is a bit ambiguous
> and it may cause inconsistent result.
>
> With this patch, we will make sure that verification check in
> efi_image_authenticate() should pass against all the signatures.
> The only exception would be
>   - the case where a digest algorithm used in signature is not supported by
> U-Boot, or
>   - the case where parsing some portion of authenticode has failed
> In those cases, we don't know how the signature be handled and should
> just ignore them.
>
> Please note that, due to this change, efi_signature_verify_with_sigdb()'s
> function prototype will be modified, taking "dbx" as well as "db"
> instead of outputing a "certificate." If "dbx" is null, the behavior would
> be the exact same as before.
> The function's name will be changed to efi_signature_verify() once
> current efi_signature_verify() has gone due to further improvement
> in intermedaite certificates support.
>
> Signed-off-by: AKASHI Takahiro 
> ---
>  include/efi_loader.h  |  10 +-
>  lib/efi_loader/efi_image_loader.c |  37 +++--
>  lib/efi_loader/efi_signature.c| 266 ++
>  3 files changed, 146 insertions(+), 167 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 9533df26dc9e..2cbd52e273d4 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -761,14 +761,12 @@ struct efi_signature_store {
>  struct x509_certificate;
>  struct pkcs7_message;
>
> -bool efi_signature_verify_cert(struct x509_certificate *cert,
> -struct efi_signature_store *dbx);
> -bool efi_signature_verify_signers(struct pkcs7_message *msg,
> -   struct efi_signature_store *dbx);
>  bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
>struct pkcs7_message *msg,
> -   struct efi_signature_store *db,
> -   struct x509_certificate **cert);
> +  struct efi_signature_store *db,
> +  struct efi_signature_store *dbx);
> +bool efi_signature_check_signers(struct pkcs7_message *msg,
> +  struct efi_signature_store *dbx);
>
>  efi_status_t efi_image_region_add(struct efi_image_regions *regs,
> const void *start, const void *end,
> diff --git a/lib/efi_loader/efi_image_loader.c 
> b/lib/efi_loader/efi_image_loader.c
> index 5cdaa93519e7..33ffb43f3886 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -492,11 +492,12 @@ static bool efi_image_authenticate(void *efi, size_t 
> efi_size)
>   size_t wincerts_len;
>   struct pkcs7_message *msg = NULL;
>   struct efi_signature_store *db = NULL, *dbx = NULL;
> - struct x509_certificate *cert = NULL;
>   void *new_efi = NULL, *auth, *wincerts_end;
>   size_t new_efi_size, auth_size;
>   bool ret = false;
>
> + debug("%s: Enter, %d\n", __func__, ret);
> +
>   if (!efi_secure_boot_enabled())
>   return true;
>
> @@ -542,7 +543,17 @@ static bool efi_image_authenticate(void *efi, size_t 
> efi_size)
>   goto err;
>   }
>
> - /* go through WIN_CERTIFICATE list */
> + /*
> +  * go through WIN_CERTIFICATE list
> +  * NOTE:
> +  * We may have multiple signatures either as WIN_CERTIFICATE's
> +  * in PE header, or as pkcs7 SignerInfo's in SignedData.
> +  * So the verification policy here is:
> +  *   - Success if, at least, one of signatures is verified
> +  *   - unless
> +  *   any of signatures is rejected explicitly, or

If one signature is good and the others are rejected (e.g. due to the
revocation list), why won't you be happy with the one good signature? Is
this a requirement in the UEFI spec?

Best regards

Heinrich

> +  *   none of digest algorithms are supported
> +  */
>   for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
>(void *)wincert < wincerts_end;
>wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
> @@ -596,37 +607,27 @@ static bool efi_image_authenticate(void *efi, size_t 
> efi_size)
>   goto err;
>   }
>
> - if (!efi_signature_verify_signers(msg, dbx)) {
> - debug("Signer was rejected by \"dbx\"\n");
> + if (!efi_signature_check_signers(msg, dbx)) {
> + debug("Signer(s) in \"dbx\"\n");
>   goto err;
> - } else {
> - ret = true;
>

[PATCH 06/13] efi_loader: image_loader: verification for all signatures should pass

2020-05-29 Thread AKASHI Takahiro 
A signed image may have multiple signatures in
  - each WIN_CERTIFICATE in authenticode, and/or
  - each SignerInfo in pkcs7 SignedData (of WIN_CERTIFICATE)

In the initial implementation of efi_image_authenticate(), the criteria
of verification check for multiple signatures case is a bit ambiguous
and it may cause inconsistent result.

With this patch, we will make sure that verification check in
efi_image_authenticate() should pass against all the signatures.
The only exception would be
  - the case where a digest algorithm used in signature is not supported by
U-Boot, or
  - the case where parsing some portion of authenticode has failed
In those cases, we don't know how the signature be handled and should
just ignore them.

Please note that, due to this change, efi_signature_verify_with_sigdb()'s
function prototype will be modified, taking "dbx" as well as "db"
instead of outputing a "certificate." If "dbx" is null, the behavior would
be the exact same as before.
The function's name will be changed to efi_signature_verify() once
current efi_signature_verify() has gone due to further improvement
in intermedaite certificates support.

Signed-off-by: AKASHI Takahiro 
---
 include/efi_loader.h  |  10 +-
 lib/efi_loader/efi_image_loader.c |  37 +++--
 lib/efi_loader/efi_signature.c| 266 ++
 3 files changed, 146 insertions(+), 167 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 9533df26dc9e..2cbd52e273d4 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -761,14 +761,12 @@ struct efi_signature_store {
 struct x509_certificate;
 struct pkcs7_message;
 
-bool efi_signature_verify_cert(struct x509_certificate *cert,
-  struct efi_signature_store *dbx);
-bool efi_signature_verify_signers(struct pkcs7_message *msg,
- struct efi_signature_store *dbx);
 bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
 struct pkcs7_message *msg,
- struct efi_signature_store *db,
- struct x509_certificate **cert);
+struct efi_signature_store *db,
+struct efi_signature_store *dbx);
+bool efi_signature_check_signers(struct pkcs7_message *msg,
+struct efi_signature_store *dbx);
 
 efi_status_t efi_image_region_add(struct efi_image_regions *regs,
  const void *start, const void *end,
diff --git a/lib/efi_loader/efi_image_loader.c 
b/lib/efi_loader/efi_image_loader.c
index 5cdaa93519e7..33ffb43f3886 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -492,11 +492,12 @@ static bool efi_image_authenticate(void *efi, size_t 
efi_size)
size_t wincerts_len;
struct pkcs7_message *msg = NULL;
struct efi_signature_store *db = NULL, *dbx = NULL;
-   struct x509_certificate *cert = NULL;
void *new_efi = NULL, *auth, *wincerts_end;
size_t new_efi_size, auth_size;
bool ret = false;
 
+   debug("%s: Enter, %d\n", __func__, ret);
+
if (!efi_secure_boot_enabled())
return true;
 
@@ -542,7 +543,17 @@ static bool efi_image_authenticate(void *efi, size_t 
efi_size)
goto err;
}
 
-   /* go through WIN_CERTIFICATE list */
+   /*
+* go through WIN_CERTIFICATE list
+* NOTE:
+* We may have multiple signatures either as WIN_CERTIFICATE's
+* in PE header, or as pkcs7 SignerInfo's in SignedData.
+* So the verification policy here is:
+*   - Success if, at least, one of signatures is verified
+*   - unless
+*   any of signatures is rejected explicitly, or
+*   none of digest algorithms are supported
+*/
for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
 (void *)wincert < wincerts_end;
 wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
@@ -596,37 +607,27 @@ static bool efi_image_authenticate(void *efi, size_t 
efi_size)
goto err;
}
 
-   if (!efi_signature_verify_signers(msg, dbx)) {
-   debug("Signer was rejected by \"dbx\"\n");
+   if (!efi_signature_check_signers(msg, dbx)) {
+   debug("Signer(s) in \"dbx\"\n");
goto err;
-   } else {
-   ret = true;
}
 
/* try white-list */
-   if (!efi_signature_verify_with_sigdb(regs, msg, db, )) {
-   debug("Verifying signature with \"db\" failed\n");
+   if (!efi_signature_verify_with_sigdb(regs, msg, db, dbx)) {
+   debug("Signature was not verified by \"db\"\n");