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