From 5495694c043de510aaf8ff5dcbe17b6547794083 Mon Sep 17 00:00:00 2001
From: Kees Cook <kees@outflux.net>
Date: Mon, 3 Dec 2012 15:52:48 -0800
Subject: [PATCH 23/74] additional bounds-checking on section sizes
This adds additional bounds-checking on the section sizes. Also adds
-Wsign-compare to the Makefile and replaces some signed variables with
unsigned counteparts for robustness.
Signed-off-by: Kees Cook <kees@ubuntu.com>
---
Makefile | 3 ++-
MokManager.c | 6 ++---
PasswordCrypt.c | 4 +--
fallback.c | 4 +--
shim.c | 83 +++++++++++++++++++++++++++++++++++++++------------------
5 files changed, 66 insertions(+), 34 deletions(-)
diff --git a/Makefile b/Makefile
index e65d28d..46e5ef9 100644
--- a/Makefile
+++ b/Makefile
@@ -16,7 +16,8 @@ EFI_LDS = elf_$(ARCH)_efi.lds
DEFAULT_LOADER := \\\\grub.efi
CFLAGS = -ggdb -O0 -fno-stack-protector -fno-strict-aliasing -fpic \
- -fshort-wchar -Wall -Werror -mno-red-zone -maccumulate-outgoing-args \
+ -fshort-wchar -Wall -Wsign-compare -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 f5ed379..3da61f4 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;
- int i;
+ unsigned 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;
- int count = 0;
+ unsigned 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;
- int i;
+ unsigned int i;
if (pw_crypt) {
auth_hash = pw_crypt->hash;
diff --git a/PasswordCrypt.c b/PasswordCrypt.c
index 8d72a82..e0a82cf 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 < 16 + alt_result[0]; ++cnt)
+ for (cnt = 0; cnt < 16ul + 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 < 16 + alt_result[0]; ++cnt)
+ for (cnt = 0; cnt < 16ul + alt_result[0]; ++cnt)
SHA512_Update(&alt_ctx, salt, salt_size);
SHA512_Final(tmp_result, &alt_ctx);
diff --git a/fallback.c b/fallback.c
index 44638ec..bc9a3c9 100644
--- a/fallback.c
+++ b/fallback.c
@@ -229,7 +229,7 @@ EFI_STATUS
find_boot_option(EFI_DEVICE_PATH *dp, CHAR16 *filename, CHAR16 *label,
CHAR16 *arguments, UINT16 *optnum)
{
- int size = sizeof(UINT32) + sizeof (UINT16) +
+ unsigned int size = sizeof(UINT32) + sizeof (UINT16) +
StrLen(label)*2 + 2 + DevicePathSize(dp) +
StrLen(arguments) * 2 + 2;
@@ -768,7 +768,7 @@ try_start_first_option(EFI_HANDLE parent_image_handle)
if (EFI_ERROR(rc)) {
CHAR16 *dps = DevicePathToStr(first_new_option);
UINTN s = DevicePathSize(first_new_option);
- int i;
+ unsigned int i;
UINT8 *dpv = (void *)first_new_option;
Print(L"LoadImage failed: %d\nDevice path: \"%s\"\n", rc, dps);
for (i = 0; i < s; i++) {
diff --git a/shim.c b/shim.c
index 0e18d38..8c583a4 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, int size, unsigned int address)
+static void *ImageAddress (void *image, unsigned int size, unsigned int address)
{
if (address > size)
return NULL;
@@ -494,18 +494,19 @@ static BOOLEAN secure_mode (void)
* Calculate the SHA1 and SHA256 hashes of a binary
*/
-static EFI_STATUS generate_hash (char *data, int datasize,
+static EFI_STATUS generate_hash (char *data, int datasize_in,
PE_COFF_LOADER_IMAGE_CONTEXT *context,
UINT8 *sha256hash, UINT8 *sha1hash)
{
unsigned int sha256ctxsize, sha1ctxsize;
- unsigned int size = datasize;
+ unsigned int size = datasize_in;
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;
@@ -517,6 +518,12 @@ static EFI_STATUS generate_hash (char *data, int datasize,
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;
@@ -577,22 +584,29 @@ static EFI_STATUS generate_hash (char *data, int datasize,
SumOfBytesHashed = context->PEHdr->Pe32.OptionalHeader.SizeOfHeaders;
#endif
- Section = (EFI_IMAGE_SECTION_HEADER *) (
- (char *)context->PEHdr + sizeof (UINT32) +
- sizeof (EFI_IMAGE_FILE_HEADER) +
- context->PEHdr->Pe32.FileHeader.SizeOfOptionalHeader
- );
-
- SectionCache = Section;
-
+ /* Validate section locations and sizes */
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;
+ EFI_IMAGE_SECTION_HEADER *SectionPtr;
+
+ /* 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;
}
SectionHeader = (EFI_IMAGE_SECTION_HEADER *) AllocateZeroPool (sizeof (EFI_IMAGE_SECTION_HEADER) * context->PEHdr->Pe32.FileHeader.NumberOfSections);
@@ -602,6 +616,11 @@ static EFI_STATUS generate_hash (char *data, int datasize,
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;
@@ -620,7 +639,6 @@ static EFI_STATUS generate_hash (char *data, int datasize,
continue;
}
hashbase = ImageAddress(data, size, Section->PointerToRawData);
- hashsize = (unsigned int) Section->SizeOfRawData;
if (!hashbase) {
Print(L"Malformed section header\n");
@@ -628,6 +646,15 @@ static EFI_STATUS generate_hash (char *data, int datasize,
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");
@@ -638,10 +665,10 @@ static EFI_STATUS generate_hash (char *data, int datasize,
}
/* Hash all remaining data */
- if (size > SumOfBytesHashed) {
+ if (datasize > SumOfBytesHashed) {
hashbase = data + SumOfBytesHashed;
hashsize = (unsigned int)(
- size -
+ datasize -
#if __LP64__
context->PEHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY].Size -
#else
@@ -884,7 +911,8 @@ static EFI_STATUS read_header(void *data, unsigned int datasize,
return EFI_UNSUPPORTED;
}
- if (((UINT8 *)context->SecDir - (UINT8 *)data) > (datasize - sizeof(EFI_IMAGE_DATA_DIRECTORY))) {
+ if ((unsigned long)((UINT8 *)context->SecDir - (UINT8 *)data) >
+ (datasize - sizeof(EFI_IMAGE_DATA_DIRECTORY))) {
Print(L"Invalid image\n");
return EFI_UNSUPPORTED;
}
@@ -904,7 +932,8 @@ static EFI_STATUS handle_image (void *data, unsigned int datasize,
{
EFI_STATUS efi_status;
char *buffer;
- int i, size;
+ int i;
+ unsigned int size;
EFI_IMAGE_SECTION_HEADER *Section;
char *base, *end;
PE_COFF_LOADER_IMAGE_CONTEXT context;
@@ -1081,7 +1110,8 @@ static EFI_STATUS generate_path(EFI_LOADED_IMAGE *li, CHAR16 *ImagePath,
{
EFI_DEVICE_PATH *devpath;
EFI_HANDLE device;
- int i, j, last = -1;
+ unsigned int i;
+ int j, last = -1;
unsigned int pathlen = 0;
EFI_STATUS efi_status = EFI_SUCCESS;
CHAR16 *bootpath;
@@ -1637,9 +1667,10 @@ EFI_STATUS set_second_stage (EFI_HANDLE image_handle)
EFI_STATUS status;
EFI_LOADED_IMAGE *li;
CHAR16 *start = NULL, *c;
- int i, remaining_size = 0;
+ unsigned int i;
+ int remaining_size = 0;
CHAR16 *loader_str = NULL;
- int loader_len = 0;
+ unsigned int loader_len = 0;
second_stage = DEFAULT_LOADER;
load_options = NULL;
--
1.9.3