Re: Fw: signed kernel modules?

[Rusty Russell <rusty@rustcorp.com.au>]

On Thu, 2004-10-14 at 07:18, David Howells wrote:
> > Write the code.  Then come back and tell me it "isn't that hard".
> 
> It isn't that hard.

I see.  Well, the good news is that I think your canonicalization step
now covers all the exploits I can think of.

@@ -1536,8 +1538,59 @@ static struct module *load_module(void _
 		goto free_hdr;
 	}
 
-	if (len < hdr->e_shoff + hdr->e_shnum * sizeof(Elf_Shdr))
-		goto truncated;
+	/* verify the module (validates ELF and checks signature) */
+	gpgsig_ok = 0;
+	err = module_verify(hdr, len);
+	if (err < 0)
+		goto free_hdr;
+	if (err == 1)
+		gpgsig_ok = 1;
 
 	/* Convenience variables */
 	sechdrs = (void *)hdr + hdr->e_shoff;

I'd prefer to see:
	err = module_verify(hdr, len, &gpgsig_ok);
	if (err)
		goto free_hdr;

And then have module_verify for the !CONFIG_MODULE_SIG case (in
module-verify.h) simply be:

	if (len < hdr->e_shoff + hdr->e_shnum * sizeof(Elf_Shdr))
		return -EINVAL;
	*gpgsig_ok = 1;

