From 56fb385a172890cf4c20c9daa89bb6cccf9c1b4e Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Wed, 23 Oct 2013 10:50:36 -0400 Subject: [PATCH] Revert "additional bounds-checking on section sizes" This reverts commit 21e40f0174814b3d91836e38c7cf95c8f2f1f3a4. In principle I like the idea of what's going on here, but generate_hash() really does need to have the expected result. --- Makefile | 3 +- MokManager.c | 6 ++-- PasswordCrypt.c | 4 +-- shim.c | 86 ++++++++++++++++--------------------------------- 4 files changed, 33 insertions(+), 66 deletions(-) diff --git a/Makefile b/Makefile index af7642b..0fce466 100644 --- a/Makefile +++ b/Makefile @@ -16,8 +16,7 @@ EFI_LDS = elf_$(ARCH)_efi.lds DEFAULT_LOADER := \\\\grub.efi CFLAGS = -ggdb -O0 -fno-stack-protector -fno-strict-aliasing -fpic \ - -fshort-wchar -Wall -Wsign-compare -Werror \ - -mno-red-zone -maccumulate-outgoing-args \ + -fshort-wchar -Wall -Werror -mno-red-zone -maccumulate-outgoing-args \ -mno-mmx -mno-sse -fno-builtin \ "-DDEFAULT_LOADER=L\"$(DEFAULT_LOADER)\"" \ "-DDEFAULT_LOADER_CHAR=\"$(DEFAULT_LOADER)\"" \ diff --git a/MokManager.c b/MokManager.c index 3da61f4..f5ed379 100644 --- a/MokManager.c +++ b/MokManager.c @@ -440,7 +440,7 @@ static EFI_STATUS list_keys (void *KeyList, UINTN KeyListSize, CHAR16 *title) MokListNode *keys = NULL; INTN key_num = 0; CHAR16 **menu_strings; - unsigned int i; + int i; if (KeyListSize < (sizeof(EFI_SIGNATURE_LIST) + sizeof(EFI_SIGNATURE_DATA))) { @@ -491,7 +491,7 @@ static EFI_STATUS list_keys (void *KeyList, UINTN KeyListSize, CHAR16 *title) static UINT8 get_line (UINT32 *length, CHAR16 *line, UINT32 line_max, UINT8 show) { EFI_INPUT_KEY key; - unsigned int count = 0; + int count = 0; do { key = console_get_keystroke(); @@ -640,7 +640,7 @@ static EFI_STATUS match_password (PASSWORD_CRYPT *pw_crypt, CHAR16 password[PASSWORD_MAX]; UINT32 pw_length; UINT8 fail_count = 0; - unsigned int i; + int i; if (pw_crypt) { auth_hash = pw_crypt->hash; diff --git a/PasswordCrypt.c b/PasswordCrypt.c index e0a82cf..8d72a82 100644 --- a/PasswordCrypt.c +++ b/PasswordCrypt.c @@ -154,7 +154,7 @@ static EFI_STATUS sha256_crypt (const char *key, UINT32 key_len, CopyMem(cp, tmp_result, cnt); SHA256_Init(&alt_ctx); - for (cnt = 0; cnt < 16ul + alt_result[0]; ++cnt) + for (cnt = 0; cnt < 16 + alt_result[0]; ++cnt) SHA256_Update(&alt_ctx, salt, salt_size); SHA256_Final(tmp_result, &alt_ctx); @@ -242,7 +242,7 @@ static EFI_STATUS sha512_crypt (const char *key, UINT32 key_len, CopyMem(cp, tmp_result, cnt); SHA512_Init(&alt_ctx); - for (cnt = 0; cnt < 16ul + alt_result[0]; ++cnt) + for (cnt = 0; cnt < 16 + alt_result[0]; ++cnt) SHA512_Update(&alt_ctx, salt, salt_size); SHA512_Final(tmp_result, &alt_ctx); diff --git a/shim.c b/shim.c index 3d1febb..537177d 100644 --- a/shim.c +++ b/shim.c @@ -102,7 +102,7 @@ typedef struct { /* * Perform basic bounds checking of the intra-image pointers */ -static void *ImageAddress (void *image, unsigned int size, unsigned int address) +static void *ImageAddress (void *image, int size, unsigned int address) { if (address > size) return NULL; @@ -483,19 +483,18 @@ static BOOLEAN secure_mode (void) * Calculate the SHA1 and SHA256 hashes of a binary */ -static EFI_STATUS generate_hash (char *data, int datasize_in, +static EFI_STATUS generate_hash (char *data, int datasize, PE_COFF_LOADER_IMAGE_CONTEXT *context, UINT8 *sha256hash, UINT8 *sha1hash) { unsigned int sha256ctxsize, sha1ctxsize; - unsigned int size; + unsigned int size = datasize; void *sha256ctx = NULL, *sha1ctx = NULL; char *hashbase; unsigned int hashsize; unsigned int SumOfBytesHashed, SumOfSectionBytes; unsigned int index, pos; - unsigned int datasize; EFI_IMAGE_SECTION_HEADER *Section; EFI_IMAGE_SECTION_HEADER *SectionHeader = NULL; EFI_IMAGE_SECTION_HEADER *SectionCache; @@ -507,12 +506,6 @@ static EFI_STATUS generate_hash (char *data, int datasize_in, sha1ctxsize = Sha1GetContextSize(); sha1ctx = AllocatePool(sha1ctxsize); - if (datasize_in < 0) { - Print(L"Invalid data size\n"); - return EFI_INVALID_PARAMETER; - } - size = datasize = (unsigned int)datasize_in; - if (!sha256ctx || !sha1ctx) { Print(L"Unable to allocate memory for hash context\n"); return EFI_OUT_OF_RESOURCES; @@ -563,29 +556,22 @@ static EFI_STATUS generate_hash (char *data, int datasize_in, /* Sort sections */ SumOfBytesHashed = context->PEHdr->Pe32Plus.OptionalHeader.SizeOfHeaders; - /* Validate section locations and sizes */ - for (index = 0, SumOfSectionBytes = 0; index < context->PEHdr->Pe32.FileHeader.NumberOfSections; index++, SectionCache++) { - EFI_IMAGE_SECTION_HEADER *SectionPtr; + Section = (EFI_IMAGE_SECTION_HEADER *) ( + (char *)context->PEHdr + sizeof (UINT32) + + sizeof (EFI_IMAGE_FILE_HEADER) + + context->PEHdr->Pe32.FileHeader.SizeOfOptionalHeader + ); - /* Validate SectionPtr is within image */ - SectionPtr = ImageAddress(data, datasize, - sizeof (UINT32) + - sizeof (EFI_IMAGE_FILE_HEADER) + - context->PEHdr->Pe32.FileHeader.SizeOfOptionalHeader + - (index * sizeof(*SectionPtr))); - if (!SectionPtr) { - Print(L"Malformed section %d\n", index); - status = EFI_INVALID_PARAMETER; - goto done; - } - /* Validate section size is within image. */ - if (SectionPtr->SizeOfRawData > - datasize - SumOfBytesHashed - SumOfSectionBytes) { - Print(L"Malformed section %d size\n", index); - status = EFI_INVALID_PARAMETER; - goto done; - } - SumOfSectionBytes += SectionPtr->SizeOfRawData; + SectionCache = Section; + + for (index = 0, SumOfSectionBytes = 0; index < context->PEHdr->Pe32.FileHeader.NumberOfSections; index++, SectionCache++) { + SumOfSectionBytes += SectionCache->SizeOfRawData; + } + + if (SumOfSectionBytes >= datasize) { + Print(L"Malformed binary: %x %x\n", SumOfSectionBytes, size); + status = EFI_INVALID_PARAMETER; + goto done; } SectionHeader = (EFI_IMAGE_SECTION_HEADER *) AllocateZeroPool (sizeof (EFI_IMAGE_SECTION_HEADER) * context->PEHdr->Pe32.FileHeader.NumberOfSections); @@ -595,11 +581,6 @@ static EFI_STATUS generate_hash (char *data, int datasize_in, goto done; } - /* Already validated above */ - Section = ImageAddress(data, datasize, sizeof (UINT32) + - sizeof (EFI_IMAGE_FILE_HEADER) + - context->PEHdr->Pe32.FileHeader.SizeOfOptionalHeader); - /* Sort the section headers */ for (index = 0; index < context->PEHdr->Pe32.FileHeader.NumberOfSections; index++) { pos = index; @@ -617,8 +598,8 @@ static EFI_STATUS generate_hash (char *data, int datasize_in, if (Section->SizeOfRawData == 0) { continue; } - hashbase = ImageAddress(data, size, - Section->PointerToRawData); + hashbase = ImageAddress(data, size, Section->PointerToRawData); + hashsize = (unsigned int) Section->SizeOfRawData; if (!hashbase) { Print(L"Malformed section header\n"); @@ -626,15 +607,6 @@ static EFI_STATUS generate_hash (char *data, int datasize_in, goto done; } - /* Verify hashsize within image. */ - if (Section->SizeOfRawData > - datasize - Section->PointerToRawData) { - Print(L"Malformed section raw size %d\n", index); - status = EFI_INVALID_PARAMETER; - goto done; - } - hashsize = (unsigned int) Section->SizeOfRawData; - if (!(Sha256Update(sha256ctx, hashbase, hashsize)) || !(Sha1Update(sha1ctx, hashbase, hashsize))) { Print(L"Unable to generate hash\n"); @@ -645,10 +617,10 @@ static EFI_STATUS generate_hash (char *data, int datasize_in, } /* Hash all remaining data */ - if (datasize > SumOfBytesHashed) { + if (size > SumOfBytesHashed) { hashbase = data + SumOfBytesHashed; hashsize = (unsigned int)( - datasize - + size - context->PEHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY].Size - SumOfBytesHashed); @@ -878,8 +850,7 @@ static EFI_STATUS read_header(void *data, unsigned int datasize, return EFI_UNSUPPORTED; } - if ((unsigned long)((UINT8 *)context->SecDir - (UINT8 *)data) > - (datasize - sizeof(EFI_IMAGE_DATA_DIRECTORY))) { + if (((UINT8 *)context->SecDir - (UINT8 *)data) > (datasize - sizeof(EFI_IMAGE_DATA_DIRECTORY))) { Print(L"Invalid image\n"); return EFI_UNSUPPORTED; } @@ -899,8 +870,7 @@ static EFI_STATUS handle_image (void *data, unsigned int datasize, { EFI_STATUS efi_status; char *buffer; - int i; - unsigned int size; + int i, size; EFI_IMAGE_SECTION_HEADER *Section; char *base, *end; PE_COFF_LOADER_IMAGE_CONTEXT context; @@ -1077,8 +1047,7 @@ static EFI_STATUS generate_path(EFI_LOADED_IMAGE *li, CHAR16 *ImagePath, { EFI_DEVICE_PATH *devpath; EFI_HANDLE device; - unsigned int i; - int j, last = -1; + int i, j, last = -1; unsigned int pathlen = 0; EFI_STATUS efi_status = EFI_SUCCESS; CHAR16 *bootpath; @@ -1633,10 +1602,9 @@ EFI_STATUS set_second_stage (EFI_HANDLE image_handle) EFI_STATUS status; EFI_LOADED_IMAGE *li; CHAR16 *start = NULL, *c; - unsigned int i; - int remaining_size = 0; + int i, remaining_size = 0; CHAR16 *loader_str = NULL; - unsigned int loader_len = 0; + int loader_len = 0; second_stage = DEFAULT_LOADER; load_options = NULL;