[RFC PATCH] efi_loader: fix uefi secure boot with intermediate certs

[RFC PATCH] efi_loader: fix uefi secure boot with intermediate certs

[Ilias Apalodimas]

The general rule of accepting or rejecting an image is
 1. Is the sha256 of the image in dbx
 2. Is the image signed with a certificate that's found in db and
    not in dbx
 3. The image carries a cert which is signed by a cert in db (and
    not in dbx) and the image can be verified against the former
 4. Is the sha256 of the image in db

For example SHIM is signed by "CN=Microsoft Windows UEFI Driver Publisher",
which is issued by "CN=Microsoft Corporation UEFI CA 2011", which in it's
turn is issued by "CN=Microsoft Corporation Third Party Marketplace Root".
The latter is a self-signed CA certificate and with our current implementation
allows shim to execute if we insert it in db.

However it's the CA cert in the middle of the chain which usually ends up
in the system's db.  pkcs7_verify_one() might or might not return the root
certificate for a given chain.  But when verifying executables in UEFI,  the
trust anchor can be in the middle of the chain, as long as that certificate
is present in db.  Currently we only allow this check on self-signed
certificates,  so let's remove that check and allow all certs to try a
match an entry in db.

Open questions:
- Does this break any aspect of variable authentication since 
  efi_signature_verify() is used on those as well?

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/efi_signature.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index 1bd1fdc95fce..79ed077ae7dd 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -518,12 +518,11 @@ bool efi_signature_verify(struct efi_image_regions *regs,
 			goto out;
 
 		EFI_PRINT("Verifying last certificate in chain\n");
-		if (signer->self_signed) {
-			if (efi_lookup_certificate(signer, db))
-				if (efi_signature_check_revocation(sinfo,
-								   signer, dbx))
-					break;
-		} else if (efi_verify_certificate(signer, db, &root)) {
+		if (efi_lookup_certificate(signer, db))
+			if (efi_signature_check_revocation(sinfo, signer, dbx))
+				break;
+		if (!signer->self_signed &&
+		    efi_verify_certificate(signer, db, &root)) {
 			bool check;
 
 			check = efi_signature_check_revocation(sinfo, root,
-- 
2.32.0

Re: [RFC PATCH] efi_loader: fix uefi secure boot with intermediate certs

[Heinrich Schuchardt]

On 2/14/22 10:14, Ilias Apalodimas wrote:
> The general rule of accepting or rejecting an image is
>   1. Is the sha256 of the image in dbx
>   2. Is the image signed with a certificate that's found in db and
>      not in dbx
>   3. The image carries a cert which is signed by a cert in db (and
>      not in dbx) and the image can be verified against the former
>   4. Is the sha256 of the image in db
>
> For example SHIM is signed by "CN=Microsoft Windows UEFI Driver Publisher",
> which is issued by "CN=Microsoft Corporation UEFI CA 2011", which in it's
> turn is issued by "CN=Microsoft Corporation Third Party Marketplace Root".
> The latter is a self-signed CA certificate and with our current implementation
> allows shim to execute if we insert it in db.
>
> However it's the CA cert in the middle of the chain which usually ends up
> in the system's db.  pkcs7_verify_one() might or might not return the root
> certificate for a given chain.  But when verifying executables in UEFI,  the
> trust anchor can be in the middle of the chain, as long as that certificate
> is present in db.  Currently we only allow this check on self-signed
> certificates,  so let's remove that check and allow all certs to try a
> match an entry in db.
>
> Open questions:
> - Does this break any aspect of variable authentication since
>    efi_signature_verify() is used on those as well?
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   lib/efi_loader/efi_signature.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index 1bd1fdc95fce..79ed077ae7dd 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -518,12 +518,11 @@ bool efi_signature_verify(struct efi_image_regions *regs,
>   			goto out;
>
>   		EFI_PRINT("Verifying last certificate in chain\n");
> -		if (signer->self_signed) {
> -			if (efi_lookup_certificate(signer, db))
> -				if (efi_signature_check_revocation(sinfo,
> -								   signer, dbx))
> -					break;
> -		} else if (efi_verify_certificate(signer, db, &root)) {
> +		if (efi_lookup_certificate(signer, db))

Why should we only check dbx if the certificate is in db? Shouldn't we
always check dbx?

Best regards

Heinrich

> +			if (efi_signature_check_revocation(sinfo, signer, dbx))
> +				break;
> +		if (!signer->self_signed &&
> +		    efi_verify_certificate(signer, db, &root)) {
>   			bool check;
>
>   			check = efi_signature_check_revocation(sinfo, root,

Re: [RFC PATCH] efi_loader: fix uefi secure boot with intermediate certs

[Ilias Apalodimas]

On Mon, Feb 14, 2022 at 11:32:53AM +0100, Heinrich Schuchardt wrote:
> On 2/14/22 10:14, Ilias Apalodimas wrote:
> > The general rule of accepting or rejecting an image is
> >   1. Is the sha256 of the image in dbx
> >   2. Is the image signed with a certificate that's found in db and
> >      not in dbx
> >   3. The image carries a cert which is signed by a cert in db (and
> >      not in dbx) and the image can be verified against the former
> >   4. Is the sha256 of the image in db
> > 
> > For example SHIM is signed by "CN=Microsoft Windows UEFI Driver Publisher",
> > which is issued by "CN=Microsoft Corporation UEFI CA 2011", which in it's
> > turn is issued by "CN=Microsoft Corporation Third Party Marketplace Root".
> > The latter is a self-signed CA certificate and with our current implementation
> > allows shim to execute if we insert it in db.
> > 
> > However it's the CA cert in the middle of the chain which usually ends up
> > in the system's db.  pkcs7_verify_one() might or might not return the root
> > certificate for a given chain.  But when verifying executables in UEFI,  the
> > trust anchor can be in the middle of the chain, as long as that certificate
> > is present in db.  Currently we only allow this check on self-signed
> > certificates,  so let's remove that check and allow all certs to try a
> > match an entry in db.
> > 
> > Open questions:
> > - Does this break any aspect of variable authentication since
> >    efi_signature_verify() is used on those as well?
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   lib/efi_loader/efi_signature.c | 11 +++++------
> >   1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> > index 1bd1fdc95fce..79ed077ae7dd 100644
> > --- a/lib/efi_loader/efi_signature.c
> > +++ b/lib/efi_loader/efi_signature.c
> > @@ -518,12 +518,11 @@ bool efi_signature_verify(struct efi_image_regions *regs,
> >   			goto out;
> > 
> >   		EFI_PRINT("Verifying last certificate in chain\n");
> > -		if (signer->self_signed) {
> > -			if (efi_lookup_certificate(signer, db))
> > -				if (efi_signature_check_revocation(sinfo,
> > -								   signer, dbx))
> > -					break;
> > -		} else if (efi_verify_certificate(signer, db, &root)) {
> > +		if (efi_lookup_certificate(signer, db))
> 
> Why should we only check dbx if the certificate is in db? Shouldn't we
> always check dbx?
> 

'db' is a little misleading here...  We do check everything,  but the code could 
use a bit cleanup (not as part of this patchset though).  

efi_signature_verify() is called on efi_image_loader.c. The call chain is:

efi_signature_verify(...., dbx, NULL) -->
  efi_signature_check_signers() --> 
    efi_signature_verify(...., db, dbx)

The first call is with dbx only, so it checks that database and tries to
find if any of the x509 certificates (of the image or the signing chain) are denied. 
Next on efi_signature_check_signers() we check if the sha256 of a
certificate that directly signed the image in not on dbx. 
The last call will try to verify if the x509 is on db while it's sha256 is not
in dbx.  

I agree we can clean up the logic in the future,  but for now that should
coverr all the cases you have in mind?


Thanks
/Ilias

> Best regards
> 
> Heinrich
> 
> > +			if (efi_signature_check_revocation(sinfo, signer, dbx))
> > +				break;
> > +		if (!signer->self_signed &&
> > +		    efi_verify_certificate(signer, db, &root)) {
> >   			bool check;
> > 
> >   			check = efi_signature_check_revocation(sinfo, root,
> 

Re: [RFC PATCH] efi_loader: fix uefi secure boot with intermediate certs

[Heinrich Schuchardt]

On 2/14/22 10:14, Ilias Apalodimas wrote:
> The general rule of accepting or rejecting an image is
>   1. Is the sha256 of the image in dbx
>   2. Is the image signed with a certificate that's found in db and
>      not in dbx
>   3. The image carries a cert which is signed by a cert in db (and
>      not in dbx) and the image can be verified against the former
>   4. Is the sha256 of the image in db
>
> For example SHIM is signed by "CN=Microsoft Windows UEFI Driver Publisher",
> which is issued by "CN=Microsoft Corporation UEFI CA 2011", which in it's
> turn is issued by "CN=Microsoft Corporation Third Party Marketplace Root".
> The latter is a self-signed CA certificate and with our current implementation
> allows shim to execute if we insert it in db.
>
> However it's the CA cert in the middle of the chain which usually ends up
> in the system's db.  pkcs7_verify_one() might or might not return the root
> certificate for a given chain.  But when verifying executables in UEFI,  the
> trust anchor can be in the middle of the chain, as long as that certificate
> is present in db.  Currently we only allow this check on self-signed
> certificates,  so let's remove that check and allow all certs to try a
> match an entry in db.
>
> Open questions:
> - Does this break any aspect of variable authentication since
>    efi_signature_verify() is used on those as well?
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   lib/efi_loader/efi_signature.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index 1bd1fdc95fce..79ed077ae7dd 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -518,12 +518,11 @@ bool efi_signature_verify(struct efi_image_regions *regs,
>   			goto out;
>
>   		EFI_PRINT("Verifying last certificate in chain\n");
> -		if (signer->self_signed) {
> -			if (efi_lookup_certificate(signer, db))
> -				if (efi_signature_check_revocation(sinfo,
> -								   signer, dbx))
> -					break;
> -		} else if (efi_verify_certificate(signer, db, &root)) {
> +		if (efi_lookup_certificate(signer, db))
> +			if (efi_signature_check_revocation(sinfo, signer, dbx))

This line is after "EFI_PRINT("Verifying last certificate in chain\n");".

According to the UEFI 2.9 specification we have to check all
certificates used in the chain against dbx not only the last:

p. 1715

"B. Any entry with SignatureListType of EFI_CERT_X509_SHA256,
EFI_CERT_X509_SHA384, or EFI_CERT_X509_SHA512, with any SignatureData
which reflects the To-Be-Signed hash included in any certificate in the
signing chain of the signature being verified."

Best regards

Heinrich

> +				break;
> +		if (!signer->self_signed &&
> +		    efi_verify_certificate(signer, db, &root)) {
>   			bool check;
>
>   			check = efi_signature_check_revocation(sinfo, root,

Re: [RFC PATCH] efi_loader: fix uefi secure boot with intermediate certs

[Ilias Apalodimas]

On Sat, Feb 19, 2022 at 10:47:16AM +0100, Heinrich Schuchardt wrote:
> On 2/14/22 10:14, Ilias Apalodimas wrote:
> > The general rule of accepting or rejecting an image is
> >   1. Is the sha256 of the image in dbx
> >   2. Is the image signed with a certificate that's found in db and
> >      not in dbx
> >   3. The image carries a cert which is signed by a cert in db (and
> >      not in dbx) and the image can be verified against the former
> >   4. Is the sha256 of the image in db
> > 
> > For example SHIM is signed by "CN=Microsoft Windows UEFI Driver Publisher",
> > which is issued by "CN=Microsoft Corporation UEFI CA 2011", which in it's
> > turn is issued by "CN=Microsoft Corporation Third Party Marketplace Root".
> > The latter is a self-signed CA certificate and with our current implementation
> > allows shim to execute if we insert it in db.
> > 
> > However it's the CA cert in the middle of the chain which usually ends up
> > in the system's db.  pkcs7_verify_one() might or might not return the root
> > certificate for a given chain.  But when verifying executables in UEFI,  the
> > trust anchor can be in the middle of the chain, as long as that certificate
> > is present in db.  Currently we only allow this check on self-signed
> > certificates,  so let's remove that check and allow all certs to try a
> > match an entry in db.
> > 
> > Open questions:
> > - Does this break any aspect of variable authentication since
> >    efi_signature_verify() is used on those as well?
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   lib/efi_loader/efi_signature.c | 11 +++++------
> >   1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> > index 1bd1fdc95fce..79ed077ae7dd 100644
> > --- a/lib/efi_loader/efi_signature.c
> > +++ b/lib/efi_loader/efi_signature.c
> > @@ -518,12 +518,11 @@ bool efi_signature_verify(struct efi_image_regions *regs,
> >   			goto out;
> > 
> >   		EFI_PRINT("Verifying last certificate in chain\n");
> > -		if (signer->self_signed) {
> > -			if (efi_lookup_certificate(signer, db))
> > -				if (efi_signature_check_revocation(sinfo,
> > -								   signer, dbx))
> > -					break;
> > -		} else if (efi_verify_certificate(signer, db, &root)) {
> > +		if (efi_lookup_certificate(signer, db))
> > +			if (efi_signature_check_revocation(sinfo, signer, dbx))
> 
> This line is after "EFI_PRINT("Verifying last certificate in chain\n");".
> 
> According to the UEFI 2.9 specification we have to check all
> certificates used in the chain against dbx not only the last:
> 
> p. 1715
> 
> "B. Any entry with SignatureListType of EFI_CERT_X509_SHA256,
> EFI_CERT_X509_SHA384, or EFI_CERT_X509_SHA512, with any SignatureData
> which reflects the To-Be-Signed hash included in any certificate in the
> signing chain of the signature being verified."
> 
> Best regards

Yes but imho this should be part of a larger rework.  The problem is
pkcs7_verify_one() is trying to verify the binary with the certificate and
walk up the chain of trust.  Now that function might or might not return
the root certificate, but we don't check all the certificates in the chain
anyway.  This patch is trying to fix the fact that non self signed certs
in the middle of the signing chain are valid and can authenticate EFI
binaries.

Regards
/Ilias
> 
> Heinrich
> 
> > +				break;
> > +		if (!signer->self_signed &&
> > +		    efi_verify_certificate(signer, db, &root)) {
> >   			bool check;
> > 
> >   			check = efi_signature_check_revocation(sinfo, root,
>