Messages in this thread | | | Subject | Re: Fw: signed kernel modules? | From | Rusty Russell <> | Date | Thu, 14 Oct 2004 09:01:21 +1000 |
| |
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
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |