lkml.org 
[lkml]   [2012]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH 16/23] pefile: Parse a PE binary to find a key and a signature contained therein
Date
Kees Cook <keescook@chromium.org> wrote:

> Which means this loop will walk past the end of the memory (loop is
> bounded by n_sections, so secs[loop] can go past datalen). While
> data_addr and raw_data_size will stay bounded, the read of sec->name
> can be out of bounds.

Assuming n_sections is checked, sec->name can't be out of bounds because it's
a char array, not a pointer.

> (Also, do you want a "break" in there after the first .keylist is found, or
> is this intentionally "use last key list"?)

Actually, no I don't. The loop also checks the limits on each section - which
I need to do since I have to individually digest the sections later.

Attached is a patch I'm applying to pefile_parse_binary() to be more rigorous
about the checking of values in the PE binary.

David
---
diff --git a/crypto/asymmetric_keys/pefile_parser.c b/crypto/asymmetric_keys/pefile_parser.c
index efae278..fb80cf0 100644
--- a/crypto/asymmetric_keys/pefile_parser.c
+++ b/crypto/asymmetric_keys/pefile_parser.c
@@ -42,8 +42,8 @@ static int pefile_parse_binary(struct key_preparsed_payload *prep,

#define chkaddr(base, x, s) \
do { \
- if ((x) < base || (s) >= datalen || (x) > datalen - (s)) \
- return -ERANGE; \
+ if ((x) < base || (s) >= datalen || (x) > datalen - (s)) \
+ return -ELIBBAD; \
} while(0)

chkaddr(0, 0, sizeof(*mz));
@@ -88,7 +88,13 @@ static int pefile_parse_binary(struct key_preparsed_payload *prep,
pr_devel("checksum @ %x\n", ctx->image_checksum_offset);
pr_devel("header size = %x\n", ctx->header_size);

- chkaddr(0, cursor, sizeof(*ddir));
+ if (cursor >= ctx->header_size || ctx->header_size >= datalen)
+ return -ELIBBAD;
+
+ if (ctx->n_data_dirents > (ctx->header_size - cursor) / sizeof(*dde) ||
+ ctx->n_data_dirents < sizeof(*ddir) / sizeof(*dde))
+ return -ELIBBAD;
+
ddir = prep->data + cursor;
cursor += sizeof(*dde) * ctx->n_data_dirents;

@@ -101,7 +107,7 @@ static int pefile_parse_binary(struct key_preparsed_payload *prep,
return -EKEYREJECTED;
}

- chkaddr(cursor, ddir->certs.virtual_address, ddir->certs.size);
+ chkaddr(ctx->header_size, ddir->certs.virtual_address, ddir->certs.size);
ctx->sig_offset = ddir->certs.virtual_address;
ctx->sig_len = ddir->certs.size;
pr_devel("cert = %x @%x [%*ph]\n",
@@ -112,7 +118,8 @@ static int pefile_parse_binary(struct key_preparsed_payload *prep,
* section containing the list of keys.
*/
ctx->n_sections = pe->sections;
- chkaddr(0, cursor, sizeof(*sec) * ctx->n_sections);
+ if (ctx->n_sections > (ctx->header_size - cursor) / sizeof(*sec))
+ return -ELIBBAD;
ctx->secs = secs = prep->data + cursor;
cursor += sizeof(*sec) * ctx->n_sections;


\
 
 \ /
  Last update: 2012-10-31 13:41    [W:0.870 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site