Skip to contentSkip to conversation

libkmod: properly validate file size

PATCH In review Tobias Stoeckmann 2 participants 2 messages Feb 10, 2015 – Feb 13, 2015

lore
Share & tools
lore

Tobias Stoeckmann

author
#0 lore raw

PATCH 1/1 libkmod: properly validate file size

Hi,

in function kmod_elf_new, the file size has to be properly validated against section offset. Currently, the file size is considered valid based on ELF header size + section header size * section count. That is not sufficient.

In fact, ELF specifies a section header offset, which doesn't have to be the size of the ELF header. The supplied test cases even cover this.

The correct test is: section offset + section header size * section count

This patch also verifies that this value won't overflow. I don't know a way to crash a tool due to this bug, because later on the offset check would prevent out-of-bounds access. An overflow would just mean to access a wrong part in elf->memory. Yet it's a validation error.

Please note: The file size does not have to be validated against the size of the ELF header again, elf_identify did this already.

Tobias

---
 libkmod/libkmod-elf.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
libkmod/libkmod-elf.c +5 -5
index 4af829e..2f50ad2 100644
@@ -272,7 +272,8 @@ static const char *elf_get_strings_section(const struct kmod_elf *elf, uint64_t
 struct kmod_elf *kmod_elf_new(const void *memory, off_t size)
 {
 	struct kmod_elf *elf;
-	size_t hdr_size, shdr_size, min_size;
+	uint64_t min_size;
+	size_t shdrs_size, shdr_size;
 	int class;
 
 	assert_cc(sizeof(uint16_t) == sizeof(Elf32_Half));

@@ -308,12 +309,10 @@ struct kmod_elf *kmod_elf_new(const void *memory, off_t size)
 	if (elf->class & KMOD_ELF_32) {
 		const Elf32_Ehdr *hdr _unused_ = elf_get_mem(elf, 0);
 		LOAD_HEADER;
-		hdr_size = sizeof(Elf32_Ehdr);
 		shdr_size = sizeof(Elf32_Shdr);
 	} else {
 		const Elf64_Ehdr *hdr _unused_ = elf_get_mem(elf, 0);
 		LOAD_HEADER;
-		hdr_size = sizeof(Elf64_Ehdr);
 		shdr_size = sizeof(Elf64_Shdr);
 	}
 #undef LOAD_HEADER

@@ -330,8 +329,9 @@ struct kmod_elf *kmod_elf_new(const void *memory, off_t size)
 		       elf->header.section.entry_size, shdr_size);
 		goto invalid;
 	}
-	min_size = hdr_size + shdr_size * elf->header.section.count;
-	if (min_size >= elf->size) {
+	shdrs_size = shdr_size * elf->header.section.count;
+	if (addu64_overflow(shdrs_size, elf->header.section.offset, &min_size)
+	    || min_size > elf->size) {
 		ELFDBG(elf, "file is too short to hold sections\n");
 		goto invalid;
 	}
Signature · 1 line
2.3.0
#1 lore raw

Hi,

Quoted 8 lines · Tobias Stoeckmann

Hi,

in function kmod_elf_new, the file size has to be properly validated against section offset. Currently, the file size is considered valid based on ELF header size + section header size * section count. That is not sufficient.

In fact, ELF specifies a section header offset, which doesn't have to be the size of the ELF header. The supplied test cases even cover this.

the supplied test cases? which test cases?

Quoted 1 line

The correct test is: section offset + section header size * section count

I'm not sure about this anymore without looking up to the specification. Could you point here in the commit message?

thanks
Lucas De Marchi