Buffer overflow vulnerablilty in MbedOS BLE Cordio stack

[fireknight-hJ]

Description of defect

Reference: https://github.com/ARMmbed/mbed-os/blob/master/connectivity/FEATURE_BLE/source/cordio/stack_adaptation/hci_tr.c

Function: hciTrSerialRxIncoming

From: mbed-os/blob/master/connectivity/FEATURE_BLE/source/cordio/stack_adaptation/hci_tr.c Line: 125

mbed-os/connectivity/FEATURE_BLE/source/cordio/stack_adaptation/hci_tr.c

Line 125 in 7c7d20d

void hciTrSerialRxIncoming(uint8_t *pBuf, uint8_t len)

Type: Buffer overflow

The BLE Cordio implementation in Mbed OS utilizes the hciTrSerialRxIncoming function to manage incoming HCI data. However, I have identified and verified a potential issue that could lead to a buffer overflow in hdrRx if packet types are excluded from the valid ones.

To elaborate, when an invalid packet type is encountered, thehdrlen (line 36) remain the inital value (i.e.,0) but the iRxhas been increased (Line 19). Consequently, the condition in Line 41 is not satisfied and the stateRx variable remains in the HCI_RX_STATE_HEADER state. This, in turn, allows incoming data to continuously accumulate in the hdrRx buffer in while loop execution, as shown in line 19. However, it's important to note that the hdrRx's size is constrained by the HCI_ACL_HDR_LEN macro, which is set to a mere 4 bytes. This causes a vulnerability to buffer overflow.

void hciTrSerialRxIncoming(uint8_t *pBuf, uint8_t len)
{
  ......
  static uint8_t    hdrRx[HCI_ACL_HDR_LEN];
  ......
  /* loop until all bytes of incoming buffer are handled */
  while (len--)
  {
    /* read single byte from incoming buffer and advance to next byte */
    dataByte = *pBuf++;
    ......
    /* --- Header State --- */
    else if (stateRx == HCI_RX_STATE_HEADER)
    {
      uint8_t  hdrLen = 0;
      uint16_t dataLen = 0;

      /* copy current byte into the temp header buffer */
      hdrRx[iRx++] = dataByte; /*vulnerbility occured: the buffer overflow problem occured here*/

      /* determine header length based on packet type */
      // pkIndRx is the first byte
      switch (pktIndRx)
      {
        case HCI_CMD_TYPE:
          hdrLen = HCI_CMD_HDR_LEN;
          break;
        case HCI_ACL_TYPE:
          hdrLen = HCI_ACL_HDR_LEN;
          break;
        case HCI_EVT_TYPE:
          hdrLen = HCI_EVT_HDR_LEN;
          break;
        default:
          /* invalid packet type */
          WSF_ASSERT(0); 
          break;
      }
      ......
      /* see if entire header has been read */
      if (iRx == hdrLen)
      {
        /* extract data length from header */
        switch (pktIndRx)
        {
          case HCI_CMD_TYPE:
            dataLen = hdrRx[2];
            break;
          case HCI_ACL_TYPE:
            BYTES_TO_UINT16(dataLen, &hdrRx[2]);
            break;
          case HCI_EVT_TYPE:
            dataLen = hdrRx[1];
            break;
          default:
            break;
        }

        /* allocate data buffer to hold entire packet */
        if (pktIndRx == HCI_ACL_TYPE)
        {
          pPktRx = (uint8_t*)WsfMsgDataAlloc(hdrLen + dataLen, 0);
        }
        else
        {
          pPktRx = (uint8_t*)WsfMsgAlloc(hdrLen + dataLen);
        }

        if (pPktRx != NULL)
        {
          pDataRx = pPktRx;

          /* copy header into data packet (note: memcpy is not so portable) */
          {
            uint8_t  i;
            for (i = 0; i < hdrLen; i++)
            {
              *pDataRx++ = hdrRx[i];
            }
          }

          /* save number of bytes left to read */
          iRx = dataLen;
          if (iRx == 0)
          {
            stateRx = HCI_RX_STATE_COMPLETE;
          }
          else
          {
            stateRx = HCI_RX_STATE_DATA;
          }
        }
        else
        {
          WSF_ASSERT(0); /* allocate falied */
        }
  }
}

In addition, note that WSF_ASSERT is turned off by default. However, even if the WSF_ASSERT is turn on the execution will be simple return or directly hang which depends on how the mbed_error function works, as shown its following defination.

#if WSF_ASSERT_ENABLED == TRUE
#if WSF_TOKEN_ENABLED == TRUE
#define WSF_ASSERT(expr)      if (!(expr)) {WsfAssert(MODULE_ID, (uint16_t) __LINE__);}
#else
#define WSF_ASSERT(expr)      if (!(expr)) {WsfAssert(__FILE__, (uint16_t) __LINE__);}
#endif
#else
#define WSF_ASSERT(expr)      (void)(expr);
#endif

#if WSF_TOKEN_ENABLED == TRUE
void WsfAssert(uint16_t modId, uint16_t line)
#else
void WsfAssert(const char *pFile, uint16_t line)
#endif
{
  ......
  PalSysAssertTrap();
}

#define MBED_ERROR( error_status, error_msg )                     mbed_error( error_status, (const char *)error_msg, (uint32_t)0          , NULL, 0 )

MBED_WEAK void PalSysAssertTrap()
{
    MBED_ERROR(function_not_implemented, "Provide implementation of PalSysAssertTrap");
}

mbed_error_status_t mbed_error(mbed_error_status_t error_status, const char *error_msg, unsigned int error_value, const char *filename, int line_number)
{
    return 0;
}

Target(s) affected by this defect ?

MbedOS BLE Cordio stack

Toolchain(s) (name and version) displaying this defect ?

N/A

What version of Mbed-os are you using (tag or sha) ?

mbed-os-6.17.0 (the latest version)

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

mbed-cli2

How is this defect reproduced ?

Send problematic HCI protocol packets to the target demo board using the Cordio protocol stack.

• wrong hci packet type

12 00 00 00 71 A1 4B D7 B1 1F 98 E2 85 B2 B3 05 84 34 EA FA D7 67 BD A8 D2 25 1D B2 20 82 CF E5 2F 72 08 4C 03 C6 0A 02 8F 25 2D 63 30 58 45 59 18 A7 11 11 8C B4 F1 C6 33 C5 0B 5D AA E3 7E 2C DE 25 01 F3 84 00 75 D2 39 20 11 10 7C 0B 35 05 D1 86 5B 3A D0 C7 58 19 7E 33 1E B0 C7 FF 10 0C A9 0E 9D 7B A0 53 7C A8 58 0B 02 70 A8 AF D1 25 FD 64 15 1B EB A3

[0xc0170]

Hi, @fireknight-hJ would you create a pull request to fix this issue?

[fireknight-hJ]

Hi, @0xc0170 I've fixed the issue and created a pull request (#15474) for your review. Please take a look and let me know if everything is in order.