From 65c4e517813470f5ab20bb0886f08f69c6291ca6 Mon Sep 17 00:00:00 2001 From: Thore Sommer Date: Thu, 22 Aug 2024 14:21:51 +0200 Subject: [PATCH 1/2] better error message when shim self validation fails This exports the component name and found and expected SBAT level from verify_sbat_section for us to display to the user why validation failed. Further we now differentiate between errors regarding .sbat section related errors and SBAT revocations. Signed-off-by: Thore Sommer --- include/pe.h | 2 +- include/sbat.h | 8 +++++--- pe.c | 4 ++-- sbat.c | 41 ++++++++++++++++++++++++++++------------- shim.c | 35 +++++++++++++++++++++++++++++++++-- test-sbat.c | 23 +++++++++++++++++++++-- 6 files changed, 90 insertions(+), 23 deletions(-) diff --git a/include/pe.h b/include/pe.h index 9ea9eb446..581b4e65d 100644 --- a/include/pe.h +++ b/include/pe.h @@ -19,7 +19,7 @@ EFI_STATUS verify_image(void *data, unsigned int datasize, PE_COFF_LOADER_IMAGE_CONTEXT *context); EFI_STATUS -verify_sbat_section(char *SBATBase, size_t SBATSize); +verify_sbat_section(char *SBATBase, size_t SBATSize, UINT16 *sbat_gen_expected, UINT16 *sbat_gen_found, CHAR8 **sbat_component_name); EFI_STATUS get_section_vma (UINTN section_num, diff --git a/include/sbat.h b/include/sbat.h index bb523e7e0..b740c9342 100644 --- a/include/sbat.h +++ b/include/sbat.h @@ -77,12 +77,14 @@ parse_sbat_section(char *section_base, size_t section_size, size_t *n, struct sbat_section_entry ***entriesp); void cleanup_sbat_section_entries(size_t n, struct sbat_section_entry **entries); -EFI_STATUS verify_sbat(size_t n, struct sbat_section_entry **entries); +EFI_STATUS verify_sbat(size_t n, struct sbat_section_entry **entries, UINT16 *sbat_gen_expected, UINT16 *sbat_gen_found, CHAR8 **sbat_component_name); #ifdef SHIM_UNIT_TEST EFI_STATUS parse_sbat_var_data(list_t *entries, UINT8 *data, UINTN datasize); -EFI_STATUS verify_sbat_helper(list_t *sbat_var, size_t n, - struct sbat_section_entry **entries); +EFI_STATUS verify_sbat_helper(list_t *local_sbat_var, size_t n, + struct sbat_section_entry **entries, + UINT16 *sbat_gen_expected, UINT16 *sbat_gen_found, + CHAR8 **sbat_component_name); #endif /* !SHIM_UNIT_TEST */ #endif /* !SBAT_H_ */ // vim:fenc=utf-8:tw=75:noet diff --git a/pe.c b/pe.c index 330560171..24490ae88 100644 --- a/pe.c +++ b/pe.c @@ -333,7 +333,7 @@ generate_hash(char *data, unsigned int datasize, } EFI_STATUS -verify_sbat_section(char *SBATBase, size_t SBATSize) +verify_sbat_section(char *SBATBase, size_t SBATSize, UINT16 *sbat_gen_expected, UINT16 *sbat_gen_found, CHAR8 **sbat_component_name) { unsigned int i; EFI_STATUS efi_status; @@ -385,7 +385,7 @@ verify_sbat_section(char *SBATBase, size_t SBATSize) entries[i]->vendor_url); } - efi_status = verify_sbat(n, entries); + efi_status = verify_sbat(n, entries, sbat_gen_expected, sbat_gen_found, sbat_component_name); cleanup_sbat_section_entries(n, entries); diff --git a/sbat.c b/sbat.c index 06956122d..8ede4a7fa 100644 --- a/sbat.c +++ b/sbat.c @@ -131,10 +131,8 @@ cleanup_sbat_section_entries(size_t n, struct sbat_section_entry **entries) } EFI_STATUS -verify_single_entry(struct sbat_section_entry *entry, struct sbat_var_entry *sbat_var_entry, bool *found) +verify_single_entry(struct sbat_section_entry *entry, struct sbat_var_entry *sbat_var_entry, bool *found, UINT16 *sbat_gen, UINT16 *sbat_var_gen) { - UINT16 sbat_gen, sbat_var_gen; - if (strcmp((const char *)entry->component_name, (const char *)sbat_var_entry->component_name) == 0) { dprint(L"component %a has a matching SBAT variable entry, verifying\n", entry->component_name); @@ -144,12 +142,12 @@ verify_single_entry(struct sbat_section_entry *entry, struct sbat_var_entry *sba * atoi returns zero for failed conversion, so essentially * badly parsed component_generation will be treated as zero */ - sbat_gen = atoi((const char *)entry->component_generation); - sbat_var_gen = atoi((const char *)sbat_var_entry->component_generation); + *sbat_gen = atoi((const char *)entry->component_generation); + *sbat_var_gen = atoi((const char *)sbat_var_entry->component_generation); - if (sbat_gen < sbat_var_gen) { + if (*sbat_gen < *sbat_var_gen) { dprint(L"component %a, generation %d, was revoked by %s variable\n", - entry->component_name, sbat_gen, SBAT_VAR_NAME); + entry->component_name, *sbat_gen, SBAT_VAR_NAME); LogError(L"image did not pass SBAT verification\n"); return EFI_SECURITY_VIOLATION; } @@ -177,9 +175,10 @@ cleanup_sbat_var(list_t *entries) } EFI_STATUS -verify_sbat_helper(list_t *local_sbat_var, size_t n, struct sbat_section_entry **entries) +verify_sbat_helper(list_t *local_sbat_var, size_t n, struct sbat_section_entry **entries, UINT16 *sbat_gen_expected, UINT16 *sbat_gen_found, CHAR8 **sbat_component_name) { unsigned int i; + UINTN component_name_size; list_t *pos = NULL; EFI_STATUS efi_status = EFI_SUCCESS; struct sbat_var_entry *sbat_var_entry; @@ -192,10 +191,22 @@ verify_sbat_helper(list_t *local_sbat_var, size_t n, struct sbat_section_entry * for (i = 0; i < n; i++) { list_for_each(pos, local_sbat_var) { bool found = false; + *sbat_gen_expected = 0; + *sbat_gen_found = 0; sbat_var_entry = list_entry(pos, struct sbat_var_entry, list); - efi_status = verify_single_entry(entries[i], sbat_var_entry, &found); - if (EFI_ERROR(efi_status)) + efi_status = verify_single_entry(entries[i], sbat_var_entry, &found, sbat_gen_found, sbat_gen_expected); + if (EFI_ERROR(efi_status)) { + if (found && efi_status == EFI_SECURITY_VIOLATION) { + component_name_size = strlen((const char *)entries[i]->component_name); + *sbat_component_name = AllocatePool(component_name_size + 1); + if (!*sbat_component_name) { + efi_status = EFI_OUT_OF_RESOURCES; + goto err; + } + memcpy(*sbat_component_name, (const char *)entries[i]->component_name, component_name_size + 1); + } goto out; + } if (found) break; } @@ -203,15 +214,16 @@ verify_sbat_helper(list_t *local_sbat_var, size_t n, struct sbat_section_entry * out: dprint(L"finished verifying SBAT data: %r\n", efi_status); +err: return efi_status; } EFI_STATUS -verify_sbat(size_t n, struct sbat_section_entry **entries) +verify_sbat(size_t n, struct sbat_section_entry **entries, UINT16 *sbat_gen_expected, UINT16 *sbat_gen_found, CHAR8 **sbat_component_name) { EFI_STATUS efi_status; - efi_status = verify_sbat_helper(&sbat_var, n, entries); + efi_status = verify_sbat_helper(&sbat_var, n, entries, sbat_gen_expected, sbat_gen_found, sbat_component_name); return efi_status; } @@ -559,9 +571,12 @@ set_sbat_uefi_variable(char *sbat_var_automatic, char *sbat_var_latest) } #ifndef SHIM_UNIT_TEST + UINT16 sbat_gen_expected = 0; + UINT16 sbat_gen_found = 0; + CHAR8 *sbat_component_name = NULL; char *sbat_start = (char *)&_sbat; char *sbat_end = (char *)&_esbat; - efi_status = verify_sbat_section(sbat_start, sbat_end - sbat_start - 1); + efi_status = verify_sbat_section(sbat_start, sbat_end - sbat_start - 1, &sbat_gen_expected, &sbat_gen_found, &sbat_component_name); if (EFI_ERROR(efi_status)) { CHAR16 *title = L"New SbatLevel would self-revoke current shim. Not applied"; CHAR16 *message = L"Press any key to continue"; diff --git a/shim.c b/shim.c index 8d5ff589a..b5d5ec42e 100644 --- a/shim.c +++ b/shim.c @@ -710,6 +710,10 @@ verify_buffer_sbat (char *data, int datasize, { int i; EFI_IMAGE_SECTION_HEADER *Section; + EFI_STATUS efi_status; + UINT16 sbat_gen_expected = 0; + UINT16 sbat_gen_found = 0; + CHAR8 *sbat_component_name = NULL; char *SBATBase = NULL; size_t SBATSize = 0; @@ -757,7 +761,11 @@ verify_buffer_sbat (char *data, int datasize, } } - return verify_sbat_section(SBATBase, SBATSize); + efi_status = verify_sbat_section(SBATBase, SBATSize, &sbat_gen_expected, &sbat_gen_found, &sbat_component_name); + if (sbat_component_name) { + FreePool(sbat_component_name); + } + return efi_status; } /* @@ -1840,6 +1848,9 @@ efi_main (EFI_HANDLE passed_image_handle, EFI_SYSTEM_TABLE *passed_systab) { EFI_STATUS efi_status; EFI_HANDLE image_handle; + UINT16 sbat_gen_expected = 0; + UINT16 sbat_gen_found = 0; + CHAR8 *sbat_component_name = NULL; verification_method = VERIFIED_BY_NOTHING; @@ -1926,7 +1937,7 @@ efi_main (EFI_HANDLE passed_image_handle, EFI_SYSTEM_TABLE *passed_systab) goto die; } - efi_status = verify_sbat_section(sbat_start, sbat_end - sbat_start - 1); + efi_status = verify_sbat_section(sbat_start, sbat_end - sbat_start - 1, &sbat_gen_expected, &sbat_gen_found, &sbat_component_name); if (EFI_ERROR(efi_status)) { perror(L"Verifying shim SBAT data failed: %r\n", efi_status); @@ -1965,6 +1976,23 @@ efi_main (EFI_HANDLE passed_image_handle, EFI_SYSTEM_TABLE *passed_systab) die: console_print(L"Something has gone seriously wrong: %s: %r\n", msgs[msg], efi_status); + + /* + * Provide additional information when the SBAT self check fails + */ + if ( msg == SBAT_SELF_CHECK ) { + if (sbat_gen_expected == 0 && sbat_gen_found == 0 && sbat_component_name == NULL) { + // In this case something related to the .sbat section is wrong. + console_print(L"\n\nSomething went wrong validating SBAT. Either:\n" + "- The shim has no .sbat section or is corrupted\n" + "- Something went wrong internally"); + } else { + console_print(L"\n\nYour shim has been revoked via SBAT. " + "Please update to a newer shim version.\n" + "For component \"%a\" expected at least level: %u. Current shim has: %u.\n", + sbat_component_name, sbat_gen_expected, sbat_gen_found); + } + } #if defined(ENABLE_SHIM_DEVEL) devel_egress(COLD_RESET); #else @@ -1973,6 +2001,9 @@ efi_main (EFI_HANDLE passed_image_handle, EFI_SYSTEM_TABLE *passed_systab) 0, NULL); #endif } + if (sbat_component_name) { + FreePool(sbat_component_name); + } /* * This variable is supposed to be set by second stages, so ensure it is diff --git a/test-sbat.c b/test-sbat.c index b37efcddf..875c32b04 100644 --- a/test-sbat.c +++ b/test-sbat.c @@ -468,10 +468,19 @@ test_verify_sbat_null_sbat_section(void) status = parse_sbat_var_data(&test_sbat_var, sbat_var_data, sizeof(sbat_var_data)); assert_equal_goto(status, EFI_SUCCESS, err, "got %#x expected %#x\n"); - status = verify_sbat_helper(&test_sbat_var, n, entries); + UINT16 sbat_gen_expected = 0; + UINT16 sbat_gen_found = 0; + CHAR8 *sbat_component_name = NULL; + status = verify_sbat_helper(&test_sbat_var, n, entries, &sbat_gen_expected, &sbat_gen_found, &sbat_component_name); + assert_equal_goto(sbat_component_name, NULL, err, "got %#x expected %#x\n"); + assert_equal_goto(sbat_gen_expected, 0, err, "got %#x expected %#x\n"); + assert_equal_goto(sbat_gen_found, 0, err, "got %#x expected %#x\n"); assert_equal_goto(status, EFI_SUCCESS, err, "got %#x expected %#x\n"); rc = 0; err: + if (sbat_component_name) { + free(sbat_component_name); + } cleanup_sbat_var(&test_sbat_var); return rc; @@ -973,12 +982,22 @@ test_parse_and_verify(void) if (status != EFI_SUCCESS || list_empty(&sbat_var)) return -1; - status = verify_sbat(n_section_entries, section_entries); + UINT16 sbat_gen_expected = 0; + UINT16 sbat_gen_found = 0; + CHAR8 *sbat_component_name = NULL; + status = verify_sbat(n_section_entries, section_entries, &sbat_gen_expected, &sbat_gen_found, &sbat_component_name); + assert_nonzero_goto(sbat_component_name, err, "not expected component name to be NULL"); + assert_goto(strcmp(sbat_component_name, "test1") == 0, err, "expected 'test1' got '%s'", sbat_component_name); + assert_equal_goto(sbat_gen_expected, 5, err, "expected %#x got %#x\n"); + assert_equal_goto(sbat_gen_found, 1, err, "expected %#x got %#x\n"); assert_equal_goto(status, EFI_SECURITY_VIOLATION, err, "expected %#x got %#x\n"); rc = 0; err: cleanup_sbat_section_entries(n_section_entries, section_entries); + if (sbat_component_name) { + free(sbat_component_name); + } cleanup_sbat_var(&sbat_var); return rc; From c9dccb28feef8218590fd656c7452c12a40dc019 Mon Sep 17 00:00:00 2001 From: Thore Sommer Date: Thu, 22 Aug 2024 14:28:23 +0200 Subject: [PATCH 2/2] shim.c: set shutdown timemout to 90 seconds in error case This allows the user more time to read the error messages. Signed-off-by: Thore Sommer --- shim.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shim.c b/shim.c index b5d5ec42e..ef9570cd9 100644 --- a/shim.c +++ b/shim.c @@ -1996,7 +1996,7 @@ efi_main (EFI_HANDLE passed_image_handle, EFI_SYSTEM_TABLE *passed_systab) #if defined(ENABLE_SHIM_DEVEL) devel_egress(COLD_RESET); #else - usleep(5000000); + usleep(90000000); // 90 sec RT->ResetSystem(EfiResetShutdown, EFI_SECURITY_VIOLATION, 0, NULL); #endif