1066582 - b2g Bluetooth integer signedness bug

Status: RESOLVED INVALID

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[banty]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0
Build ID: 20140716183446

Steps to reproduce:

While auditing the bluetooth ipc code I came across an insufficient buffer length check in the ParseHeaders() function located at

http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/ObexBase.cpp

bool
ParseHeaders(const uint8_t* aHeaderStart,
             int aTotalLength,
             ObexHeaderSet* aRetHandlerSet)
{
  const uint8_t* ptr = aHeaderStart;

  while (ptr - aHeaderStart < aTotalLength) {
    ObexHeaderId headerId = (ObexHeaderId)*ptr++;

    uint16_t contentLength = 0;
    uint8_t highByte, lowByte;

    // Defined in 2.1 OBEX Headers, IrOBEX 1.2
    switch (headerId >> 6)
    {
      case 0x00:
        // Null-terminated Unicode text, length prefixed with 2-byte
        // unsigned integer.
      case 0x01:
        // byte sequence, length prefixed with 2 byte unsigned integer.
        highByte = *ptr++;
        lowByte = *ptr++;
        contentLength = (((uint16_t)highByte << 8) | lowByte) - 3;  //underflow
        break;

      case 0x02:
        // 1 byte quantity
        contentLength = 1;
        break;

      case 0x03:
        // 4 byte quantity
        contentLength = 4;
        break;
    }

    // Length check to prevent from memory pollusion.
    if (ptr + contentLength > aHeaderStart + aTotalLength) {  //signed variable makes this a signed comparison???   
      // Severe error occurred. We can't even believe the received data, so
      // clear all headers.
      MOZ_ASSERT(false);
      aRetHandlerSet->ClearHeaders();
      return false;
    }

    aRetHandlerSet->AddHeader(new ObexHeader(headerId, contentLength, ptr)); 

    ptr += contentLength;
  }

  return true;
}



Actual results:

'aTotalLength' is a signed integer variable. When it is added to aHeaderStart the comparison will become signed. Since ptr and aHeaderStart are memory addresses bad things may happen if either gets above address 0x80000000. We also have control of 'contentLength' which could help trigger signedness issue and possibly bypass the length check.

If triggered out of bounds reads will occur here
aRetHandlerSet->AddHeader(new ObexHeader(headerId, contentLength, ptr));

Leading to DoS and maybe info leak?


Expected results:

Comparisons involving pointer locations should be unsigned

Comment 1

[Frederik Braun [:freddy]]

Bruce, Marco: Can you help with triage?

Comment 2

[banty]

After taking another look at the B2G_DEBUG binary I realize I made some mistakes in my initial report.

The first check:
'(ptr - aHeaderStart < aTotalLength)'
is a signed comparison thanks to 'aTotalLength'

however the second:
'if (ptr + contentLength > aHeaderStart + aTotalLength)'
remains unsigned.

Additionally the potential underflow: 
'contentLength = (((uint16_t)highByte << 8) | lowByte) - 3;'
is also unexploitable since the top 16 bits get cleared.

Taking all that into account, I now believe there is no bug here and we can close this