Cordio BLE: ignore messages with overlong headers (CVE-2024-48986)

[Diff-fusion]

Summary of changes

hciEvtProcessMsg parses incoming hci command packets. In doing so, it dynamically determines the length of the packet body and the event type by reading 2 bytes from the packet header.

• mbed-os/connectivity/FEATURE_BLE/libraries/cordio_stack/ble-host/sources/hci/dual_chip/hci_evt.c

  Lines 2747 to 2748 in 54e8693

  	BSTREAM_TO_UINT8(evt, pEvt);
  	BSTREAM_TO_UINT8(len, pEvt);

Depending on the event type, it will set a callback flag. If this flag is set later, it allocates a buffer with a semi-constant length determined by an event type to buffer length lookup table.
It then copies bytes into the allocated buffer but uses the packet supplied length value to copy them.

• mbed-os/connectivity/FEATURE_BLE/libraries/cordio_stack/ble-host/sources/hci/dual_chip/hci_evt.c

  Lines 3017 to 3025 in 54e8693

  	if ((pMsg = WsfBufAlloc(hciEvtCbackLen[cbackEvt])) != NULL)
  	{
  	/* initialize message header */
  	pMsg->hdr.param = 0;
  	pMsg->hdr.event = cbackEvt;
  	pMsg->hdr.status = HCI_SUCCESS;
  	/* execute parsing function for the event */
  	(*hciEvtParseFcnTbl[cbackEvt])(pMsg, pEvt, len);

This leads to a buffer overflow.

This fix addresses the issue by only copying as many bytes as fit in the buffer.

Impact of changes

Migration actions required

Documentation

None

---

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

---

[Diff-fusion]

This PR fixes CVE-2024-48986

[Diff-fusion]

I've looked through the code again and noticed a couple of things:

1. The len parameter for any of the parser functions is the length of the input byte stream:
   

   mbed-os/connectivity/FEATURE_BLE/libraries/cordio_stack/ble-host/sources/hci/dual_chip/hci_evt.c

   Lines 398 to 400 in 86472d7

   	* \param pMsg Pointer to output event message structure.
   	* \param p Pointer to input HCI event parameter byte stream.
   	* \param len Parameter byte stream length.

   
   Which means that the current implementation is correct.
2. The only function using the len parameter is hciEvtParseVendorSpec.
   

   mbed-os/connectivity/FEATURE_BLE/libraries/cordio_stack/ble-host/sources/hci/dual_chip/hci_evt.c

   Lines 748 to 760 in 86472d7

   	* \brief Parse an HCI event.
   	*
   	* \param pMsg Pointer to output event message structure.
   	* \param p Pointer to input HCI event parameter byte stream.
   	* \param len Parameter byte stream length.
   	*
   	* \return None.
   	*/
   	/*************************************************************************************************/
   	static void hciEvtParseVendorSpec(hciEvt_t *pMsg, uint8_t *p, uint8_t len)
   	{
   	memcpy(pMsg->vendorSpec.param, p, len);
   	}

   
   This function copies the entire byte stream into a 1 byte sized buffer in the hciVendorSpecEvt_t struct.
   

   mbed-os/connectivity/FEATURE_BLE/libraries/cordio_stack/ble-host/include/hci_api.h

   Lines 410 to 415 in 86472d7

   	/*! \brief Vendor specific event */
   	typedef struct
   	{
   	wsfMsgHdr_t hdr; /*!< \brief Event header. */
   	uint8_t param[1]; /*!< \brief Vendor specific event. */
   	} hciVendorSpecEvt_t;

   
   It seems that this overflow is the actual bug here and must be fixed in a different way.

The fix requires that the size of the allocation in hciEvtProcessMsg must depend on the length of the event.
I'll propose a new fix that takes this into account.

[multiplemonomials]

Minor correction to above, it's not a 1 byte sized buffer in hciVendorSpecEvt_t, it's a "flexible array member". Basically it's used to trick the C compiler into allowing an array of arbitrary length in memory after the end of the hciVendorSpecEvt_t structure, which can be accessed through its param member.