diff mbox series

[02/13] efi_loader: image_loader: add a check against certificate type of authenticode

Message ID 20200529064130.28332-3-takahiro.akashi@linaro.org
State Superseded, archived
Delegated to: Heinrich Schuchardt
Headers show
Series efi_loader: rework/improve UEFI secure boot code | expand

Commit Message

AKASHI Takahiro May 29, 2020, 6:41 a.m. UTC 
UEFI specification requires that we shall support three type of
certificates of authenticode in PE image:
  WIN_CERT_TYPE_EFI_GUID with the guid, EFI_CERT_TYPE_PCKS7_GUID
  WIN_CERT_TYPE_PKCS_SIGNED_DATA
  WIN_CERT_TYPE_EFI_PKCS1_15

As EDK2 does, we will support the first two that are pkcs7 SignedData.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_image_loader.c | 55 ++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 12 deletions(-)

Comments

Heinrich Schuchardt May 29, 2020, 10:37 a.m. UTC | #1 
On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> UEFI specification requires that we shall support three type of
> certificates of authenticode in PE image:
>   WIN_CERT_TYPE_EFI_GUID with the guid, EFI_CERT_TYPE_PCKS7_GUID
>   WIN_CERT_TYPE_PKCS_SIGNED_DATA
>   WIN_CERT_TYPE_EFI_PKCS1_15
>
> As EDK2 does, we will support the first two that are pkcs7 SignedData.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_image_loader.c | 55 ++++++++++++++++++++++++-------
>  1 file changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index 7f35b1e58fe5..a13ba3692ec2 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -375,7 +375,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>  	}
>
>  	/* Return Certificates Table */
> -	if (authsz) {
> +	if (authsz > 0) {

authsz is unsigned. We should avoid superfluous comparisons with 0.

>  		if (len < authoff + authsz) {
>  			debug("%s: Size for auth too large: %u >= %zu\n",
>  			      __func__, authsz, len - authoff);
> @@ -480,8 +480,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  	struct pkcs7_message *msg = NULL;
>  	struct efi_signature_store *db = NULL, *dbx = NULL;
>  	struct x509_certificate *cert = NULL;
> -	void *new_efi = NULL;
> -	size_t new_efi_size;
> +	void *new_efi = NULL, *auth, *wincerts_end;
> +	size_t new_efi_size, auth_size;
>  	bool ret = false;
>
>  	if (!efi_secure_boot_enabled())
> @@ -530,20 +530,51 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  	}
>
>  	/* go through WIN_CERTIFICATE list */
> -	for (wincert = wincerts;
> -	     (void *)wincert < (void *)wincerts + wincerts_len;
> +	for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
> +	     (void *)wincert < wincerts_end;

Adding to (void *) is not defined in the C spec (though gcc treats it
according to your intention). Please, use (u8 *) if you want to add byte
lengths.

>  	     wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
> -		if (wincert->dwLength < sizeof(*wincert)) {
> -			debug("%s: dwLength too small: %u < %zu\n",
> -			      __func__, wincert->dwLength, sizeof(*wincert));
> -			goto err;
> +		if ((void *)wincert + sizeof(*wincert) >= wincerts_end)
> +			break;
> +
> +		if (wincert->dwLength <= sizeof(*wincert)) {
> +			debug("dwLength too small: %u < %zu\n",
> +			      wincert->dwLength, sizeof(*wincert));
> +			continue;
>  		}
> -		msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert),
> -					  wincert->dwLength - sizeof(*wincert));
> +
> +		debug("WIN_CERTIFICATE_TYPE: 0x%x\n",
> +		      wincert->wCertificateType);

Should this EFI_PRINT()?

Best regards

Heinrich

> +
> +		auth = (void *)wincert + sizeof(*wincert);
> +		auth_size = wincert->dwLength - sizeof(*wincert);
> +		if (wincert->wCertificateType == WIN_CERT_TYPE_EFI_GUID) {
> +			if (auth + sizeof(efi_guid_t) >= wincerts_end)
> +				break;
> +
> +			if (auth_size <= sizeof(efi_guid_t)) {
> +				debug("dwLength too small: %u < %zu\n",
> +				      wincert->dwLength, sizeof(*wincert));
> +				continue;
> +			}
> +			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
> +				debug("Certificate type not supported: %pUl\n",
> +				      auth);
> +				continue;
> +			}
> +
> +			auth += sizeof(efi_guid_t);
> +			auth_size -= sizeof(efi_guid_t);
> +		} else if (wincert->wCertificateType
> +				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> +			debug("Certificate type not supported\n");
> +			continue;
> +		}
> +
> +		msg = pkcs7_parse_message(auth, auth_size);
>  		if (IS_ERR(msg)) {
>  			debug("Parsing image's signature failed\n");
>  			msg = NULL;
> -			goto err;
> +			continue;
>  		}
>
>  		/* try black-list first */
>
AKASHI Takahiro June 2, 2020, 2:22 a.m. UTC | #2 
Heinrich,

On Fri, May 29, 2020 at 12:37:26PM +0200, Heinrich Schuchardt wrote:
> On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> > UEFI specification requires that we shall support three type of
> > certificates of authenticode in PE image:
> >   WIN_CERT_TYPE_EFI_GUID with the guid, EFI_CERT_TYPE_PCKS7_GUID
> >   WIN_CERT_TYPE_PKCS_SIGNED_DATA
> >   WIN_CERT_TYPE_EFI_PKCS1_15
> >
> > As EDK2 does, we will support the first two that are pkcs7 SignedData.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  lib/efi_loader/efi_image_loader.c | 55 ++++++++++++++++++++++++-------
> >  1 file changed, 43 insertions(+), 12 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > index 7f35b1e58fe5..a13ba3692ec2 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -375,7 +375,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> >  	}
> >
> >  	/* Return Certificates Table */
> > -	if (authsz) {
> > +	if (authsz > 0) {
> 
> authsz is unsigned. We should avoid superfluous comparisons with 0.

Okay. I was a bit confused here.

> >  		if (len < authoff + authsz) {
> >  			debug("%s: Size for auth too large: %u >= %zu\n",
> >  			      __func__, authsz, len - authoff);
> > @@ -480,8 +480,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  	struct pkcs7_message *msg = NULL;
> >  	struct efi_signature_store *db = NULL, *dbx = NULL;
> >  	struct x509_certificate *cert = NULL;
> > -	void *new_efi = NULL;
> > -	size_t new_efi_size;
> > +	void *new_efi = NULL, *auth, *wincerts_end;
> > +	size_t new_efi_size, auth_size;
> >  	bool ret = false;
> >
> >  	if (!efi_secure_boot_enabled())
> > @@ -530,20 +530,51 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  	}
> >
> >  	/* go through WIN_CERTIFICATE list */
> > -	for (wincert = wincerts;
> > -	     (void *)wincert < (void *)wincerts + wincerts_len;
> > +	for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
> > +	     (void *)wincert < wincerts_end;
> 
> Adding to (void *) is not defined in the C spec (though gcc treats it
> according to your intention). Please, use (u8 *) if you want to add byte
> lengths.

Okay.

> >  	     wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
> > -		if (wincert->dwLength < sizeof(*wincert)) {
> > -			debug("%s: dwLength too small: %u < %zu\n",
> > -			      __func__, wincert->dwLength, sizeof(*wincert));
> > -			goto err;
> > +		if ((void *)wincert + sizeof(*wincert) >= wincerts_end)
> > +			break;
> > +
> > +		if (wincert->dwLength <= sizeof(*wincert)) {
> > +			debug("dwLength too small: %u < %zu\n",
> > +			      wincert->dwLength, sizeof(*wincert));
> > +			continue;
> >  		}
> > -		msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert),
> > -					  wincert->dwLength - sizeof(*wincert));
> > +
> > +		debug("WIN_CERTIFICATE_TYPE: 0x%x\n",
> > +		      wincert->wCertificateType);
> 
> Should this EFI_PRINT()?

Okay, I will replace all.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +
> > +		auth = (void *)wincert + sizeof(*wincert);
> > +		auth_size = wincert->dwLength - sizeof(*wincert);
> > +		if (wincert->wCertificateType == WIN_CERT_TYPE_EFI_GUID) {
> > +			if (auth + sizeof(efi_guid_t) >= wincerts_end)
> > +				break;
> > +
> > +			if (auth_size <= sizeof(efi_guid_t)) {
> > +				debug("dwLength too small: %u < %zu\n",
> > +				      wincert->dwLength, sizeof(*wincert));
> > +				continue;
> > +			}
> > +			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
> > +				debug("Certificate type not supported: %pUl\n",
> > +				      auth);
> > +				continue;
> > +			}
> > +
> > +			auth += sizeof(efi_guid_t);
> > +			auth_size -= sizeof(efi_guid_t);
> > +		} else if (wincert->wCertificateType
> > +				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> > +			debug("Certificate type not supported\n");
> > +			continue;
> > +		}
> > +
> > +		msg = pkcs7_parse_message(auth, auth_size);
> >  		if (IS_ERR(msg)) {
> >  			debug("Parsing image's signature failed\n");
> >  			msg = NULL;
> > -			goto err;
> > +			continue;
> >  		}
> >
> >  		/* try black-list first */
> >
>
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index 7f35b1e58fe5..a13ba3692ec2 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -375,7 +375,7 @@  bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 	}
 
 	/* Return Certificates Table */
