[bug] ble_audio_broadcast_sink configuration does not bound num_subgroups before writing sink->subgroups
- Dominant language
- C
- Stars
- 893
- Forks
- 512
- Avg merge
- 13d 31m
- Merged PRs (30d)
- 7
Description
````
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
````
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by reading the configuration write path in nimble/host/audio/src/ble_audio_broadcast_sink.c and compare its fixed subgroup capacity with sync_opt->num_subgroups. Then inspect the producer path in ble_audio_scan_delegator.c and the ble_audio_scan_delegator_sync_opt definition. Done means oversized subgroup counts are rejected before either fixed array is written.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- embedded-iot
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 58/100