From 6588046a645616356004fee95c2649d72edf1ac4 Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Fri, 2 Dec 2022 00:41:24 +0000 Subject: [PATCH 01/12] Report failure when not able to connect to AVRCP A crash may occur when creating a bluetooth AVRCP connection to a device. The code fails to check a return value from an AVRCP function being used to index into an array. The return value may exceed the size of the array causing memory outside the bounds of the array to be accessed leading to memory corruption and a crash. The fix is to ensure the return value is within the bounds of the array before accessing the array contents. If the return value is not within the bounds of the array report it as a failure to the bluetooth stack. This change is relevant for android automotive because the IVI (in-vehicle infotainment system) acts as the an AVRCP controller which still executes this code. Note: this is a backport of b/214569798, inducted as a non-security issue. Per b/226927612 it has been found to have security impact and should be backported to earlier branches. Bug: 226927612 Test: Manual - set return value to be out of bounds, verify no crash Tag: #security Ignore-AOSP-First: Security Change-Id: I03f89f894c759b85e555a024435b625397ef7e5c (cherry picked from commit 6a543761f2dc3db0ebf541285a0b3b2afc83a6a6) Merged-In: I03f89f894c759b85e555a024435b625397ef7e5c --- bta/av/bta_av_act.cc | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/bta/av/bta_av_act.cc b/bta/av/bta_av_act.cc index dcbb18a7b9..771faa6c6c 100644 --- a/bta/av/bta_av_act.cc +++ b/bta/av/bta_av_act.cc @@ -1975,8 +1975,23 @@ void bta_av_rc_disc_done(UNUSED_ATTR tBTA_AV_DATA* p_data) { if (p_lcb) { rc_handle = bta_av_rc_create(p_cb, AVCT_INT, (uint8_t)(p_scb->hdi + 1), p_lcb->lidx); - p_cb->rcb[rc_handle].peer_features = peer_features; - p_cb->rcb[rc_handle].cover_art_psm = cover_art_psm; + if (rc_handle < BTA_AV_NUM_RCB) { + p_cb->rcb[rc_handle].peer_features = peer_features; + p_cb->rcb[rc_handle].cover_art_psm = cover_art_psm; + } else { + /* cannot create valid rc_handle for current device. report failure + */ + APPL_TRACE_ERROR("%s: no link resources available", __func__); + p_scb->use_rc = false; + tBTA_AV_RC_OPEN rc_open; + rc_open.peer_addr = p_scb->PeerAddress(); + rc_open.peer_features = 0; + rc_open.cover_art_psm = 0; + rc_open.status = BTA_AV_FAIL_RESOURCES; + tBTA_AV bta_av_data; + bta_av_data.rc_open = rc_open; + (*p_cb->p_cback)(BTA_AV_RC_OPEN_EVT, &bta_av_data); + } } else { APPL_TRACE_ERROR("%s: can not find LCB!!", __func__); } From 7e3abb6c9b88605697cd40a1a27d5a88a7906e87 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Wed, 28 Dec 2022 00:32:37 +0000 Subject: [PATCH 02/12] Fix an OOB Write bug in gatt_check_write_long_terminate this is the backport of Ifffa2c7f679c4ef72dbdb6b1f3378ca506680084 Bug: 258652631 Test: manual Tag: #security Ignore-AOSP-First: security Change-Id: Ic84122f07cbc198c676d366e39606621b7cb4e66 (cherry picked from commit 9b17660bfd6f0f41cb9400ce0236d76c83605e03) Merged-In: Ic84122f07cbc198c676d366e39606621b7cb4e66 --- stack/gatt/gatt_cl.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/stack/gatt/gatt_cl.cc b/stack/gatt/gatt_cl.cc index 5a20050d11..4e78995688 100644 --- a/stack/gatt/gatt_cl.cc +++ b/stack/gatt/gatt_cl.cc @@ -587,7 +587,8 @@ void gatt_process_prep_write_rsp(tGATT_TCB& tcb, tGATT_CLCB* p_clcb, VLOG(1) << StringPrintf("value resp op_code = %s len = %d", gatt_dbg_op_name(op_code), len); - if (len < GATT_PREP_WRITE_RSP_MIN_LEN) { + if (len < GATT_PREP_WRITE_RSP_MIN_LEN || + len > GATT_PREP_WRITE_RSP_MIN_LEN + sizeof(value.value)) { LOG(ERROR) << "illegal prepare write response length, discard"; gatt_end_operation(p_clcb, GATT_INVALID_PDU, &value); return; @@ -596,7 +597,7 @@ void gatt_process_prep_write_rsp(tGATT_TCB& tcb, tGATT_CLCB* p_clcb, STREAM_TO_UINT16(value.handle, p); STREAM_TO_UINT16(value.offset, p); - value.len = len - 4; + value.len = len - GATT_PREP_WRITE_RSP_MIN_LEN; memcpy(value.value, p, value.len); From e70015e7c1d65cedfb6378119b8ca1dffd0a69c3 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Mon, 2 Jan 2023 22:05:45 +0000 Subject: [PATCH 03/12] Fix an OOB access bug in A2DP_BuildMediaPayloadHeaderSbc In A2DP_BuildCodecHeaderSbc when p_buf->offset is 0, the `-=` operation on it may result in integer underflow and OOB write with the computed pointer passed to A2DP_BuildMediaPayloadHeaderSbc. This is a backport of I45320085b1e458d3b0e0d86162a35aaaae7b34cb Test: atest net_test_stack_a2dp_codecs_native Ignore-AOSP-First: security Tag:#security Bug: 186803518 Change-Id: I4ff1a1de71884b8de23008b2569fdea3650e85ec (cherry picked from commit a710300216be4a86373a65c6a685aeef8509cfa7) Merged-In: I4ff1a1de71884b8de23008b2569fdea3650e85ec --- stack/a2dp/a2dp_sbc.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/stack/a2dp/a2dp_sbc.cc b/stack/a2dp/a2dp_sbc.cc index de55883d23..7abdc58e73 100644 --- a/stack/a2dp/a2dp_sbc.cc +++ b/stack/a2dp/a2dp_sbc.cc @@ -696,6 +696,11 @@ bool A2DP_BuildCodecHeaderSbc(UNUSED_ATTR const uint8_t* p_codec_info, return false; } + // there is a timestamp right following p_buf + if (p_buf->offset < 4 + A2DP_SBC_MPL_HDR_LEN) { + return false; + } + p_buf->offset -= A2DP_SBC_MPL_HDR_LEN; uint8_t* p = (uint8_t*)(p_buf + 1) + p_buf->offset; p_buf->len += A2DP_SBC_MPL_HDR_LEN; From 4ff415454b90473b4629ca1f1d65c077c1f24b85 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Wed, 4 Jan 2023 22:45:13 +0000 Subject: [PATCH 04/12] Fix an OOB write in SDP_AddAttribute When the `attr_pad` becomes full, it is possible that un index of `-1` is computed write a zero byte to `p_val`, rusulting OOB write. ``` p_val[SDP_MAX_PAD_LEN - p_rec->free_pad_ptr - 1] = '\0'; ``` This is a backport of I937d22a2df26fca1d7f06b10182c4e713ddfed1b Bug: 261867748 Test: manual Tag: #security Ignore-AOSP-First: security Change-Id: Ibdda754e628cfc9d1706c14db114919a15d8d6b1 (cherry picked from commit cc527a97f78a2999a0156a579e488afe9e3675b2) Merged-In: Ibdda754e628cfc9d1706c14db114919a15d8d6b1 --- stack/sdp/sdp_db.cc | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/stack/sdp/sdp_db.cc b/stack/sdp/sdp_db.cc index 2b258d25c6..d64a647b37 100644 --- a/stack/sdp/sdp_db.cc +++ b/stack/sdp/sdp_db.cc @@ -349,6 +349,11 @@ bool SDP_AddAttribute(uint32_t handle, uint16_t attr_id, uint8_t attr_type, uint16_t xx, yy, zz; tSDP_RECORD* p_rec = &sdp_cb.server_db.record[0]; + if (p_val == nullptr) { + SDP_TRACE_WARNING("Trying to add attribute with p_val == nullptr, skipped"); + return (false); + } + if (sdp_cb.trace_level >= BT_TRACE_LEVEL_DEBUG) { if ((attr_type == UINT_DESC_TYPE) || (attr_type == TWO_COMP_INT_DESC_TYPE) || @@ -396,6 +401,13 @@ bool SDP_AddAttribute(uint32_t handle, uint16_t attr_id, uint8_t attr_type, if (p_rec->record_handle == handle) { tSDP_ATTRIBUTE* p_attr = &p_rec->attribute[0]; + // error out early, no need to look up + if (p_rec->free_pad_ptr >= SDP_MAX_PAD_LEN) { + SDP_TRACE_ERROR("the free pad for SDP record with handle %d is " + "full, skip adding the attribute", handle); + return (false); + } + /* Found the record. Now, see if the attribute already exists */ for (xx = 0; xx < p_rec->num_attributes; xx++, p_attr++) { /* The attribute exists. replace it */ @@ -435,15 +447,13 @@ bool SDP_AddAttribute(uint32_t handle, uint16_t attr_id, uint8_t attr_type, attr_len = 0; } - if ((attr_len > 0) && (p_val != 0)) { + if (attr_len > 0) { p_attr->len = attr_len; memcpy(&p_rec->attr_pad[p_rec->free_pad_ptr], p_val, (size_t)attr_len); p_attr->value_ptr = &p_rec->attr_pad[p_rec->free_pad_ptr]; p_rec->free_pad_ptr += attr_len; - } else if ((attr_len == 0 && - p_attr->len != - 0) || /* if truncate to 0 length, simply don't add */ - p_val == 0) { + } else if (attr_len == 0 && p_attr->len != 0) { + /* if truncate to 0 length, simply don't add */ SDP_TRACE_ERROR( "SDP_AddAttribute fail, length exceed maximum: ID %d: attr_len:%d ", attr_id, attr_len); From 52cfa0f7c3c01dd53ef44b656a2c68697e44d5e7 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Thu, 8 Dec 2022 01:08:11 +0000 Subject: [PATCH 05/12] Fix OOB access in avdt_scb_hdl_pkt_no_frag This is a back port of the following 2 CLs: - Id13b1ebde8f603123c8b7a49922b2f1378ab788f - If0c7b25f2e6cb4531bbb6254e176e8ad1b5c5fb4 Regression test: I9c87e30ed58e7ad6a34ab7c96b0a8fb06324ad54 Bug: 142546355 258057241 Test: atest net_test_stack_avdtp Ignore-AOSP-First: security Change-Id: Ie1707385d6452ece47915c153f4faaa1c8a287c9 (cherry picked from commit b0b968e8c6214e20a5dc3617d66567225df0884f) Merged-In: Ie1707385d6452ece47915c153f4faaa1c8a287c9 --- stack/avdt/avdt_scb_act.cc | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/stack/avdt/avdt_scb_act.cc b/stack/avdt/avdt_scb_act.cc index 66289f8ea0..b399492234 100644 --- a/stack/avdt/avdt_scb_act.cc +++ b/stack/avdt/avdt_scb_act.cc @@ -257,19 +257,24 @@ void avdt_scb_hdl_pkt_no_frag(AvdtpScb* p_scb, tAVDT_SCB_EVT* p_data) { if (offset > len) goto length_error; p += 2; BE_STREAM_TO_UINT16(ex_len, p); - offset += ex_len * 4; p += ex_len * 4; } + if ((p - p_start) >= len) { + AVDT_TRACE_WARNING("%s: handling malformatted packet: ex_len too large", __func__); + osi_free_and_reset((void**)&p_data->p_pkt); + return; + } + offset = p - p_start; + /* adjust length for any padding at end of packet */ if (o_p) { /* padding length in last byte of packet */ - pad_len = *(p_start + p_data->p_pkt->len); + pad_len = *(p_start + len - 1); } /* do sanity check */ - if ((offset > p_data->p_pkt->len) || - ((pad_len + offset) > p_data->p_pkt->len)) { + if (pad_len >= (len - offset)) { AVDT_TRACE_WARNING("Got bad media packet"); osi_free_and_reset((void**)&p_data->p_pkt); } From 51e179b837d1001fe0219f37bd0ebd4fd30a09d6 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Fri, 2 Dec 2022 02:11:31 +0000 Subject: [PATCH 06/12] Add mocking support for now function in AttributionProcessor The triggering of the code we want to test depends on the return value of std::chrono::system_clock::now(). To facilicate testing, in this patch we add a now_func_ field in AttributionProcessor and make it call it instead of std::chrono::system_clock::now(). Mocking `now` is made possible by passing a custom function to the constructor of AttributionProcessor. Note: 1. This is a manual cherrypick of I7dd3a0e665f72c27e4d1844f45ec15a8dd1ddb53 Bug: 254774758 Test: refactoring, existing tests still pass Ignore-AOSP-First: security Change-Id: I1526d794ddd86f53a189f3ff226bddbff7a487e5 (cherry picked from commit 7378585e68bdccd53e2b713d474229310f0672d0) Merged-In: I1526d794ddd86f53a189f3ff226bddbff7a487e5 --- gd/btaa/attribution_processor.h | 13 +++++++++++++ gd/btaa/linux_generic/attribution_processor.cc | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/gd/btaa/attribution_processor.h b/gd/btaa/attribution_processor.h index 11bbc9cc47..37c7e72c2d 100644 --- a/gd/btaa/attribution_processor.h +++ b/gd/btaa/attribution_processor.h @@ -58,7 +58,20 @@ class AttributionProcessor { void Dump( std::promise> promise, flatbuffers::FlatBufferBuilder* fb_builder); + using ClockType = std::chrono::time_point; + using NowFunc = ClockType (*)(); + + // by default, we use the std::chrono::system_clock::now implementation to + // get the current timestamp + AttributionProcessor() : now_func_(std::chrono::system_clock::now) {} + // in other cases, we may need to use different implementation + // e.g., for testing purposes + AttributionProcessor(NowFunc func) : now_func_(func) {} + private: + // this function is added for testing support in + // OnWakelockReleased + NowFunc now_func_ = std::chrono::system_clock::now; bool wakeup_ = false; std::unordered_map btaa_aggregator_; std::unordered_map wakelock_duration_aggregator_; diff --git a/gd/btaa/linux_generic/attribution_processor.cc b/gd/btaa/linux_generic/attribution_processor.cc index a55243a060..b38bb3a8b5 100644 --- a/gd/btaa/linux_generic/attribution_processor.cc +++ b/gd/btaa/linux_generic/attribution_processor.cc @@ -64,7 +64,7 @@ void AttributionProcessor::OnWakelockReleased(uint32_t duration_ms) { } ms_per_byte = duration_ms / total_byte_count; - auto cur_time = std::chrono::system_clock::now(); + auto cur_time = now_func_(); for (auto& it : wakelock_duration_aggregator_) { it.second.wakelock_duration_ms = ms_per_byte * it.second.byte_count; if (btaa_aggregator_.find(it.first) == btaa_aggregator_.end()) { From cc4174633dcd04857811c345c4c1247a3eafcf94 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Fri, 2 Dec 2022 02:13:40 +0000 Subject: [PATCH 07/12] Add regression test for b/254774758 Note: this is a manual cherrypick of I1709af943b6fa238dd4df41a62e6add36984c9ec Bug: 254774758 Ignore-AOSP-First: security Test: atest bluetooth_test_gd_unit Change-Id: If40eb63e00c1a97e15dcdfdbbf12fad1070cd97b (cherry picked from commit 6059eb91d45d87a8e339bd0928a5837e6a96a324) Merged-In: If40eb63e00c1a97e15dcdfdbbf12fad1070cd97b --- gd/Android.bp | 1 + gd/btaa/Android.bp | 7 ++ .../attribution_processor_tests.cc | 84 +++++++++++++++++++ 3 files changed, 92 insertions(+) create mode 100644 gd/btaa/linux_generic/attribution_processor_tests.cc diff --git a/gd/Android.bp b/gd/Android.bp index c9985a8459..d214a96204 100644 --- a/gd/Android.bp +++ b/gd/Android.bp @@ -335,6 +335,7 @@ cc_test { ":BluetoothShimTestSources", ":BluetoothSecurityUnitTestSources", ":BluetoothStorageUnitTestSources", + ":BluetoothBtaaSources_linux_generic_tests", ], generated_headers: [ "BluetoothGeneratedBundlerSchema_h_bfbs", diff --git a/gd/btaa/Android.bp b/gd/btaa/Android.bp index 2aea7eefb1..6b4c269ba4 100644 --- a/gd/btaa/Android.bp +++ b/gd/btaa/Android.bp @@ -30,3 +30,10 @@ filegroup { "linux_generic/wakelock_processor.cc", ], } + +filegroup { + name: "BluetoothBtaaSources_linux_generic_tests", + srcs: [ + "linux_generic/attribution_processor_tests.cc", + ], +} diff --git a/gd/btaa/linux_generic/attribution_processor_tests.cc b/gd/btaa/linux_generic/attribution_processor_tests.cc new file mode 100644 index 0000000000..e54e7fbe28 --- /dev/null +++ b/gd/btaa/linux_generic/attribution_processor_tests.cc @@ -0,0 +1,84 @@ +/* + * Copyright 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include +#include + +#include +#include +#include +#include + +#include "btaa/activity_attribution.h" +#include "btaa/attribution_processor.h" + +using bluetooth::hci::Address; +using namespace bluetooth::activity_attribution; +using namespace std::chrono; + +// mock for std::chrono::system_clock::now +static AttributionProcessor::ClockType now_ret_val; +static AttributionProcessor::ClockType fake_now() { + return now_ret_val; +} + +class AttributionProcessorTest : public ::testing::Test { + protected: + void SetUp() override { + pAttProc = std::make_unique(fake_now); + } + void TearDown() override { + pAttProc.reset(); + } + + std::unique_ptr pAttProc; +}; + +static void fake_now_set_current() { + now_ret_val = system_clock::now(); +} + +static void fake_now_advance_1000sec() { + now_ret_val += seconds(1000s); +} + +TEST_F(AttributionProcessorTest, UAFInOnWakelockReleasedRegressionTest) { + std::vector btaaPackets; + Address addr; + + fake_now_set_current(); + + // setup the condition 1 for triggering erase operation + // add 220 entries in app_activity_aggregator_ + // and btaa_aggregator_ + for (int i = 0; i < 220; i++) { + std::string addrStr = base::StringPrintf("21:43:65:87:a9:%02x", i + 10); + ASSERT_TRUE(Address::FromString(addrStr, addr)); + BtaaHciPacket packet(Activity::ACL, addr, 30 * i); + btaaPackets.push_back(packet); + } + + pAttProc->OnBtaaPackets(btaaPackets); + pAttProc->OnWakelockReleased(100); + + // setup the condition 2 for triggering erase operation + // make elapsed_time_sec > 900s + fake_now_advance_1000sec(); + + pAttProc->OnBtaaPackets(btaaPackets); + pAttProc->OnWakelockReleased(100); +} From 2bb476b6aabd3abb97823c6fbe3927e159738d06 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Fri, 20 Jan 2023 19:39:30 +0000 Subject: [PATCH 08/12] Fix an OOB bug in register_notification_rsp This is a backport of I901d973a736678d7f3cc816ddf0cbbcbbd1fe93f to rvc-dev. Bug: 245916076 Test: manual Ignore-AOSP-First: security Change-Id: I37a9f45e707702b2ec52b5a2d572f177f2911765 (cherry picked from commit 901e34203c6280d414cbfa3978de04fd6515ffdf) Merged-In: I37a9f45e707702b2ec52b5a2d572f177f2911765 --- btif/src/btif_rc.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/btif/src/btif_rc.cc b/btif/src/btif_rc.cc index c77ba6f55c..31a0cb98e8 100644 --- a/btif/src/btif_rc.cc +++ b/btif/src/btif_rc.cc @@ -1955,6 +1955,11 @@ static bt_status_t register_notification_rsp( dump_rc_notification_event_id(event_id)); std::unique_lock lock(btif_rc_cb.lock); + if (event_id > MAX_RC_NOTIFICATIONS) { + BTIF_TRACE_ERROR("Invalid event id"); + return BT_STATUS_PARM_INVALID; + } + memset(&(avrc_rsp.reg_notif), 0, sizeof(tAVRC_REG_NOTIF_RSP)); avrc_rsp.reg_notif.event_id = event_id; From a4b4a858fff31e4e6096a1fc80e4153e92c9cf6c Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Fri, 2 Dec 2022 02:02:01 +0000 Subject: [PATCH 09/12] Fix a use-after-free bug in AttributionProcessor::OnWakelockReleased There is a use-after-free bug in AttributionProcessor::OnWakelockReleased resulted from a well-known misuse of using iterators to delete items in containers (the deleted items are used for calculating the next iterator in the next round). This patch fix it with correct usage. Note: 1. This is a cherry-pick of If9f14d5fe2fbf2150f2ab0d1f90ce0f263399227 2. The regression test is: If40eb63e00c1a97e15dcdfdbbf12fad1070cd97b Bug: 254774758 Ignore-AOSP-First: security Test: atest bluetooth_test_gd_unit Change-Id: I75576e59e0c81a82473a68a6c5ba3ce882a84f99 (cherry picked from commit 9774aeff84a834ae4403300b5ef88f0a4635e9ac) Merged-In: I75576e59e0c81a82473a68a6c5ba3ce882a84f99 --- gd/btaa/linux_generic/attribution_processor.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/gd/btaa/linux_generic/attribution_processor.cc b/gd/btaa/linux_generic/attribution_processor.cc index b38bb3a8b5..4c0eac7802 100644 --- a/gd/btaa/linux_generic/attribution_processor.cc +++ b/gd/btaa/linux_generic/attribution_processor.cc @@ -91,12 +91,15 @@ void AttributionProcessor::OnWakelockReleased(uint32_t duration_ms) { return; } // Trim down the transient entries in the aggregator to avoid that it overgrows - for (auto& it : btaa_aggregator_) { + auto it = btaa_aggregator_.begin(); + while (it != btaa_aggregator_.end()) { auto elapsed_time_sec = - std::chrono::duration_cast(cur_time - it.second.creation_time).count(); + std::chrono::duration_cast(cur_time - it->second.creation_time).count(); if (elapsed_time_sec > kDurationTransientDeviceActivityEntrySecs && - it.second.byte_count < kByteCountTransientDeviceActivityEntry) { - btaa_aggregator_.erase(it.first); + it->second.byte_count < kByteCountTransientDeviceActivityEntry) { + it = btaa_aggregator_.erase(it); + } else { + it++; } } } From aab624632ec0663e8db8d84058b62ff8855a2fa7 Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Tue, 11 Oct 2022 21:23:22 +0000 Subject: [PATCH 10/12] Prevent use-after-free of HID reports BTA sends the the HID report pointer to BTIF and deallocates it immediately. This is now prevented by providing a deep copy callback function for HID reports when tranferring context from BTA to BTIF. This is a backport of change Icef7a7ed1185b4283ee4fe4f812ca154d8f1b825, already merged on T for b/227620181. Bug: 228837201 Test: Validated against researcher POC, ran BT unit tests, played audio manually. Tag: #security Ignore-AOSP-First: Security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:874c495c886cd8722625756dc5fd0634b16b4f42) Merged-In: Ib837f395883de2369207f1b3b974d6bff02dcb19 Change-Id: Ib837f395883de2369207f1b3b974d6bff02dcb19 --- btif/src/btif_hh.cc | 50 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/btif/src/btif_hh.cc b/btif/src/btif_hh.cc index 9e5db0c1af..b35b2cd498 100644 --- a/btif/src/btif_hh.cc +++ b/btif/src/btif_hh.cc @@ -1092,6 +1092,38 @@ static void btif_hh_upstreams_evt(uint16_t event, char* p_param) { } } +/******************************************************************************* + * + * Function btif_hh_hsdata_rpt_copy_cb + * + * Description Deep copies the tBTA_HH_HSDATA structure + * + * Returns void + * + ******************************************************************************/ + +static void btif_hh_hsdata_rpt_copy_cb(uint16_t event, char* p_dest, + char* p_src) { + tBTA_HH_HSDATA* p_dst_data = (tBTA_HH_HSDATA*)p_dest; + tBTA_HH_HSDATA* p_src_data = (tBTA_HH_HSDATA*)p_src; + BT_HDR* hdr; + + if (!p_src) { + BTIF_TRACE_ERROR("%s: Nothing to copy", __func__); + return; + } + + memcpy(p_dst_data, p_src_data, sizeof(tBTA_HH_HSDATA)); + + hdr = p_src_data->rsp_data.p_rpt_data; + if (hdr != NULL) { + uint8_t* p_data = ((uint8_t*)p_dst_data) + sizeof(tBTA_HH_HSDATA); + memcpy(p_data, hdr, BT_HDR_SIZE + hdr->offset + hdr->len); + + p_dst_data->rsp_data.p_rpt_data = (BT_HDR*)p_data; + } +} + /******************************************************************************* * * Function bte_hh_evt @@ -1105,6 +1137,7 @@ static void btif_hh_upstreams_evt(uint16_t event, char* p_param) { void bte_hh_evt(tBTA_HH_EVT event, tBTA_HH* p_data) { bt_status_t status; int param_len = 0; + tBTIF_COPY_CBACK* p_copy_cback = NULL; if (BTA_HH_ENABLE_EVT == event) param_len = sizeof(tBTA_HH_STATUS); @@ -1116,11 +1149,18 @@ void bte_hh_evt(tBTA_HH_EVT event, tBTA_HH* p_data) { param_len = sizeof(tBTA_HH_CBDATA); else if (BTA_HH_GET_DSCP_EVT == event) param_len = sizeof(tBTA_HH_DEV_DSCP_INFO); - else if ((BTA_HH_GET_PROTO_EVT == event) || (BTA_HH_GET_RPT_EVT == event) || - (BTA_HH_GET_IDLE_EVT == event)) + else if ((BTA_HH_GET_PROTO_EVT == event) || (BTA_HH_GET_IDLE_EVT == event)) + param_len = sizeof(tBTA_HH_HSDATA); + else if (BTA_HH_GET_RPT_EVT == event) { + BT_HDR* hdr = p_data->hs_data.rsp_data.p_rpt_data; param_len = sizeof(tBTA_HH_HSDATA); - else if ((BTA_HH_SET_PROTO_EVT == event) || (BTA_HH_SET_RPT_EVT == event) || - (BTA_HH_VC_UNPLUG_EVT == event) || (BTA_HH_SET_IDLE_EVT == event)) + + if (hdr != NULL) { + p_copy_cback = btif_hh_hsdata_rpt_copy_cb; + param_len += BT_HDR_SIZE + hdr->offset + hdr->len; + } + } else if ((BTA_HH_SET_PROTO_EVT == event) || (BTA_HH_SET_RPT_EVT == event) || + (BTA_HH_VC_UNPLUG_EVT == event) || (BTA_HH_SET_IDLE_EVT == event)) param_len = sizeof(tBTA_HH_CBDATA); else if ((BTA_HH_ADD_DEV_EVT == event) || (BTA_HH_RMV_DEV_EVT == event)) param_len = sizeof(tBTA_HH_DEV_INFO); @@ -1129,7 +1169,7 @@ void bte_hh_evt(tBTA_HH_EVT event, tBTA_HH* p_data) { /* switch context to btif task context (copy full union size for convenience) */ status = btif_transfer_context(btif_hh_upstreams_evt, (uint16_t)event, - (char*)p_data, param_len, NULL); + (char*)p_data, param_len, p_copy_cback); /* catch any failed context transfers */ ASSERTC(status == BT_STATUS_SUCCESS, "context transfer failed", status); From 792d2156befcbc8230682ca91042fbc332103284 Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Tue, 21 Mar 2023 22:35:08 +0000 Subject: [PATCH 11/12] Revert^2 "Validate buffer length in sdpu_build_uuid_seq" fd2ded7341c7f867a153e86f003758808f11bfb9 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:4d33899d2c0573cf351691cdf27628416621f545) Merged-In: I40ea9f3858215f460e6dab3768e0c6d2155e4755 Change-Id: I40ea9f3858215f460e6dab3768e0c6d2155e4755 --- stack/sdp/sdp_discovery.cc | 58 ++++++++++++++++++++++++++++++++++---- 1 file changed, 53 insertions(+), 5 deletions(-) diff --git a/stack/sdp/sdp_discovery.cc b/stack/sdp/sdp_discovery.cc index fc53fc7408..72f2413a0f 100644 --- a/stack/sdp/sdp_discovery.cc +++ b/stack/sdp/sdp_discovery.cc @@ -72,10 +72,15 @@ static uint8_t* add_attr(uint8_t* p, uint8_t* p_end, tSDP_DISCOVERY_DB* p_db, * ******************************************************************************/ static uint8_t* sdpu_build_uuid_seq(uint8_t* p_out, uint16_t num_uuids, - Uuid* p_uuid_list) { + Uuid* p_uuid_list, uint16_t& bytes_left) { uint16_t xx; uint8_t* p_len; + if (bytes_left < 2) { + DCHECK(0) << "SDP: No space for data element header"; + return (p_out); + } + /* First thing is the data element header */ UINT8_TO_BE_STREAM(p_out, (DATA_ELE_SEQ_DESC_TYPE << 3) | SIZE_IN_NEXT_BYTE); @@ -83,9 +88,20 @@ static uint8_t* sdpu_build_uuid_seq(uint8_t* p_out, uint16_t num_uuids, p_len = p_out; p_out += 1; + /* Account for data element header and length */ + bytes_left -= 2; + /* Now, loop through and put in all the UUID(s) */ for (xx = 0; xx < num_uuids; xx++, p_uuid_list++) { int len = p_uuid_list->GetShortestRepresentationSize(); + + if (len + 1 > bytes_left) { + DCHECK(0) << "SDP: Too many UUIDs for internal buffer"; + break; + } else { + bytes_left -= (len + 1); + } + if (len == Uuid::kNumBytes16) { UINT8_TO_BE_STREAM(p_out, (UUID_DESC_TYPE << 3) | SIZE_TWO_BYTES); UINT16_TO_BE_STREAM(p_out, p_uuid_list->As16Bit()); @@ -122,6 +138,7 @@ static void sdp_snd_service_search_req(tCONN_CB* p_ccb, uint8_t cont_len, uint8_t *p, *p_start, *p_param_len; BT_HDR* p_cmd = (BT_HDR*)osi_malloc(SDP_DATA_BUF_SIZE); uint16_t param_len; + uint16_t bytes_left = SDP_DATA_BUF_SIZE; /* Prepare the buffer for sending the packet to L2CAP */ p_cmd->offset = L2CAP_MIN_OFFSET; @@ -136,9 +153,24 @@ static void sdp_snd_service_search_req(tCONN_CB* p_ccb, uint8_t cont_len, p_param_len = p; p += 2; -/* Build the UID sequence. */ + /* Account for header size, max service record count and + * continuation state */ + const uint16_t base_bytes = (sizeof(BT_HDR) + L2CAP_MIN_OFFSET + + 3u + /* service search request header */ + 2u + /* param len */ + 3u + ((p_cont) ? cont_len : 0)); + + if (base_bytes > bytes_left) { + DCHECK(0) << "SDP: Overran SDP data buffer"; + osi_free(p_cmd); + return; + } + + bytes_left -= base_bytes; + + /* Build the UID sequence. */ p = sdpu_build_uuid_seq(p, p_ccb->p_db->num_uuid_filters, - p_ccb->p_db->uuid_filters); + p_ccb->p_db->uuid_filters, bytes_left); /* Set max service record count */ UINT16_TO_BE_STREAM(p, sdp_cb.max_recs_per_search); @@ -572,6 +604,7 @@ static void process_service_search_attr_rsp(tCONN_CB* p_ccb, uint8_t* p_reply, if ((cont_request_needed) || (!p_reply)) { BT_HDR* p_msg = (BT_HDR*)osi_malloc(SDP_DATA_BUF_SIZE); uint8_t* p; + uint16_t bytes_left = SDP_DATA_BUF_SIZE; p_msg->offset = L2CAP_MIN_OFFSET; p = p_start = (uint8_t*)(p_msg + 1) + L2CAP_MIN_OFFSET; @@ -585,9 +618,24 @@ static void process_service_search_attr_rsp(tCONN_CB* p_ccb, uint8_t* p_reply, p_param_len = p; p += 2; -/* Build the UID sequence. */ + /* Account for header size, max service record count and + * continuation state */ + const uint16_t base_bytes = (sizeof(BT_HDR) + L2CAP_MIN_OFFSET + + 3u + /* service search request header */ + 2u + /* param len */ + 3u + /* max service record count */ + ((p_reply) ? (*p_reply) : 0)); + + if (base_bytes > bytes_left) { + sdp_disconnect(p_ccb, SDP_INVALID_CONT_STATE); + return; + } + + bytes_left -= base_bytes; + + /* Build the UID sequence. */ p = sdpu_build_uuid_seq(p, p_ccb->p_db->num_uuid_filters, - p_ccb->p_db->uuid_filters); + p_ccb->p_db->uuid_filters, bytes_left); /* Max attribute byte count */ UINT16_TO_BE_STREAM(p, sdp_cb.max_attr_list_size); From 4e9947742d50d490693ba501bee5db3222b2319f Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Tue, 21 Mar 2023 22:39:16 +0000 Subject: [PATCH 12/12] Revert "Revert "Fix wrong BR/EDR link key downgrades (P_256->P_192)"" This reverts commit d733c86cbc06ce0ec72216b9d41e172d1939c46f. Function btm_sec_encrypt_change() is called at most places with argument "encr_enable" treated as bool and not as per (tHCI_ENCRYPT_MODE = 0/1/2) expected by the function. The function has special handling for "encr_enable=1" to downgrade the link key type for BR/EDR case. This gets executed even when the caller/context did not mean/expect so. It appears this handling in btm_sec_encrypt_change() is not necessary and is removed by this commit to prevent accidental execution of it. Test: Verified re-pairing with an iPhone works fine now Issue Reproduction Steps: 1. Enable Bluetooth Hotspot on Android device (DUT). 2. Pair and connect an iPhone to DUT. 3. Forget this pairing on DUT. 4. On iPhone settings, click on old DUT's paired entry to connect. 5. iPhone notifies to click 'Forget Device' and try fresh pairing. 6. On iPhone, after doing 'Forget Device', discover DUT again. 7. Attempt pairing to DUT by clicking on discovered DUT entry. Pairing will be unsuccessful. Issue Cause: During re-pairing, DUT is seen to downgrade BR/EDR link key unexpectedly from link key type 0x8 (BTM_LKEY_TYPE_AUTH_COMB_P_256) to 0x5 (BTM_LKEY_TYPE_AUTH_COMB). Log snippet (re-pairing time): btm_sec_link_key_notification set new_encr_key_256 to 1 btif_dm_auth_cmpl_evt: Storing link key. key_type=0x8, bond_type=1 btm_sec_encrypt_change new_encr_key_256 is 1 --On DUT, HCI_Encryption_Key_Refresh_Complete event noticed--- btm_sec_encrypt_change new_encr_key_256 is 0 updated link key type to 5 btif_dm_auth_cmpl_evt: Storing link key. key_type=0x5, bond_type=1 This is a backport of the following patch: aosp/1890096 Bug: 258834033 Reason for revert: Reinstate original change for QPR (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:56891eedc68c86b40977191dad28d65ebf86a94f) Merged-In: Iba0c220b82bcf6b15368762b7052a3987ccbc0c6 Change-Id: Iba0c220b82bcf6b15368762b7052a3987ccbc0c6 --- stack/btm/btm_sec.cc | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/stack/btm/btm_sec.cc b/stack/btm/btm_sec.cc index 4b5d70d699..b3d9fb2a91 100644 --- a/stack/btm/btm_sec.cc +++ b/stack/btm/btm_sec.cc @@ -3300,22 +3300,6 @@ void btm_sec_encrypt_change(uint16_t handle, tHCI_STATUS status, BTM_TRACE_DEBUG("%s start SM over BR/EDR", __func__); SMP_BR_PairWith(p_dev_rec->bd_addr); } - } else { - // BR/EDR is successfully encrypted. Correct LK type if needed - // (BR/EDR LK derived from LE LTK was used for encryption) - if ((encr_enable == 1) && /* encryption is ON for SSP */ - /* LK type is for BR/EDR SC */ - (p_dev_rec->link_key_type == BTM_LKEY_TYPE_UNAUTH_COMB_P_256 || - p_dev_rec->link_key_type == BTM_LKEY_TYPE_AUTH_COMB_P_256)) { - if (p_dev_rec->link_key_type == BTM_LKEY_TYPE_UNAUTH_COMB_P_256) - p_dev_rec->link_key_type = BTM_LKEY_TYPE_UNAUTH_COMB; - else /* BTM_LKEY_TYPE_AUTH_COMB_P_256 */ - p_dev_rec->link_key_type = BTM_LKEY_TYPE_AUTH_COMB; - - BTM_TRACE_DEBUG("updated link key type to %d", - p_dev_rec->link_key_type); - btm_send_link_key_notif(p_dev_rec); - } } }