apache / apache/mynewt-nimble

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

Open
#2,188 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.