From bd376c16b82385493e3971292d1495e4fa6c0a91 Mon Sep 17 00:00:00 2001 From: Dennis Tseng Date: Sun, 27 Oct 2024 22:34:04 +0800 Subject: [PATCH 1/2] shim: immediately stop if signature is matched. If an image signed by multiple keys matches any one of vendor certificates, the image will be considered valid and should stop checking the next signature. This PR immediately stops do-while() next signature checking. in verify_buffer_authenticode() when key matches. Signed-off-by: Dennis Tseng --- shim.c | 54 ++++++++++++++++++++++++++---------------------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/shim.c b/shim.c index 87202f7ff..04f56cedf 100644 --- a/shim.c +++ b/shim.c @@ -563,7 +563,7 @@ verify_buffer_authenticode (char *data, int datasize, PE_COFF_LOADER_IMAGE_CONTEXT *context, UINT8 *sha256hash, UINT8 *sha1hash) { - EFI_STATUS ret_efi_status; + EFI_STATUS efi_status; size_t size = datasize; size_t offset = 0; unsigned int i = 0; @@ -578,27 +578,27 @@ verify_buffer_authenticode (char *data, int datasize, */ drain_openssl_errors(); - ret_efi_status = generate_hash(data, datasize, context, sha256hash, sha1hash); - if (EFI_ERROR(ret_efi_status)) { - dprint(L"generate_hash: %r\n", ret_efi_status); + efi_status = generate_hash(data, datasize, context, sha256hash, sha1hash); + if (EFI_ERROR(efi_status)) { + dprint(L"generate_hash: %r\n", efi_status); PrintErrors(); ClearErrors(); - crypterr(ret_efi_status); - return ret_efi_status; + crypterr(efi_status); + return efi_status; } /* * Ensure that the binary isn't forbidden by hash */ drain_openssl_errors(); - ret_efi_status = check_denylist(NULL, sha256hash, sha1hash); - if (EFI_ERROR(ret_efi_status)) { + efi_status = check_denylist(NULL, sha256hash, sha1hash); + if (EFI_ERROR(efi_status)) { // perror(L"Binary is forbidden\n"); -// dprint(L"Binary is forbidden: %r\n", ret_efi_status); +// dprint(L"Binary is forbidden: %r\n", efi_status); PrintErrors(); ClearErrors(); - crypterr(ret_efi_status); - return ret_efi_status; + crypterr(efi_status); + return efi_status; } /* @@ -606,20 +606,20 @@ verify_buffer_authenticode (char *data, int datasize, * firmware databases */ drain_openssl_errors(); - ret_efi_status = check_allowlist(NULL, sha256hash, sha1hash); - if (EFI_ERROR(ret_efi_status)) { - LogError(L"check_allowlist(): %r\n", ret_efi_status); - dprint(L"check_allowlist: %r\n", ret_efi_status); - if (ret_efi_status != EFI_NOT_FOUND) { - dprint(L"check_allowlist(): %r\n", ret_efi_status); + efi_status = check_allowlist(NULL, sha256hash, sha1hash); + if (EFI_ERROR(efi_status)) { + LogError(L"check_allowlist(): %r\n", efi_status); + dprint(L"check_allowlist: %r\n", efi_status); + if (efi_status != EFI_NOT_FOUND) { + dprint(L"check_allowlist(): %r\n", efi_status); PrintErrors(); ClearErrors(); - crypterr(ret_efi_status); - return ret_efi_status; + crypterr(efi_status); + return efi_status; } } else { drain_openssl_errors(); - return ret_efi_status; + return efi_status; } if (context->SecDir->Size == 0) { @@ -634,7 +634,7 @@ verify_buffer_authenticode (char *data, int datasize, } offset = 0; - ret_efi_status = EFI_NOT_FOUND; + efi_status = EFI_NOT_FOUND; do { WIN_CERTIFICATE_EFI_PKCS *sig = NULL; size_t sz; @@ -669,8 +669,6 @@ verify_buffer_authenticode (char *data, int datasize, } if (sig->Hdr.wCertificateType == WIN_CERT_TYPE_PKCS_SIGNED_DATA) { - EFI_STATUS efi_status; - dprint(L"Attempting to verify signature %d:\n", i++); efi_status = verify_one_signature(sig, sha256hash, sha1hash); @@ -683,8 +681,8 @@ verify_buffer_authenticode (char *data, int datasize, * So don't clobber successes with security violation * here; that just means it isn't a success. */ - if (ret_efi_status != EFI_SUCCESS) - ret_efi_status = efi_status; + if (efi_status == EFI_SUCCESS) + break; } else { perror(L"Unsupported certificate type %x\n", sig->Hdr.wCertificateType); @@ -692,15 +690,15 @@ verify_buffer_authenticode (char *data, int datasize, offset = ALIGN_VALUE(offset + sz, 8); } while (offset < context->SecDir->Size); - if (ret_efi_status != EFI_SUCCESS) { + if (efi_status != EFI_SUCCESS) { dprint(L"Binary is not authorized\n"); PrintErrors(); ClearErrors(); crypterr(EFI_SECURITY_VIOLATION); - ret_efi_status = EFI_SECURITY_VIOLATION; + efi_status = EFI_SECURITY_VIOLATION; } drain_openssl_errors(); - return ret_efi_status; + return efi_status; } /* From 4a17e58da5d3aa9e72669e183f33f1ef31ac5e26 Mon Sep 17 00:00:00 2001 From: Dennis Tseng Date: Wed, 30 Oct 2024 16:59:05 +0800 Subject: [PATCH 2/2] Skip verification for multiple signatures once one of them is matched If a single signature is correctly verified for an image with multiple signatures, then the verify_one_signature() should be skipped to avoid cpu-consumption. In the mean time, it is wise to at least check the format of the remaining signatures to reduce the risk of corrupted or malformed signatures. --- shim.c | 74 ++++++++++++++++++++++++++++++---------------------------- 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/shim.c b/shim.c index 04f56cedf..f9ec32a30 100644 --- a/shim.c +++ b/shim.c @@ -563,7 +563,7 @@ verify_buffer_authenticode (char *data, int datasize, PE_COFF_LOADER_IMAGE_CONTEXT *context, UINT8 *sha256hash, UINT8 *sha1hash) { - EFI_STATUS efi_status; + EFI_STATUS ret_efi_status; size_t size = datasize; size_t offset = 0; unsigned int i = 0; @@ -578,27 +578,27 @@ verify_buffer_authenticode (char *data, int datasize, */ drain_openssl_errors(); - efi_status = generate_hash(data, datasize, context, sha256hash, sha1hash); - if (EFI_ERROR(efi_status)) { - dprint(L"generate_hash: %r\n", efi_status); + ret_efi_status = generate_hash(data, datasize, context, sha256hash, sha1hash); + if (EFI_ERROR(ret_efi_status)) { + dprint(L"generate_hash: %r\n", ret_efi_status); PrintErrors(); ClearErrors(); - crypterr(efi_status); - return efi_status; + crypterr(ret_efi_status); + return ret_efi_status; } /* * Ensure that the binary isn't forbidden by hash */ drain_openssl_errors(); - efi_status = check_denylist(NULL, sha256hash, sha1hash); - if (EFI_ERROR(efi_status)) { + ret_efi_status = check_denylist(NULL, sha256hash, sha1hash); + if (EFI_ERROR(ret_efi_status)) { // perror(L"Binary is forbidden\n"); -// dprint(L"Binary is forbidden: %r\n", efi_status); +// dprint(L"Binary is forbidden: %r\n", ret_efi_status); PrintErrors(); ClearErrors(); - crypterr(efi_status); - return efi_status; + crypterr(ret_efi_status); + return ret_efi_status; } /* @@ -606,20 +606,20 @@ verify_buffer_authenticode (char *data, int datasize, * firmware databases */ drain_openssl_errors(); - efi_status = check_allowlist(NULL, sha256hash, sha1hash); - if (EFI_ERROR(efi_status)) { - LogError(L"check_allowlist(): %r\n", efi_status); - dprint(L"check_allowlist: %r\n", efi_status); - if (efi_status != EFI_NOT_FOUND) { - dprint(L"check_allowlist(): %r\n", efi_status); + ret_efi_status = check_allowlist(NULL, sha256hash, sha1hash); + if (EFI_ERROR(ret_efi_status)) { + LogError(L"check_allowlist(): %r\n", ret_efi_status); + dprint(L"check_allowlist: %r\n", ret_efi_status); + if (ret_efi_status != EFI_NOT_FOUND) { + dprint(L"check_allowlist(): %r\n", ret_efi_status); PrintErrors(); ClearErrors(); - crypterr(efi_status); - return efi_status; + crypterr(ret_efi_status); + return ret_efi_status; } } else { drain_openssl_errors(); - return efi_status; + return ret_efi_status; } if (context->SecDir->Size == 0) { @@ -634,7 +634,7 @@ verify_buffer_authenticode (char *data, int datasize, } offset = 0; - efi_status = EFI_NOT_FOUND; + ret_efi_status = EFI_NOT_FOUND; do { WIN_CERTIFICATE_EFI_PKCS *sig = NULL; size_t sz; @@ -669,20 +669,22 @@ verify_buffer_authenticode (char *data, int datasize, } if (sig->Hdr.wCertificateType == WIN_CERT_TYPE_PKCS_SIGNED_DATA) { + EFI_STATUS efi_status; + dprint(L"Attempting to verify signature %d:\n", i++); - efi_status = verify_one_signature(sig, sha256hash, sha1hash); - - /* - * If we didn't get EFI_SECURITY_VIOLATION from - * checking the hashes above, then any dbx entries are - * for a certificate, not this individual binary. - * - * So don't clobber successes with security violation - * here; that just means it isn't a success. - */ - if (efi_status == EFI_SUCCESS) - break; + if (ret_efi_status != EFI_SUCCESS) { + efi_status = verify_one_signature(sig, sha256hash, sha1hash); + /* + * If we didn't get EFI_SECURITY_VIOLATION from + * checking the hashes above, then any dbx entries are + * for a certificate, not this individual binary. + * + * So don't clobber successes with security violation + * here; that just means it isn't a success. + */ + ret_efi_status = efi_status; + } } else { perror(L"Unsupported certificate type %x\n", sig->Hdr.wCertificateType); @@ -690,15 +692,15 @@ verify_buffer_authenticode (char *data, int datasize, offset = ALIGN_VALUE(offset + sz, 8); } while (offset < context->SecDir->Size); - if (efi_status != EFI_SUCCESS) { + if (ret_efi_status != EFI_SUCCESS) { dprint(L"Binary is not authorized\n"); PrintErrors(); ClearErrors(); crypterr(EFI_SECURITY_VIOLATION); - efi_status = EFI_SECURITY_VIOLATION; + ret_efi_status = EFI_SECURITY_VIOLATION; } drain_openssl_errors(); - return efi_status; + return ret_efi_status; } /*