Bug 206761 - Kernel stack overflow in sysctl handler for kern.binmisc.add
Summary: Kernel stack overflow in sysctl handler for kern.binmisc.add
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: FreeBSD bugs mailing list
URL:
Keywords: needs-patch, needs-qa, security
Depends on:
Blocks:
 
Reported: 2016-01-30 16:38 UTC by CTurt
Modified: 2016-04-05 19:03 UTC (History)
8 users (show)

See Also:
sbruno: mfc-stable10+


Attachments
Proposed patch (1.25 KB, patch)
2016-03-31 20:22 UTC, Stacey Son
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-01-30 16:38:26 UTC
There is a stack overflow in the `sysctl_kern_binmisc` code path from `sys/kern/imgact_binmisc.c`:

	switch(arg2) {
	case IBC_ADD:
		/* Add an entry. Limited to IBE_MAX_ENTRIES. */
		error = SYSCTL_IN(req, &xbe, sizeof(xbe));
		if (error)
			return (error);
		if (IBE_VERSION != xbe.xbe_version)
			return (EINVAL);
		if (interp_list_entry_count == IBE_MAX_ENTRIES)
			return (ENOSPC);
		error = imgact_binmisc_add_entry(&xbe);
		break;

Notice that contents of `xbe` are user controlled, with just a check on the `xbe_version` member before calling `imgact_binmisc_add_entry`:

static int
imgact_binmisc_add_entry(ximgact_binmisc_entry_t *xbe)
{
	imgact_binmisc_entry_t *ibe;
	char *p;

	if (xbe->xbe_msize > IBE_MAGIC_MAX)
		return (EINVAL);

	for(p = xbe->xbe_name; *p != 0; p++)
		if (!isascii((int)*p))
			return (EINVAL);

	for(p = xbe->xbe_interpreter; *p != 0; p++)
		if (!isascii((int)*p))
			return (EINVAL);

Notice that already 2 out of bounds reads are possible. Since these strings come from userland, there is no guarantee that they are NULL truncated. Limiting the contentents of `sb_interpreter` to ASCII characters only is a minor annoyance, but let's continue:

	/* Make sure we don't have any invalid #'s. */
	p = xbe->xbe_interpreter;
	while (1) {
		p = strchr(p, '#');
		if (!p)
			break;

		p++;
		switch(*p) {
		case ISM_POUND:
			/* "##" */
			p++;
			break;

		case ISM_OLD_ARGV0:
			/* "#a" */
			p++;
			break;

		case 0:
		default:
			/* Anything besides the above is invalid. */
			return (EINVAL);
		}
	}

	sx_xlock(&interp_list_sx);
	if (imgact_binmisc_find_entry(xbe->xbe_name) != NULL) {
		sx_xunlock(&interp_list_sx);
		return (EEXIST);
	}

	/* Preallocate a new entry. */
	ibe = imgact_binmisc_new_entry(xbe);
	if (!ibe)
		return (ENOMEM);

	SLIST_INSERT_HEAD(&interpreter_list, ibe, link);
	interp_list_entry_count++;
	sx_xunlock(&interp_list_sx);

	return (0);
}

Basically, some more checks on the contents, but nothing limiting the size. Moving onto the `imgact_binmisc_new_entry` call:

static imgact_binmisc_entry_t *
imgact_binmisc_new_entry(ximgact_binmisc_entry_t *xbe)
{
	imgact_binmisc_entry_t *ibe = NULL;
	size_t namesz = min(strlen(xbe->xbe_name) + 1, IBE_NAME_MAX);

	ibe = malloc(sizeof(*ibe), M_BINMISC, M_WAITOK|M_ZERO);

	ibe->ibe_name = malloc(namesz, M_BINMISC, M_WAITOK|M_ZERO);
	strlcpy(ibe->ibe_name, xbe->xbe_name, namesz);

	imgact_binmisc_populate_interp(xbe->xbe_interpreter, ibe);

The important thing here is that `xbe->xbe_name` is limited to `IBE_NAME_MAX`, but there is no check on the size of `xbe->xbe_interpreter` string, it is passed directly to `imgact_binmisc_populate_interp`:

static void
imgact_binmisc_populate_interp(char *str, imgact_binmisc_entry_t *ibe)
{
	uint32_t len = 0, argc = 1;
	char t[IBE_INTERP_LEN_MAX];
	char *sp, *tp;

	memset(t, 0, sizeof(t));

	/*
	 * Normalize interpreter string. Replace white space between args with
	 * single space.
	 */
	sp = str; tp = t;
	while (*sp != '\0') {
		if (*sp == ' ' || *sp == '\t') {
			if (++len > IBE_INTERP_LEN_MAX)
				break;
			*tp++ = ' ';
			argc++;
			while (*sp == ' ' || *sp == '\t')
				sp++;
			continue;
		} else {
			*tp++ = *sp++;
			len++;
		}
	}
	*tp = '\0';
	len++;

	ibe->ibe_interpreter = malloc(len, M_BINMISC, M_WAITOK|M_ZERO);

	/* Populate all the ibe fields for the interpreter. */
	memcpy(ibe->ibe_interpreter, t, len);
	ibe->ibe_interp_argcnt = argc;
	ibe->ibe_interp_length = len;
}

Clearly, it is not safe to use this function on a user supplied string!

The characters of the input string (`str`) are iterated over without any check on size:

    while (*sp != '\0') {

There is a check on the `len` value, but only when the string reaches whitespace:

		if (*sp == ' ' || *sp == '\t') {
			if (++len > IBE_INTERP_LEN_MAX)
				break;

If the string consists of no whitespace we can increase `tp`, `sp`, and `len` freely (beyond the `IBE_INTERP_LEN_MAX` limit):

		} else {
			*tp++ = *sp++;
			len++;
		}

We then have a write from `tp`:

    *tp = '\0';

And also a `malloc` and `copyin` with `len`:

	ibe->ibe_interpreter = malloc(len, M_BINMISC, M_WAITOK|M_ZERO);

	/* Populate all the ibe fields for the interpreter. */
	memcpy(ibe->ibe_interpreter, t, len);

Remember that `t` is a fixed stack buffer:

    char t[IBE_INTERP_LEN_MAX];

So, when `len` exceeds `IBE_INTERP_LEN_MAX`, we have an out of bounds stack read here, and an out of bounds stack write from `*tp = '\0'`.

But the most exploitable thing is the `*tp++ = *sp++;` copy; we can use it write past the `t` buffer on the stack.

Here's the `ximgact_binmisc_entry` struct from `/sys/sys/imgact_binmisc.h`:

typedef struct ximgact_binmisc_entry {
	uint32_t xbe_version;	/* Struct version(IBE_VERSION) */
	uint32_t xbe_flags;	/* Entry flags (IBF_*) */
	uint32_t xbe_moffset;	/* Magic offset in header */
	uint32_t xbe_msize;	/* Magic size */
	uint32_t spare[3];	/* Spare fields for future use */
	char xbe_name[IBE_NAME_MAX];	/* Unique interpreter name */
	char xbe_interpreter[IBE_INTERP_LEN_MAX]; /* Interpreter path + args */
	uint8_t xbe_magic[IBE_MAGIC_MAX]; /* Header Magic */
	uint8_t xbe_mask[IBE_MAGIC_MAX]; /* Magic Mask */
} ximgact_binmisc_entry_t;

If we make `xbe_interpreter` non-terminated, the following fields, `xbe_magic` and `xbe_mask`, will be read from.

Let's look at these some `define`s to get an idea of the sizes:

#define MAXPATHLEN 1024
#define	IBE_ARG_LEN_MAX	256
#define	IBE_INTERP_LEN_MAX	(MAXPATHLEN + IBE_ARG_LEN_MAX)
#define	IBE_MAGIC_MAX	256

`xbe_interpreter` is 1280 bytes.
`xbe_magic` is 256 bytes.
`xbe_mask` is 256 bytes.

Now that we know we control the 512 bytes after the designated area for `xbe_interpreter`, let's go back to the target code:

	char t[IBE_INTERP_LEN_MAX];
	...
	sp = str; tp = t;
	while (*sp != '\0') {
		if (*sp == ' ' || *sp == '\t') {
			if (++len > IBE_INTERP_LEN_MAX)
				break;
			*tp++ = ' ';
			argc++;
			while (*sp == ' ' || *sp == '\t')
				sp++;
			continue;
		} else {
			*tp++ = *sp++;
			len++;
		}
	}
	*tp = '\0';

We fill our `xbe_interpreter` (`str`) with non-whitespace, ASCII bytes, 'a' for example so that the following code happens 1280 times:

    *tp++ = *sp++;

We then have stack overflow past `t` from user controlled contents `xbe_magic` and `xbe_mask` (ASCII values only) for any desired size up to the next 512 bytes (just place a 0 byte to end the overflow); we can use this to overflow return addresses on the stack to get kernel to jump anywhere we want as soon as this function returns, but only addresses with all ASCII bytes (but this isn't a problem).

Unfortunately, the sysctl node, `kern.binmisc.add` is only accessible as root.
Comment 1 CTurt 2016-01-30 17:15:24 UTC
PoC which causes panic:

https://gist.github.com/CTurt/ddcda1a5ff4a3a38cad2
Comment 2 Shawn Webb 2016-01-30 17:18:36 UTC
Please attribute any code fix to "CTurt from HardenedBSD"
Comment 3 Shawn Webb 2016-01-30 17:23:35 UTC
Does a CVE need to be issued for this vulnerability?
Comment 4 CTurt 2016-01-30 17:25:49 UTC
No need, I'll write a patch later. I'm busy with other potential bugs at the moment though.

I don't think a CVE is needed, since it is only triggerable as root (every time I find a bug, ffs).
Comment 5 johannes 2016-01-31 02:13:37 UTC
Is this also missing a sx_xunlock(&interp_list_sx) for the ENOMEM error case?
Comment 6 CTurt 2016-01-31 10:04:35 UTC
I didn't even notice this before, but you're right.

imgact_binmisc_add_entry:

	sx_xlock(&interp_list_sx);
	if (imgact_binmisc_find_entry(xbe->xbe_name) != NULL) {
		sx_xunlock(&interp_list_sx);
		return (EEXIST);
	}

	/* Preallocate a new entry. */
	ibe = imgact_binmisc_new_entry(xbe);
	if (!ibe)
		return (ENOMEM);

	SLIST_INSERT_HEAD(&interpreter_list, ibe, link);
	interp_list_entry_count++;
	sx_xunlock(&interp_list_sx);

If the code ever reaches `return (ENOMEM);`, it is missing an `sx_xunlock(&interp_list_sx);` call.

Unfortunately, this bug isn't triggerable, because `imgact_binmisc_add_entry` uses `M_WAITOK` for its allocations, and so can never return `NULL`:

static imgact_binmisc_entry_t *
imgact_binmisc_new_entry(ximgact_binmisc_entry_t *xbe)
{
	ibe = malloc(sizeof(*ibe), M_BINMISC, M_WAITOK|M_ZERO);

	...
	
	return (ibe);
}

My recommendation is to just remove the following check altogether:

	if (!ibe)
		return (ENOMEM);
Comment 7 Shawn Webb 2016-03-01 21:00:07 UTC
Any movement on this?
Comment 8 Sean Bruno freebsd_committer 2016-03-31 20:09:42 UTC
Something like this has been suggested by sson@ to resolve these issues.  What do you guys think?

diff --git a/sys/kern/imgact_binmisc.c b/sys/kern/imgact_binmisc.c
index dd57717..39ca156 100644
--- a/sys/kern/imgact_binmisc.c
+++ b/sys/kern/imgact_binmisc.c
@@ -1,5 +1,5 @@
/*-
- * Copyright (c) 2013-15, Stacey D. Son
+ * Copyright (c) 2013-16, Stacey D. Son
 * All rights reserved.
 *
 * Redistribution and use in source and binary forms, with or without
@@ -220,16 +220,17 @@ imgact_binmisc_add_entry(ximgact_binmisc_entry_t *xbe)
{
	imgact_binmisc_entry_t *ibe;
	char *p;
+	int cnt;

	if (xbe->xbe_msize > IBE_MAGIC_MAX)
		return (EINVAL);

-	for(p = xbe->xbe_name; *p != 0; p++)
-		if (!isascii((int)*p))
+	for(cnt = 0, p = xbe->xbe_name; *p != 0; cnt++, p++)
+		if (cnt >= IBE_NAME_MAX || !isascii((int)*p))
			return (EINVAL);

-	for(p = xbe->xbe_interpreter; *p != 0; p++)
-		if (!isascii((int)*p))
+	for(cnt = 0, p = xbe->xbe_interpreter; *p != 0; cnt++, p++)
+		if (cnt >= IBE_INTERP_LEN_MAX || !isascii((int)*p))
			return (EINVAL);

	/* Make sure we don't have any invalid #'s. */
@@ -266,8 +267,6 @@ imgact_binmisc_add_entry(ximgact_binmisc_entry_t *xbe)

	/* Preallocate a new entry. */
	ibe = imgact_binmisc_new_entry(xbe);
-	if (!ibe)
-		return (ENOMEM);

	SLIST_INSERT_HEAD(&interpreter_list, ibe, link);
	interp_list_entry_count++;
Comment 9 Stacey Son freebsd_committer 2016-03-31 20:22:16 UTC
Created attachment 168847 [details]
Proposed patch

Attached is a proposed patch.  Thanks to emaste@ for bring this to my attention and to CTurt and Shawn for their work on this.  If approved then please feel free to commit.
Comment 10 Sean Bruno freebsd_committer 2016-03-31 20:32:54 UTC
With the patch, the propsed test code does not crash the machine and returns:

root@tasty.ysv:/var/tmp # ./bad_test 
result -1
errno 22
Comment 11 Sean Bruno freebsd_committer 2016-03-31 20:34:04 UTC
Without this patch from sson, machine panics as described:

panic: stack overflow detected; backtrace may be corrupted
cpuid = 3
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe046ad0cb40
vpanic() at vpanic+0x182/frame 0xfffffe046ad0cbc0
panic() at panic+0x43/frame 0xfffffe046ad0cc20
__stack_chk_fail() at __stack_chk_fail+0x12/frame 0xfffffe046ad0cc30
sysctl_kern_binmisc() at sysctl_kern_binmisc+0x7b4/frame 0xfffffe046ad0d8b0
KDB: enter: panic
[ thread pid 16434 tid 101185 ]
Stopped at      kdb_enter+0x3b: movq    $0,kdb_why
db> bt
Tracing pid 16434 tid 101185 td 0xfffff8010054b9a0
kdb_enter() at kdb_enter+0x3b/frame 0xfffffe046ad0cb40
vpanic() at vpanic+0x19f/frame 0xfffffe046ad0cbc0
panic() at panic+0x43/frame 0xfffffe046ad0cc20
__stack_chk_fail() at __stack_chk_fail+0x12/frame 0xfffffe046ad0cc30
sysctl_kern_binmisc() at sysctl_kern_binmisc+0x7b4/frame 0xfffffe046ad0d8b0
db>
Comment 12 op 2016-03-31 21:55:19 UTC
@CTurt, could you please very / review the patch?
Comment 13 CTurt 2016-04-01 11:08:09 UTC
(In reply to Stacey Son from comment #9)
This patch looks good to me; correctly checks the size of the `xbe_name` and `xbe_interpreter` strings early on, before any out of bounds reads or writes could be performed.
Comment 14 CTurt 2016-04-01 11:36:49 UTC
I've taken another look at the code and found another potential bug. I'm not certain if this is a bug yet, but I'd also like to bring the following code from `imgact_binmisc_add_entry` to attention:

	/* Make sure we don't have any invalid #'s. */
	p = xbe->xbe_interpreter;
	while (1) {
		p = strchr(p, '#');
		if (!p)
			break;

		p++;
		switch(*p) {
		case ISM_POUND:
			/* "##" */
			p++;
			break;

		case ISM_OLD_ARGV0:
			/* "#a" */
			p++;
			break;

		case 0:
		default:
			/* Anything besides the above is invalid. */
			return (EINVAL);
		}
	}

From the comment, and usage of a loop, it seems like this code should be checking that every '#' character in the string follows either another '#' or an 'a' character, however there is no way that this loop will ever be executed more than once since all conditions lead to `break` or `return`. In its current form the code will only validate the first '#' character.

To instead check that _every_ '#' character follows a valid character (and not just the first '#' character), the `case`s should `continue` the loop as below:

	/* Make sure we don't have any invalid #'s. */
	p = xbe->xbe_interpreter;
	while (1) {
		p = strchr(p, '#');
		if (!p)
			break;

		p++;
		switch(*p) {
		case ISM_POUND:
			/* "##" */
			p++;
			continue;

		case ISM_OLD_ARGV0:
			/* "#a" */
			p++;
			continue;

		case 0:
		default:
			/* Anything besides the above is invalid. */
			return (EINVAL);
		}
	}
Comment 15 CTurt 2016-04-01 11:50:11 UTC
(In reply to CTurt from comment #14)
Don't worry about this actually, the `break` exits the `switch`, and the loop will continue, checking all '#' characters, no need for change here.
Comment 16 commit-hook freebsd_committer 2016-04-01 16:16:43 UTC
A commit references this bug:

Author: sbruno
Date: Fri Apr  1 16:16:26 UTC 2016
New revision: 297488
URL: https://svnweb.freebsd.org/changeset/base/297488

Log:
  Repair a overflow condition where a user could submit a string that was
  not getting a proper bounds check.

  Thanks to CTurt for pointing at this with a big red blinking neon sign.

  PR:		206761
  Submitted by:	sson
  Reviewed by:	cturt@hardenedbsd.org
  MFC after:	3 days

Changes:
  head/sys/kern/imgact_binmisc.c
Comment 17 commit-hook freebsd_committer 2016-04-05 18:28:05 UTC
A commit references this bug:

Author: sbruno
Date: Tue Apr  5 18:27:47 UTC 2016
New revision: 297588
URL: https://svnweb.freebsd.org/changeset/base/297588

Log:
  MFC r297488

  Repair an overflow condition where a user could submit a string that was
  not getting a proper bounds check.

  PR:		206761
  Submitted by:	sson
  Reviewed by:	cturt@hardenedbsd.org

Changes:
_U  stable/10/
  stable/10/sys/kern/imgact_binmisc.c