linux-kernel - Re: [PATCH 16/23] pefile: Parse a PE binary to find a key and a signature contained therein

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 31 Oct 2012 12:31:29 +0000
From:	David Howells <dhowells@...hat.com>
To:	Kees Cook <keescook@...omium.org>
Cc:	dhowells@...hat.com, rusty@...tcorp.com.au, pjones@...hat.com,
	jwboyer@...hat.com, mjg@...hat.com, dmitry.kasatkin@...el.com,
	zohar@...ux.vnet.ibm.com, keyrings@...ux-nfs.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 16/23] pefile: Parse a PE binary to find a key and a signature contained therein

Kees Cook <keescook@...omium.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;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/