Fix l2cap_channels OOB in get_channel_for_cid #2081
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
While the bounds check is performed on
l2cap_channel_count, only the lower byte is checked due to the uint8_t data type. Afterwards, the expression foriis re-calculated without the explicit data type, which means without truncation into 8 bits. Instead, the full 16-bit value is used as the channel index. This pointer will point out-of-bounds of the channel array.This leads to a large 16 out-of-bounds channel id to be used as an index into the array for large CID inputs.
Sample value: 0xff41 -> 0xff41 - 0x41 = 0xff00 == 0x00 (as uint8_t)
This results in out-of-bounds memory to be interpreted as a channel object, and this memory to potentially be corrupted in the following call to
input_l2cap_frame_flow_channel, which fills the out-of-bounds memory (the supposed channel object) with user-supplied data.The fix here is to re-use the
ivariable as an index, which was used to perform the bounds check, and is truncated to 8 bits.As a side note, due to the unsignedness of the data type
uint8_t i, the checki >= 0is always true. Instead, a signed data type such asint16_tcould be used to make the check take effect.