Bug 206585 - hpt_set_info possible buffer overflow
Summary: hpt_set_info possible buffer overflow
Status: Open
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-qa, patch, security
Depends on:
Blocks:
 
Reported: 2016-01-24 17:57 UTC by CTurt
Modified: 2016-04-18 23:26 UTC (History)
4 users (show)

See Also:
koobs: mfc-stable9?
koobs: mfc-stable10?


Attachments
Fix heap overflow and check result of copyin (1.34 KB, patch)
2016-04-07 16:40 UTC, CTurt
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-24 17:57:25 UTC
In `hpt_status` -> `hpt_set_info`, `nOutBufferSize` and `nInBufferSize` are checked at the same time, but not individually:

if (piop->nInBufferSize+piop->nOutBufferSize > PAGE_SIZE) {
	KdPrintE(("User buffer too large\n"));
	return -EINVAL;
}

Before performing a kernel allocation:

ke_area = malloc(piop->nInBufferSize+piop->nOutBufferSize, M_DEVBUF, M_NOWAIT);

However, the sizes are later used individually for some copies:

if (piop->nInBufferSize)
	copyin((void*)(ULONG_PTR)piop->lpInBuffer, ke_area, piop->nInBufferSize);

...

if (piop->nOutBufferSize)
	copyout(ke_area + piop->nInBufferSize, (void*)(ULONG_PTR)piop->lpOutBuffer, piop->nOutBufferSize);

It might be possible for `nInBufferSize`, or `outBufferSize`, or both, to be large enough for `piop->nInBufferSize+piop->nOutBufferSize` to overflow and be less than `PAGE_SIZE`.

In this situation the copy calls would result in a heap overflow.
Comment 1 CTurt 2016-01-25 07:39:05 UTC
These sizes are defined as `DWORD`, a `typedef` for `unsigned int`, rather than a 64bit type like `size_t`, so getting the sum of both sizes to overflow doesn't seem possible.
Comment 2 CTurt 2016-01-25 16:22:16 UTC
Not sure what I was talking about earlier, but I've decompiled the module to produce more easily analysable code:

if ( (unsigned int)(*((_DWORD *)&hptproc_buffer + 4) + *((_DWORD *)&hptproc_buffer + 7)) <= 0x1000 )
  {
    ptr = malloc((unsigned int)(*((_DWORD *)&hptproc_buffer + 4) + *((_DWORD *)&hptproc_buffer + 7)));
    if ( ptr )
    {
      if ( *((_DWORD *)&hptproc_buffer + 4) )
        copyin(*((_QWORD *)&hptproc_buffer + 1), ptr, *((_DWORD *)&hptproc_buffer + 4));

Now it's clear that the comparison on the sum of the two sizes is treated as `unsigned int`.

So, theoretically:

`nInBufferSize` of `0xffffffff`,

`nOutBufferSize` of `1`,

The sum of these two sizes will overflow to `0`, so the check will pass:

if ( (unsigned int)(*((_DWORD *)&hptproc_buffer + 4) + *((_DWORD *)&hptproc_buffer + 7)) <= 0x1000 )

The size passed to `malloc` will also be `0`.

However, `copyin` will copy from `nInBufferSize` of `0xffffffff`.

To exploit this, the size of the allocation could be controlled to manipulate the heap layout to reliably target the overflow.
Comment 3 CTurt 2016-01-25 16:43:19 UTC
PoC code from the above explanation, which results in panic:

https://gist.github.com/CTurt/696a34664bc8d4f4e905
Comment 4 CTurt 2016-01-25 19:38:35 UTC
Supplying the `HPT_IOCTL_GET_EVENT` command will ensure that `Kernel_DeviceIoControl` function instantly returns, resulting in `hpt_set_info` returning straight after doing the `malloc`, `copyin`, and `free`:

	case HPT_IOCTL_GET_EVENT:
		{
			PHPT_EVENT pInfo;

			if (nInBufferSize!=0) return -1;

I've also refined the `size` related parameters needed to fully control the heap overflow:

	params.dwIoControlCode = HPT_IOCTL_GET_EVENT;
	params.lpInBuffer = mapping;
	params.nInBufferSize = bufferSize + overflowSize;
	params.lpOutBuffer = NULL;
	params.nOutBufferSize = -overflowSize;
	params.lpBytesReturned = &bytesReturned;
	
	printf("  [+] nInBufferSize (size copied in): %08x\n", params.nInBufferSize);
	printf("  [+] nOutBufferSize: %08x\n", params.nOutBufferSize);
	printf("  [+] Sum (allocation size): %08x\n", params.nInBufferSize + params.nOutBufferSize);
	printf("  [+] Will be accepted: %d\n", (params.nInBufferSize + params.nOutBufferSize) <= PAGE_SIZE);

You can get very manageable sizes from this, for example, allocated `bufferSize` of `0x500`, and copy size of `0x1000`.

I'll try to get around to writing a full exploit for this soon, but won't be very useful since the vulnerability is only triggerable as `root` anyway.
Comment 5 CTurt 2016-01-30 08:48:45 UTC
I'd also like to add that the result of `copyin` isn't checked here, which can lead to use of initialised heap buffer (it is not allocated with `M_ZERO`).
Comment 6 Shawn Webb 2016-03-01 20:57:19 UTC
Any movement on this?
Comment 7 CTurt 2016-04-07 16:40:25 UTC
Created attachment 169074 [details]
Fix heap overflow and check result of copyin
Comment 8 Sean Bruno freebsd_committer 2016-04-18 20:52:56 UTC
(In reply to CTurt from comment #7)
Is the last bit of the diff just whitespace?
Comment 9 CTurt 2016-04-18 21:22:42 UTC
Yes. This file has some indentation inconsistencies: most parts use tabs, but there are occasional lines which use spaces for indentation; my editor accidentally replaced the spaces with tabs for one of these lines.
Comment 10 commit-hook freebsd_committer 2016-04-18 23:26:35 UTC
A commit references this bug:

Author: sbruno
Date: Mon Apr 18 23:26:11 UTC 2016
New revision: 298231
URL: https://svnweb.freebsd.org/changeset/base/298231

Log:
  hptmv(4) Fix potential buffer overflow in hpt_set_info.

  While here, adjust some whitespace and yeild some useful debug info.

  This is untested on this hardware, testing requests to -scsi went
  unanswered.

  PR:	206585
  Submitted by:	cturt@hardenedbsd.org
  MFC after:	2 weeks

Changes:
  head/sys/dev/hptmv/hptproc.c