+/*****************************************************************************/
+/*
+ * verify the ELF structure of a module
+ */
+static int module_verify_elf(struct module_verify_data *mvdata)
+{
+	const Elf_Ehdr *hdr = mvdata->hdr;
+	const Elf_Shdr *section, *section2, *secstop;
+	const Elf_Rela *relas, *rela, *relastop;
+	const Elf_Rel *rels, *rel, *relstop;
+	const Elf_Sym *symbol, *symstop;
+	size_t size, sssize, *secsize, tmp, tmp2;
+	int line;
+
+	size = mvdata->size;
+	mvdata->nsects = hdr->e_shnum;
+
+#define elfcheck(X) \
+do { if (unlikely(!(X))) { line = __LINE__; goto elfcheck_error; } } while(0)
+
+#define seccheck(X) \
+do { if (unlikely(!(X))) { line = __LINE__; goto seccheck_error; } } while(0)
+
+#define symcheck(X) \
+do { if (unlikely(!(X))) { line = __LINE__; goto symcheck_error; } } while(0)
+
+#define relcheck(X) \
+do { if (unlikely(!(X))) { line = __LINE__; goto relcheck_error; } } while(0)
+
+#define relacheck(X) \
+do { if (unlikely(!(X))) { line = __LINE__; goto relacheck_error; } } while(0)
+
+	/* validate the ELF header */
+	elfcheck(hdr->e_ehsize < size);
+	elfcheck(hdr->e_entry == 0);
+	elfcheck(hdr->e_phoff == 0);
+	elfcheck(hdr->e_phnum == 0);
+
+	elfcheck(hdr->e_shoff < size);
+	elfcheck(hdr->e_shoff >= hdr->e_ehsize);
+	elfcheck((hdr->e_shoff & (sizeof(long) - 1)) == 0);
+	elfcheck(hdr->e_shstrndx > 0);
+	elfcheck(hdr->e_shstrndx < hdr->e_shnum);
+
+	tmp = (size_t) hdr->e_shentsize * (size_t) hdr->e_shnum;
+	elfcheck(tmp < size - hdr->e_shoff);

Multiplicative overflow.  Also check that hdr->e_shentsize is
sizeof(Elf_Shdr) since you assume that below.

+	/* allocate a table to hold in-file section sizes */
+	mvdata->secsizes = kmalloc(hdr->e_shnum * sizeof(size_t), GFP_KERNEL);
+	if (!mvdata->secsizes)
+		return -ENOMEM;
+	memset(mvdata->secsizes, 0, hdr->e_shnum * sizeof(size_t));

Multiplicative overflow again: we could kmalloc 0 bytes and overflow below.

+	/* validate the ELF section headers */
+	mvdata->sections = mvdata->buffer + hdr->e_shoff;
+	secstop = mvdata->sections + mvdata->nsects;

Subtler multiplicative overflow.

+	sssize = mvdata->sections[hdr->e_shstrndx].sh_size;
+	elfcheck(sssize > 0);
+
+	section = mvdata->sections;
+	seccheck(section->sh_type == SHT_NULL);
+	seccheck(section->sh_size == 0);
+	seccheck(section->sh_offset == 0);
+
+	secsize = mvdata->secsizes + 1;
+	for (section++; section < secstop; secsize++, section++) {
+		seccheck(section->sh_name < sssize);
+		seccheck(section->sh_addr == 0);
+		seccheck(section->sh_link < hdr->e_shnum);
+
+		if (section->sh_entsize > 0)
+			seccheck(section->sh_size % section->sh_entsize == 0);

Divide by zero (thanks Alan!).

+		seccheck(section->sh_offset >= hdr->e_ehsize);
+		seccheck(section->sh_offset < size);
+
+		/* determine the section's in-file size */
+		tmp = size - section->sh_offset;
+		if (section->sh_offset < hdr->e_shoff)
+			tmp = hdr->e_shoff - section->sh_offset;
+
+		for (section2 = mvdata->sections + 1; section2 < secstop; section2++) {
+			if (section->sh_offset < section2->sh_offset) {
+				tmp2 = section2->sh_offset - section->sh_offset;
+				if (tmp2 < tmp)
+					tmp = tmp2;
+			}
+		}
+		*secsize = tmp;
+
+		_debug("Section %ld: %zx bytes at %lx\n",
+		       section - mvdata->sections,
+		       *secsize,
+		       section->sh_offset);
+
+		/* perform section type specific checks */
+		switch (section->sh_type) {
+		case SHT_NOBITS:
+			break;
+
+		case SHT_REL:
+		case SHT_RELA:
+			seccheck(section->sh_info > 0);
+			seccheck(section->sh_info < hdr->e_shnum);
+			/* fall through */
+
+		default:
+			/* most types of section must be contained entirely
+			 * within the file */
+			seccheck(section->sh_size <= *secsize);
+			break;
+		}
+	}
+
+	/* validate the ELF section names */
+	section = &mvdata->sections[hdr->e_shstrndx];
+
+	seccheck(section->sh_offset != hdr->e_shoff);
+
+	mvdata->secstrings = mvdata->buffer + section->sh_offset;
+
+	for (section = mvdata->sections + 1; section < secstop; section++) {
+		const char *secname;
+		tmp = sssize - section->sh_name;
+		secname = mvdata->secstrings + section->sh_name;
+		seccheck(secname[0] != 0);
+		seccheck(memchr(secname, 0, tmp) != NULL);
+	}
+
+	/* look for various sections in the module */
+	for (section = mvdata->sections + 1; section < secstop; section++) {
+		switch (section->sh_type) {
+		case SHT_SYMTAB:
+			if (strcmp(mvdata->secstrings + section->sh_name,
+				   ".symtab") == 0
+			    ) {
+				seccheck(mvdata->symbols == NULL);
+				mvdata->symbols =
+					mvdata->buffer + section->sh_offset;
+				mvdata->nsyms =
+					section->sh_size / sizeof(Elf_Sym);
+				seccheck(section->sh_size > 0);
+			}
+			break;
+
+		case SHT_STRTAB:
+			if (strcmp(mvdata->secstrings + section->sh_name,
+				   ".strtab") == 0
+			    ) {
+				seccheck(mvdata->strings == NULL);
+				mvdata->strings =
+					mvdata->buffer + section->sh_offset;
+				sssize = mvdata->nstrings = section->sh_size;
+				seccheck(section->sh_size > 0);
+			}
+			break;
+		}
+	}
+
+	if (!mvdata->symbols) {
+		printk("Couldn't locate module symbol table\n");
+		goto format_error;
+	}
+
+	if (!mvdata->strings) {
+		printk("Couldn't locate module strings table\n");
+		goto format_error;
+	}
+
+	/* validate the symbol table */
+	symstop = mvdata->symbols + mvdata->nsyms;
+
+	symbol = mvdata->symbols;
+	symcheck(ELF_ST_TYPE(symbol[0].st_info) == STT_NOTYPE);
+	symcheck(symbol[0].st_shndx == SHN_UNDEF);
+	symcheck(symbol[0].st_value == 0);
+	symcheck(symbol[0].st_size == 0);
+
+	for (symbol++; symbol < symstop; symbol++) {
+		symcheck(symbol->st_name < sssize);

I think you have to check (as above) that st_name is nul terminated
within size.

+		symcheck(symbol->st_shndx < mvdata->nsects ||
+			 symbol->st_shndx >= SHN_LORESERVE);
+	}
+
+	/* validate each relocation table as best we can */
+	for (section = mvdata->sections + 1; section < secstop; section++) {
+		section2 = mvdata->sections + section->sh_info;
+
+		switch (section->sh_type) {
+		case SHT_REL:
+			rels = mvdata->buffer + section->sh_offset;
+			relstop = mvdata->buffer + section->sh_offset + section->sh_size;
+
+			for (rel = rels; rel < relstop; rel++) {
+				relcheck(rel->r_offset < section2->sh_size);
+				relcheck(ELF_R_SYM(rel->r_info) > 0);
+				relcheck(ELF_R_SYM(rel->r_info) < mvdata->nsyms);

I think you can overflow here.  For REL and RELA sections, you don't
check that sh_size is <= *secsize.

That's all I found,
Rusty.
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell