diff options
| author | Brian Delwiche <delwiche@google.com> | 2023-05-23 23:09:48 +0000 |
|---|---|---|
| committer | Julian Veit <Claymore1298@gmail.com> | 2024-01-09 07:17:03 +0100 |
| commit | 1818968925214a3f7dc5a9a11280c5ce34d1fbb0 (patch) | |
| tree | 551d6d86e3fd4e93627a986302ae78ffb45763d9 | |
| parent | 91e75155c948fbfdd7f747b4df01732bf0e03aaf (diff) | |
Fix some OOB errors in BTM parsing
Some HCI BLE events are missing bounds checks, leading to possible OOB
access. Add the appropriate bounds checks on the packets.
Bug: 279169188
Test: atest bluetooth_test_gd_unit, net_test_stack_btm
Tag: #security
Ignore-AOSP-First: Security
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:66e2be0585514de92e8a31df09ab31528fd67e20)
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:5d1a3febede9f835797cf5feff978a9f007f2593)
Merged-In: If7752f6edd749d6d5a4bb957b4824c22b5602737
Change-Id: If7752f6edd749d6d5a4bb957b4824c22b5602737
| -rw-r--r-- | system/stack/btm/btm_ble_gap.cc | 62 | ||||
| -rw-r--r-- | system/stack/btu/btu_hcif.cc | 6 |
2 files changed, 54 insertions, 14 deletions
diff --git a/system/stack/btm/btm_ble_gap.cc b/system/stack/btm/btm_ble_gap.cc index e781bca8d2..9fba1a5af5 100644 --- a/system/stack/btm/btm_ble_gap.cc +++ b/system/stack/btm/btm_ble_gap.cc @@ -1433,6 +1433,16 @@ void btm_ble_biginfo_adv_report_rcvd(uint8_t* p, uint16_t param_len) { uint16_t sync_handle, iso_interval, max_pdu, max_sdu; uint8_t num_bises, nse, bn, pto, irc, phy, framing, encryption; uint32_t sdu_interval; + + // 2 bytes for sync handle, 1 byte for num_bises, 1 byte for nse, 2 bytes for + // iso_interval, 1 byte each for bn, pto, irc, 2 bytes for max_pdu, 3 bytes + // for sdu_interval, 2 bytes for max_sdu, 1 byte each for phy, framing, + // encryption + if (param_len < 19) { + LOG_ERROR("Insufficient data"); + return; + } + STREAM_TO_UINT16(sync_handle, p); STREAM_TO_UINT8(num_bises, p); STREAM_TO_UINT8(nse, p); @@ -2562,20 +2572,27 @@ void btm_ble_process_ext_adv_pkt(uint8_t data_len, const uint8_t* data) { advertising_sid; int8_t rssi, tx_power; uint16_t event_type, periodic_adv_int, direct_address_type; + size_t bytes_to_process; /* Only process the results if the inquiry is still active */ if (!btm_cb.ble_ctr_cb.is_ble_scan_active()) return; + bytes_to_process = 1; + + if (data_len < bytes_to_process) { + LOG(ERROR) << "Malformed LE extended advertising packet: not enough room " + "for num reports"; + return; + } + /* Extract the number of reports in this event. */ STREAM_TO_UINT8(num_reports, p); - constexpr int extended_report_header_size = 24; while (num_reports--) { - if (p + extended_report_header_size > data + data_len) { - // TODO(jpawlowski): we should crash the stack here - BTM_TRACE_ERROR( - "Malformed LE Extended Advertising Report Event from controller - " - "can't loop the data"); + bytes_to_process += 24; + if (data_len < bytes_to_process) { + LOG(ERROR) << "Malformed LE extended advertising packet: not enough room " + "for metadata"; return; } @@ -2595,8 +2612,11 @@ void btm_ble_process_ext_adv_pkt(uint8_t data_len, const uint8_t* data) { const uint8_t* pkt_data = p; p += pkt_data_len; /* Advance to the the next packet*/ - if (p > data + data_len) { - LOG(ERROR) << "Invalid pkt_data_len: " << +pkt_data_len; + + bytes_to_process += pkt_data_len; + if (data_len < bytes_to_process) { + LOG(ERROR) << "Malformed LE extended advertising packet: not enough room " + "for packet data"; return; } @@ -2629,18 +2649,28 @@ void btm_ble_process_adv_pkt(uint8_t data_len, const uint8_t* data) { const uint8_t* p = data; uint8_t legacy_evt_type, addr_type, num_reports, pkt_data_len; int8_t rssi; + size_t bytes_to_process; /* Only process the results if the inquiry is still active */ if (!btm_cb.ble_ctr_cb.is_ble_scan_active()) return; + bytes_to_process = 1; + + if (data_len < bytes_to_process) { + LOG(ERROR) + << "Malformed LE advertising packet: not enough room for num reports"; + return; + } + /* Extract the number of reports in this event. */ STREAM_TO_UINT8(num_reports, p); - constexpr int report_header_size = 10; while (num_reports--) { - if (p + report_header_size > data + data_len) { - // TODO(jpawlowski): we should crash the stack here - BTM_TRACE_ERROR("Malformed LE Advertising Report Event from controller"); + bytes_to_process += 9; + + if (data_len < bytes_to_process) { + LOG(ERROR) + << "Malformed LE advertising packet: not enough room for metadata"; return; } @@ -2652,8 +2682,12 @@ void btm_ble_process_adv_pkt(uint8_t data_len, const uint8_t* data) { const uint8_t* pkt_data = p; p += pkt_data_len; /* Advance to the the rssi byte */ - if (p > data + data_len - sizeof(rssi)) { - LOG(ERROR) << "Invalid pkt_data_len: " << +pkt_data_len; + + // include rssi for this check + bytes_to_process += pkt_data_len + 1; + if (data_len < bytes_to_process) { + LOG(ERROR) << "Malformed LE advertising packet: not enough room for " + "packet data and/or RSSI"; return; } diff --git a/system/stack/btu/btu_hcif.cc b/system/stack/btu/btu_hcif.cc index ec05df2e13..f00effe87d 100644 --- a/system/stack/btu/btu_hcif.cc +++ b/system/stack/btu/btu_hcif.cc @@ -1693,6 +1693,12 @@ static void btu_ble_data_length_change_evt(uint8_t* p, uint16_t evt_len) { return; } + // 2 bytes each for handle, tx_data_len, TxTimer, rx_data_len + if (evt_len < 8) { + LOG_ERROR("Event packet too short"); + return; + } + STREAM_TO_UINT16(handle, p); STREAM_TO_UINT16(tx_data_len, p); p += 2; /* Skip the TxTimer */ |
