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

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 */