From fecdfb1f136a85f6e0953350d749eac8e708ddf4 Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Oct 30 2013 20:45:13 +0000 Subject: Fix a FreePool(NULL) call on machines too old for SB --- diff --git a/0002-Don-t-reject-all-binaries-without-a-certificate-data.patch b/0002-Don-t-reject-all-binaries-without-a-certificate-data.patch new file mode 100644 index 0000000..220de58 --- /dev/null +++ b/0002-Don-t-reject-all-binaries-without-a-certificate-data.patch @@ -0,0 +1,146 @@ +From be73f6bd4f064015c9f12323e2fb2f51b8cdb631 Mon Sep 17 00:00:00 2001 +From: Peter Jones +Date: Tue, 22 Oct 2013 13:36:54 -0400 +Subject: [PATCH 2/5] Don't reject all binaries without a certificate database. + +If a binary isn't signed, but its hash is enrolled in db, it won't have +a certificate database. So in those cases, don't check it against +certificate databases in db/dbx/etc, but we don't need to reject it +outright. + +Signed-off-by: Peter Jones +--- + shim.c | 70 +++++++++++++++++++++++++++++++++++------------------------------- + 1 file changed, 37 insertions(+), 33 deletions(-) + +diff --git a/shim.c b/shim.c +index 58136db..3d1febb 100644 +--- a/shim.c ++++ b/shim.c +@@ -371,8 +371,8 @@ static EFI_STATUS check_blacklist (WIN_CERTIFICATE_EFI_PKCS *cert, + SHA1_DIGEST_SIZE, EFI_CERT_SHA1_GUID) == + DATA_FOUND) + return EFI_ACCESS_DENIED; +- if (check_db_cert_in_ram(dbx, vendor_dbx_size, cert, +- sha256hash) == DATA_FOUND) ++ if (cert && check_db_cert_in_ram(dbx, vendor_dbx_size, cert, ++ sha256hash) == DATA_FOUND) + return EFI_ACCESS_DENIED; + + if (check_db_hash(L"dbx", secure_var, sha256hash, SHA256_DIGEST_SIZE, +@@ -381,7 +381,8 @@ static EFI_STATUS check_blacklist (WIN_CERTIFICATE_EFI_PKCS *cert, + if (check_db_hash(L"dbx", secure_var, sha1hash, SHA1_DIGEST_SIZE, + EFI_CERT_SHA1_GUID) == DATA_FOUND) + return EFI_ACCESS_DENIED; +- if (check_db_cert(L"dbx", secure_var, cert, sha256hash) == DATA_FOUND) ++ if (cert && check_db_cert(L"dbx", secure_var, cert, sha256hash) == ++ DATA_FOUND) + return EFI_ACCESS_DENIED; + + return EFI_SUCCESS; +@@ -414,7 +415,8 @@ static EFI_STATUS check_whitelist (WIN_CERTIFICATE_EFI_PKCS *cert, + update_verification_method(VERIFIED_BY_HASH); + return EFI_SUCCESS; + } +- if (check_db_cert(L"db", secure_var, cert, sha256hash) == DATA_FOUND) { ++ if (cert && check_db_cert(L"db", secure_var, cert, sha256hash) ++ == DATA_FOUND) { + verification_method = VERIFIED_BY_CERT; + update_verification_method(VERIFIED_BY_CERT); + return EFI_SUCCESS; +@@ -427,7 +429,8 @@ static EFI_STATUS check_whitelist (WIN_CERTIFICATE_EFI_PKCS *cert, + update_verification_method(VERIFIED_BY_HASH); + return EFI_SUCCESS; + } +- if (check_db_cert(L"MokList", shim_var, cert, sha256hash) == DATA_FOUND) { ++ if (cert && check_db_cert(L"MokList", shim_var, cert, sha256hash) == ++ DATA_FOUND) { + verification_method = VERIFIED_BY_CERT; + update_verification_method(VERIFIED_BY_CERT); + return EFI_SUCCESS; +@@ -712,25 +715,24 @@ static EFI_STATUS verify_buffer (char *data, int datasize, + UINT8 sha256hash[SHA256_DIGEST_SIZE]; + UINT8 sha1hash[SHA1_DIGEST_SIZE]; + EFI_STATUS status = EFI_ACCESS_DENIED; +- WIN_CERTIFICATE_EFI_PKCS *cert; ++ WIN_CERTIFICATE_EFI_PKCS *cert = NULL; + unsigned int size = datasize; + +- if (context->SecDir->Size == 0) { +- Print(L"Empty security header\n"); +- return EFI_INVALID_PARAMETER; +- } +- +- cert = ImageAddress (data, size, context->SecDir->VirtualAddress); ++ if (context->SecDir->Size != 0) { ++ cert = ImageAddress (data, size, ++ context->SecDir->VirtualAddress); + +- if (!cert) { +- Print(L"Certificate located outside the image\n"); +- return EFI_INVALID_PARAMETER; +- } ++ if (!cert) { ++ Print(L"Certificate located outside the image\n"); ++ return EFI_INVALID_PARAMETER; ++ } + +- if (cert->Hdr.wCertificateType != WIN_CERT_TYPE_PKCS_SIGNED_DATA) { +- Print(L"Unsupported certificate type %x\n", +- cert->Hdr.wCertificateType); +- return EFI_UNSUPPORTED; ++ if (cert->Hdr.wCertificateType != ++ WIN_CERT_TYPE_PKCS_SIGNED_DATA) { ++ Print(L"Unsupported certificate type %x\n", ++ cert->Hdr.wCertificateType); ++ return EFI_UNSUPPORTED; ++ } + } + + status = generate_hash(data, datasize, context, sha256hash, sha1hash); +@@ -761,27 +763,29 @@ static EFI_STATUS verify_buffer (char *data, int datasize, + if (status == EFI_SUCCESS) + return status; + +- /* +- * Check against the shim build key +- */ +- if (AuthenticodeVerify(cert->CertData, ++ if (cert) { ++ /* ++ * Check against the shim build key ++ */ ++ if (AuthenticodeVerify(cert->CertData, + context->SecDir->Size - sizeof(cert->Hdr), + shim_cert, sizeof(shim_cert), sha256hash, + SHA256_DIGEST_SIZE)) { +- status = EFI_SUCCESS; +- return status; +- } ++ status = EFI_SUCCESS; ++ return status; ++ } + + +- /* +- * And finally, check against shim's built-in key +- */ +- if (AuthenticodeVerify(cert->CertData, ++ /* ++ * And finally, check against shim's built-in key ++ */ ++ if (AuthenticodeVerify(cert->CertData, + context->SecDir->Size - sizeof(cert->Hdr), + vendor_cert, vendor_cert_size, sha256hash, + SHA256_DIGEST_SIZE)) { +- status = EFI_SUCCESS; +- return status; ++ status = EFI_SUCCESS; ++ return status; ++ } + } + + status = EFI_ACCESS_DENIED; +-- +1.8.3.1 + diff --git a/0004-We-should-be-checking-both-mok-and-the-system-s-SB-s.patch b/0004-We-should-be-checking-both-mok-and-the-system-s-SB-s.patch new file mode 100644 index 0000000..617c34d --- /dev/null +++ b/0004-We-should-be-checking-both-mok-and-the-system-s-SB-s.patch @@ -0,0 +1,30 @@ +From 83b3a7cf6d4d4e91579864cfc75dadf2b7304da9 Mon Sep 17 00:00:00 2001 +From: Peter Jones +Date: Mon, 28 Oct 2013 10:41:03 -0400 +Subject: [PATCH 4/5] We should be checking both mok and the system's SB + settings + +When we call hook_system_services(), we're currently only checking mok's +setting. We should use secure_mode() instead so it'll check both. + +Signed-off-by: Peter Jones +--- + shim.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/shim.c b/shim.c +index 537177d..9d0d884 100644 +--- a/shim.c ++++ b/shim.c +@@ -1718,7 +1718,7 @@ EFI_STATUS efi_main (EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *passed_systab) + /* + * Tell the user that we're in insecure mode if necessary + */ +- if (insecure_mode) { ++ if (!secure_mode()) { + Print(L"Booting in insecure mode\n"); + uefi_call_wrapper(BS->Stall, 1, 2000000); + } else { +-- +1.8.3.1 + diff --git a/0005-Don-t-free-GetVariable-return-data-without-checking-.patch b/0005-Don-t-free-GetVariable-return-data-without-checking-.patch new file mode 100644 index 0000000..e342960 --- /dev/null +++ b/0005-Don-t-free-GetVariable-return-data-without-checking-.patch @@ -0,0 +1,54 @@ +From 556c445ea19fc257fe35ac1a67477e7352ba3fcd Mon Sep 17 00:00:00 2001 +From: Peter Jones +Date: Wed, 30 Oct 2013 16:36:01 -0400 +Subject: [PATCH 5/5] Don't free GetVariable() return data without checking the + status code. + +This breaks every machine from before Secure Boot was a thing. + +Signed-off-by: Peter Jones +--- + shim.c | 15 ++++++++++++--- + 1 file changed, 12 insertions(+), 3 deletions(-) + +diff --git a/shim.c b/shim.c +index 9d0d884..0081342 100644 +--- a/shim.c ++++ b/shim.c +@@ -456,21 +456,30 @@ static BOOLEAN secure_mode (void) + return FALSE; + + status = get_variable(L"SecureBoot", &Data, &len, global_var); ++ if (status != EFI_SUCCESS) { ++ if (verbose) ++ console_notify(L"Secure boot not enabled\n"); ++ return FALSE; ++ } + sb = *Data; + FreePool(Data); + +- /* FIXME - more paranoia here? */ +- if (status != EFI_SUCCESS || sb != 1) { ++ if (sb != 1) { + if (verbose) + console_notify(L"Secure boot not enabled\n"); + return FALSE; + } + + status = get_variable(L"SetupMode", &Data, &len, global_var); ++ if (status == EFI_SUCCESS) { ++ if (verbose) ++ console_notify(L"Platform is in setup mode\n"); ++ return FALSE; ++ } + setupmode = *Data; + FreePool(Data); + +- if (status == EFI_SUCCESS && setupmode == 1) { ++ if (setupmode == 1) { + if (verbose) + console_notify(L"Platform is in setup mode\n"); + return FALSE; +-- +1.8.3.1 + diff --git a/shim.spec b/shim.spec index 15657a6..866c3ca 100644 --- a/shim.spec +++ b/shim.spec @@ -1,6 +1,6 @@ Name: shim Version: 0.5 -Release: 1%{?dist} +Release: 2%{?dist} Summary: First-stage UEFI bootloader License: BSD @@ -22,6 +22,11 @@ Source2: https://github.com/lcp/mokutil/archive/mokutil-%{mokutilver}.tar.bz2 # woops. Source3: dbx.esl +Patch0001: 0002-Don-t-reject-all-binaries-without-a-certificate-data.patch +Patch0002: 0004-We-should-be-checking-both-mok-and-the-system-s-SB-s.patch +Patch0003: 0005-Don-t-free-GetVariable-return-data-without-checking-.patch + + BuildRequires: gnu-efi git openssl-devel openssl BuildRequires: pesign >= 0.106-1 BuildRequires: gnu-efi = 3.0u, gnu-efi-devel = 3.0u @@ -110,6 +115,9 @@ install -m 0644 MokManager.efi.debug $RPM_BUILD_ROOT/usr/lib/debug/%{_datadir}/s /usr/share/man/man1/mokutil.1.gz %changelog +* Wed Oct 30 2013 Peter Jones - 0.5-1 +- Fix a FreePool(NULL) call on machines too old for SB + * Fri Oct 04 2013 Peter Jones - 0.5-1 - Update to 0.5