zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.82k stars 6.59k forks source link

Bluetooth: HCI: Handle truncated packets in hci_le_big_complete and hci_le_big_sync_established #80597

Open ekleezg opened 5 days ago

ekleezg commented 5 days ago

In the function hci_le_big_complete and hci_le_big_sync_established,

    SYS_SLIST_FOR_EACH_CONTAINER(&big->bis_channels, bis, node) {
        const uint16_t handle = evt->handle[i++];
        struct bt_conn *iso_conn = bis->iso;

        iso_conn->handle = sys_le16_to_cpu(handle);
        store_bis_broadcaster_info(evt, &iso_conn->iso.info);
        bt_conn_set_state(iso_conn, BT_CONN_CONNECTED);
    }

It appears that there’s no handling for cases where the evt message might be truncated.

I believe that check should be added to verify that the number of evt->handle entries matches the number of bis_channels.

It doesn't seem to be a security issue, but improving it would help prevent bugs.

henrikbrixandersen commented 1 day ago

Please use our bug template when reporting bugs. You need to edit this issue to include the information requested in https://github.com/zephyrproject-rtos/zephyr/blob/main/.github/ISSUE_TEMPLATE/001_bug_report.md

Thalley commented 1 day ago

In the function hci_le_big_complete and hci_le_big_sync_established,

  SYS_SLIST_FOR_EACH_CONTAINER(&big->bis_channels, bis, node) {
      const uint16_t handle = evt->handle[i++];
      struct bt_conn *iso_conn = bis->iso;

      iso_conn->handle = sys_le16_to_cpu(handle);
      store_bis_broadcaster_info(evt, &iso_conn->iso.info);
      bt_conn_set_state(iso_conn, BT_CONN_CONNECTED);
  }

It appears that there’s no handling for cases where the evt message might be truncated.

I believe that check should be added to verify that the number of evt->handle entries matches the number of bis_channels.

It doesn't seem to be a security issue, but improving it would help prevent bugs.

Can you elaborate on what you mean by truncated?

AFAIK it's not valid for the controller to truncate events. That being said, the size of the event is verified (just the length field) I believe, and we do validate that the num_bis in both events correspond to the number we expected before we do the SYS_SLIST_FOR_EACH_CONTAINER.

Have you seen this occur? If so, then please provide instructions on how to reproduce