Skip to content

[bug] ble_audio_broadcast_sink configuration does not bound num_subgroups before writing sink->subgroups #2188

@neosys007

Description

@neosys007

Hello,

I reviewed current upstream head and found a current-head bounds issue in the broadcast sink configuration path. I am deliberately reporting it as a runtime/API robustness bug, not overclaiming it as a generic remote exploit.

The sink object in `nimble/host/audio/src/ble_audio_broadcast_sink.c` contains:

```c
uint8_t num_subgroups;
struct {
    uint32_t bis_sync;
} subgroups[MYNEWT_VAL(BLE_AUDIO_SCAN_DELEGATOR_SUBGROUP_MAX)];
SLIST_ENTRY(ble_audio_broadcast_sink) next;
```

The write path later does:

```c
sink->num_subgroups = sync_opt->num_subgroups;

for (uint8_t subgroup_index = 0; subgroup_index < sink->num_subgroups; subgroup_index++) {
    sink->subgroups[subgroup_index].bis_sync =
        sync_opt->subgroups[subgroup_index].bis_sync;
}
```

What matters here is that there is no runtime check in this sink-side path that `sync_opt->num_subgroups` is within the fixed `sink->subgroups[]` capacity before the loop runs.

I also checked the producer side in `ble_audio_scan_delegator.c`. The relevant code is:

```c
sync_opt = &action.source_modify.sync_opt;
sync_opt_init(sync_opt, op->modify_source.pa_sync, op->modify_source.pa_interval, NULL, 0);

BLE_AUDIO_DBG_ASSERT(sync_opt->num_subgroups < ARRAY_SIZE(sync_opt->subgroups));

for (uint8_t i = 0; i < sync_opt->num_subgroups; i++) {
    sync_opt->subgroups[i].bis_sync = op->modify_source.subgroups[i].bis_sync_state;
    ...
}
```

and `struct ble_audio_scan_delegator_sync_opt` itself also uses a fixed subgroup array:

```c
uint8_t num_subgroups;
struct ble_audio_scan_delegator_subgroup subgroups[
    BLE_AUDIO_SCAN_DELEGATOR_SUBGROUP_MAX];
```

So the only visible protection on this path is a debug assertion, not a hard runtime bound that survives production builds.

That is why I think the real current-head claim is:

- the broadcast sink configuration path relies on an invariant that is only asserted in debug,
- but still uses `num_subgroups` as a runtime write bound into fixed arrays,
- making the current code fragile and memory-unsafe if the invariant is violated on a real caller path.

I am not overclaiming the trigger surface. My claim is simply that current head is missing a required runtime bound check before writing `sink->subgroups[]`.

A straightforward fix would be to reject any `num_subgroups` larger than the array capacity before copying into either `sync_opt->subgroups[]` or `sink->subgroups[]`.

Best regards
Pengpeng Hou
ISCAS

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions