gen_init_cpio: avoid stack overflow when expanding

PATCH RESEND In review Kees Cook 2 participants 6 messages Oct 24, 2012 Kbuild

lore

Share & tools

lore

Kees Cook

author

2012-10-24 20:58 UTC #0 lore raw

PATCH RESEND 1/1 gen_init_cpio: avoid stack overflow when expanding

Fix possible overflow of the buffer used for expanding environment variables when building file list.

$ cat usr/crash.list
file foo ${BIG}${BIG}${BIG}${BIG}${BIG}${BIG} 0755 0 0
$ BIG=$(perl -e 'print "A" x 4096;') ./usr/gen_init_cpio usr/crash.list
*** buffer overflow detected ***: ./usr/gen_init_cpio terminated

This also replaces the space-indenting with tabs.

Patch based on existing fix extracted from grsecurity.

Cc Andrew Morton <[email protected]>

Cc Michal Marek <[email protected]>

Cc Brad Spengler <[email protected]>

Cc PaX Team <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 usr/gen_init_cpio.c |   43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

usr/gen_init_cpio.c +23 ~~-20~~

index af0f22f..aca6edc 100644
@@ -303,7 +303,7 @@ static int cpio_mkfile(const char *name, const char *location,
 	int retval;
 	int rc = -1;
 	int namesize;
-	int i;
+	unsigned int i;
 
 	mode |= S_IFREG;

@@ -381,25 +381,28 @@ error:
 
 static char *cpio_replace_env(char *new_location)
 {
-       char expanded[PATH_MAX + 1];
-       char env_var[PATH_MAX + 1];
-       char *start;
-       char *end;
-
-       for (start = NULL; (start = strstr(new_location, "${")); ) {
-               end = strchr(start, '}');
-               if (start < end) {
-                       *env_var = *expanded = '\0';
-                       strncat(env_var, start + 2, end - start - 2);
-                       strncat(expanded, new_location, start - new_location);
-                       strncat(expanded, getenv(env_var), PATH_MAX);
-                       strncat(expanded, end + 1, PATH_MAX);
-                       strncpy(new_location, expanded, PATH_MAX);
-               } else
-                       break;
-       }
-
-       return new_location;
+	char expanded[PATH_MAX + 1];
+	char env_var[PATH_MAX + 1];
+	char *start;
+	char *end;
+
+	for (start = NULL; (start = strstr(new_location, "${")); ) {
+		end = strchr(start, '}');
+		if (start < end) {
+			*env_var = *expanded = '\0';
+			strncat(env_var, start + 2, end - start - 2);
+			strncat(expanded, new_location, start - new_location);
+			strncat(expanded, getenv(env_var),
+				PATH_MAX - strlen(expanded));
+			strncat(expanded, end + 1,
+				PATH_MAX - strlen(expanded));
+			strncpy(new_location, expanded, PATH_MAX);
+			new_location[PATH_MAX] = 0;
+		} else
+			break;
+	}
+
+	return new_location;
 }

Signature · 6 lines

1.7.9.5


-- 
Kees Cook
Chrome OS Security

Andrew Morton

2012-10-24 21:02 UTC #1 lore raw

Quoted 8 lines · On Wed, 24 Oct 2012 13:57:56 -0700 Kees Cook <[email protected]> wrote:

Fix possible overflow of the buffer used for expanding environment variables when building file list.
$ cat usr/crash.list
file foo ${BIG}${BIG}${BIG}${BIG}${BIG}${BIG} 0755 0 0
$ BIG=$(perl -e 'print "A" x 4096;') ./usr/gen_init_cpio usr/crash.list
*** buffer overflow detected ***: ./usr/gen_init_cpio terminated

Quoted 7 lines

This also replaces the space-indenting with tabs.
Patch based on existing fix extracted from grsecurity.
...
Cc [email protected]

Why did you feel we need to backport this to -stable?

Kees Cook

author

2012-10-24 21:33 UTC #2 lore raw

Quoted 13 lines · Andrew Morton

Quoted 10 lines · On Wed, 24 Oct 2012 13:57:56 -0700 Kees Cook <[email protected]> wrote:
Fix possible overflow of the buffer used for expanding environment variables when building file list.
$ cat usr/crash.list
file foo ${BIG}${BIG}${BIG}${BIG}${BIG}${BIG} 0755 0 0
$ BIG=$(perl -e 'print "A" x 4096;') ./usr/gen_init_cpio usr/crash.list
*** buffer overflow detected ***: ./usr/gen_init_cpio terminated
This also replaces the space-indenting with tabs.

Quoted 7 lines

Quoted 5 lines
Patch based on existing fix extracted from grsecurity.
...
Cc [email protected]
Why did you feel we need to backport this to -stable?

It's an extremely hard to hit security issue, but it's a security fix regardless. I won't cry if it doesn't go to stable, but it seems a trivial fix, so I included it for stable.

-Kees

Signature · 2 lines

Kees Cook
Chrome OS Security

Andrew Morton

2012-10-24 21:44 UTC #3 lore raw

Quoted 19 lines · On Wed, 24 Oct 2012 14:33:02 -0700 Kees Cook <[email protected]> wrote:

Quoted 17 lines · Andrew Morton
Quoted 14 lines · On Wed, 24 Oct 2012 13:57:56 -0700 Kees Cook <[email protected]> wrote:
Fix possible overflow of the buffer used for expanding environment variables when building file list.
$ cat usr/crash.list
file foo ${BIG}${BIG}${BIG}${BIG}${BIG}${BIG} 0755 0 0
$ BIG=$(perl -e 'print "A" x 4096;') ./usr/gen_init_cpio usr/crash.list
*** buffer overflow detected ***: ./usr/gen_init_cpio terminated
This also replaces the space-indenting with tabs.
Patch based on existing fix extracted from grsecurity.
...

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Cc [email protected]
Why did you feel we need to backport this to -stable?
It's an extremely hard to hit security issue, but it's a security fix regardless. I won't cry if it doesn't go to stable, but it seems a trivial fix, so I included it for stable.

Well, I do think that a description of the user impact of the bug should be included in the changelog so that poor old Greg can work out why we sent it at him.

If you can suggest some suitable text I can copy-n-slurp that into the changelog.

Kees Cook

author

2012-10-24 21:53 UTC #4 lore raw

Quoted 30 lines · Andrew Morton

Quoted 19 lines · On Wed, 24 Oct 2012 14:33:02 -0700 Kees Cook <[email protected]> wrote:
Quoted 17 lines · Andrew Morton
Quoted 14 lines · On Wed, 24 Oct 2012 13:57:56 -0700 Kees Cook <[email protected]> wrote:
Fix possible overflow of the buffer used for expanding environment variables when building file list.
$ cat usr/crash.list
file foo ${BIG}${BIG}${BIG}${BIG}${BIG}${BIG} 0755 0 0
$ BIG=$(perl -e 'print "A" x 4096;') ./usr/gen_init_cpio usr/crash.list
*** buffer overflow detected ***: ./usr/gen_init_cpio terminated
This also replaces the space-indenting with tabs.
Patch based on existing fix extracted from grsecurity.
...
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Cc [email protected]
Why did you feel we need to backport this to -stable?
It's an extremely hard to hit security issue, but it's a security fix regardless. I won't cry if it doesn't go to stable, but it seems a trivial fix, so I included it for stable.

Quoted 6 lines

Well, I do think that a description of the user impact of the bug should be included in the changelog so that poor old Greg can work out why we sent it at him.
If you can suggest some suitable text I can copy-n-slurp that into the changelog.

How about replacing the first paragraph with:

Fix possible overflow of the buffer used for expanding environment variables when building file list. In the extremely unlikely case of an attacker having control over the environment variables visible to gen_init_cpio, control over the contents of the file gen_init_cpio parses, and gen_init_cpio was built without compiler hardening, the attacker can gain arbitrary execution control via a stack buffer overflow.

-Kees

Signature · 2 lines

Kees Cook
Chrome OS Security

Andrew Morton

2012-10-24 21:57 UTC #5 lore raw

Quoted 9 lines · On Wed, 24 Oct 2012 14:53:33 -0700 Kees Cook <[email protected]> wrote:

Quoted 6 lines
Well, I do think that a description of the user impact of the bug should be included in the changelog so that poor old Greg can work out why we sent it at him.
If you can suggest some suitable text I can copy-n-slurp that into the changelog.
How about replacing the first paragraph with:

Quoted 7 lines

Fix possible overflow of the buffer used for expanding environment variables when building file list. In the extremely unlikely case of an attacker having control over the environment variables visible to gen_init_cpio, control over the contents of the file gen_init_cpio parses, and gen_init_cpio was built without compiler hardening, the attacker can gain arbitrary execution control via a stack buffer overflow.

ooh, spiffy - even I understood that!