New issue
Advanced search Search tips

Issue 1625 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 18
Cc:



Sign in to add a comment

iOS/MacOS kernel memory corruption due to integer overflow in IOHIDResourceQueue::enqueueReport

Project Member Reported by ianbeer@google.com, Jul 31

Issue description

IOHIDResourceQueue inherits from IOSharedDataQueue and adds its own ::enqueueReport method,
which seems to be mostly copy-pasted from IOSharedDataQueue and IODataQueue's ::enqueue methods.

I reported a bunch of integer overflows in IODataQueue over four years ago (CVE-2014-4389, apple issue 607452866)

IOHIDResourceQueue::enqueueReport has basically the same bug:

Boolean IOHIDResourceQueue::enqueueReport(IOHIDResourceDataQueueHeader * header, IOMemoryDescriptor * report)
{
    UInt32              headerSize  = sizeof(IOHIDResourceDataQueueHeader);
    UInt32              reportSize  = report ? (UInt32)report->getLength() : 0;
    UInt32              dataSize    = ALIGNED_DATA_SIZE(headerSize + reportSize, sizeof(uint32_t));   <--- (a)
    UInt32              head;
    UInt32              tail;
    UInt32              newTail;
    const UInt32        entrySize   = dataSize + DATA_QUEUE_ENTRY_HEADER_SIZE;
    IODataQueueEntry *  entry;

    // Force a single read of head and tail
    head = __c11_atomic_load((_Atomic UInt32 *)&dataQueue->head, __ATOMIC_RELAXED);
    tail = __c11_atomic_load((_Atomic UInt32 *)&dataQueue->tail, __ATOMIC_RELAXED);

    if ( tail > getQueueSize() || head > getQueueSize() || dataSize < headerSize || entrySize < dataSize)   <--- (b)
    {
        return false;
    }

    if ( tail >= head )
    {
        // Is there enough room at the end for the entry?
        if ((getQueueSize() - tail) >= entrySize )
        {
            entry = (IODataQueueEntry *)((UInt8 *)dataQueue->queue + tail);

            entry->size = dataSize;

            bcopy(header, &entry->data, headerSize);

            if ( report )
                report->readBytes(0, ((UInt8*)&entry->data) + headerSize, reportSize);     <--- (c)




Report is the IOMemoryDescriptor which wraps the stucture input to the io_connect_call, it's wrapping a portion 
of userspace so we can actually make an IOMemoryDescriptor with a length of 0xffffffff. This will overflow at (a)
giving us a small value for dataSize. This will pass the checks at (b) but then the reportSize value is used at (c)
for the actually memory write operation.

The IOHIDResource is used when userspace wants to implement an HID device; to exploit this you need there to actually be one
of these devices. If you have the com.apple.hid.manager.user-access-device entitlement you can create one of these.

A bunch of daemons do possess this entitlement, for example bluetoothd needs it to implement bluetooth HID keyboards,
so if you have a bluetooth keyboard connected you can trigger this bug without needing com.apple.hid.manager.user-access-device.)

You can test this PoC either by connecting a bluetooth HID device, or by building the IOHIDResource keyboard example
from the IOHIDFamily code, giving it the correct entitlement and running it.
 
Project Member

Comment 1 by ianbeer@google.com, Jul 31

Labels: Reported-2018-Jul-31 Id-696255847
Project Member

Comment 2 by ianbeer@google.com, Oct 18

Labels: Fixed-2018-Sept-17
Status: Fixed (was: New)
This issue has been fixed in iOS 12/MacOS 10.14 but Apple have not assigned a CVE or mentioned it in their security bulletins.
hid_client.c
5.0 KB View Download
Project Member

Comment 3 by ianbeer@google.com, Oct 18

Labels: -Restrict-View-Commit
explore/topics/objective-c/index.md

Sign in to add a comment