-	if (authsz) {
+	if (authsz > 0) {
 		if (len < authoff + authsz) {
 			debug("%s: Size for auth too large: %u >= %zu\n",
 			      __func__, authsz, len - authoff);
@@ -480,8 +480,8 @@  static bool efi_image_authenticate(void *efi, size_t efi_size)
 	struct pkcs7_message *msg = NULL;
 	struct efi_signature_store *db = NULL, *dbx = NULL;
 	struct x509_certificate *cert = NULL;
-	void *new_efi = NULL;
-	size_t new_efi_size;
+	void *new_efi = NULL, *auth, *wincerts_end;
+	size_t new_efi_size, auth_size;
 	bool ret = false;
 
 	if (!efi_secure_boot_enabled())
@@ -530,20 +530,51 @@  static bool efi_image_authenticate(void *efi, size_t efi_size)
 	}
 
 	/* go through WIN_CERTIFICATE list */
-	for (wincert = wincerts;
-	     (void *)wincert < (void *)wincerts + wincerts_len;
+	for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
+	     (void *)wincert < wincerts_end;
 	     wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
-		if (wincert->dwLength < sizeof(*wincert)) {
-			debug("%s: dwLength too small: %u < %zu\n",
-			      __func__, wincert->dwLength, sizeof(*wincert));
-			goto err;
+		if ((void *)wincert + sizeof(*wincert) >= wincerts_end)
+			break;
+
+		if (wincert->dwLength <= sizeof(*wincert)) {
+			debug("dwLength too small: %u < %zu\n",
+			      wincert->dwLength, sizeof(*wincert));
+			continue;
 		}
-		msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert),
-					  wincert->dwLength - sizeof(*wincert));
+
+		debug("WIN_CERTIFICATE_TYPE: 0x%x\n",
+		      wincert->wCertificateType);
+
+		auth = (void *)wincert + sizeof(*wincert);
+		auth_size = wincert->dwLength - sizeof(*wincert);
+		if (wincert->wCertificateType == WIN_CERT_TYPE_EFI_GUID) {
+			if (auth + sizeof(efi_guid_t) >= wincerts_end)
+				break;
+
+			if (auth_size <= sizeof(efi_guid_t)) {
+				debug("dwLength too small: %u < %zu\n",
+				      wincert->dwLength, sizeof(*wincert));
+				continue;
+			}
+			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
+				debug("Certificate type not supported: %pUl\n",
+				      auth);
+				continue;
+			}
+
+			auth += sizeof(efi_guid_t);
+			auth_size -= sizeof(efi_guid_t);
+		} else if (wincert->wCertificateType
+				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
+			debug("Certificate type not supported\n");
+			continue;
+		}
+
+		msg = pkcs7_parse_message(auth, auth_size);
 		if (IS_ERR(msg)) {
 			debug("Parsing image's signature failed\n");
 			msg = NULL;
-			goto err;
+			continue;
 		}
 
 		/* try black-list first */