lkml.org 
[lkml]   [2004]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: Fw: signed kernel modules?
From
Date
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/

\
 
 \ /
  Last update: 2005-03-22 14:07    [W:0.138 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site