From 7b30443dac7bb9138275c909549110191bcbcae9 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Fri, 28 Apr 2023 02:42:22 +0000 Subject: [PATCH 01/31] Fix multiple OOB bugs in btm_ble_gap.cc Bug: 275057843 Bug: 275057678 Test: manual Tag: #security Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:3bb913ee8c7da4602798db754045c0fac57afecf) Merged-In: I4c8ec50c15e2727839a49da0e582164557bcd38a Change-Id: I4c8ec50c15e2727839a49da0e582164557bcd38a --- system/stack/btm/btm_ble_gap.cc | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/system/stack/btm/btm_ble_gap.cc b/system/stack/btm/btm_ble_gap.cc index b4e52f62f16..7083c88b35b 100644 --- a/system/stack/btm/btm_ble_gap.cc +++ b/system/stack/btm/btm_ble_gap.cc @@ -916,6 +916,12 @@ void btm_ble_start_sync_request(uint8_t sid, RawAddress addr, uint16_t skip, uint8_t options = 0; uint8_t cte_type = 7; int index = btm_ble_get_psync_index(sid, addr); + + if (index == MAX_SYNC_TRANSACTION) { + LOG_ERROR("Failed to get sync transfer index"); + return; + } + tBTM_BLE_PERIODIC_SYNC* p = &btm_ble_pa_sync_cb.p_sync[index]; p->sync_state = PERIODIC_SYNC_PENDING; @@ -989,6 +995,11 @@ static void btm_ble_start_sync_timeout(void* data) { int index = btm_ble_get_psync_index(adv_sid, address); + if (index == MAX_SYNC_TRANSACTION) { + LOG_ERROR("Failed to get sync transfer index"); + return; + } + tBTM_BLE_PERIODIC_SYNC* p = &btm_ble_pa_sync_cb.p_sync[index]; if (BleScanningManager::IsInitialized()) { @@ -1192,11 +1203,14 @@ void BTM_BleStartPeriodicSync(uint8_t adv_sid, RawAddress address, SyncLostCb lostCb) { LOG_DEBUG("%s", "[PSync]"); int index = btm_ble_get_free_psync_index(); - tBTM_BLE_PERIODIC_SYNC* p = &btm_ble_pa_sync_cb.p_sync[index]; + if (index == MAX_SYNC_TRANSACTION) { syncCb.Run(BTM_NO_RESOURCES, 0, adv_sid, BLE_ADDR_RANDOM, address, 0, 0); return; } + + tBTM_BLE_PERIODIC_SYNC* p = &btm_ble_pa_sync_cb.p_sync[index]; + p->in_use = true; p->remote_bda = address; p->sid = adv_sid; @@ -1328,6 +1342,12 @@ void BTM_BlePeriodicSyncTransfer(RawAddress addr, uint16_t service_data, } int index = btm_ble_get_free_sync_transfer_index(); + if (index == MAX_SYNC_TRANSACTION) { + BTM_TRACE_ERROR("Failed to get sync transfer index"); + cb.Run(BTM_ILLEGAL_VALUE, addr); + return; + } + tBTM_BLE_PERIODIC_SYNC_TRANSFER* p_sync_transfer = &btm_ble_pa_sync_cb.sync_transfer[index]; p_sync_transfer->in_use = true; @@ -1367,6 +1387,12 @@ void BTM_BlePeriodicSyncSetInfo(RawAddress addr, uint16_t service_data, } int index = btm_ble_get_free_sync_transfer_index(); + if (index == MAX_SYNC_TRANSACTION) { + BTM_TRACE_ERROR("Failed to get sync transfer index"); + cb.Run(BTM_ILLEGAL_VALUE, addr); + return; + } + tBTM_BLE_PERIODIC_SYNC_TRANSFER* p_sync_transfer = &btm_ble_pa_sync_cb.sync_transfer[index]; p_sync_transfer->in_use = true; -- GitLab From 25a7d9aaceea0f7d6cb4ae3da5aa66efb0bc7db8 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Fri, 25 Aug 2023 17:15:38 -0700 Subject: [PATCH 02/31] Reject access to secure service authenticated from a temp bonding [1] Rejecct access to services running on l2cap Backport of Idef4ea28eb3d17b0807ab7dc6849433ddc5581b3 Bug: 294854926 Test: m com.android.btservices Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:232f4f81a9774196f688e956f50084514110798a) Merged-In: Idef4ea28eb3d17b0807ab7dc6849433ddc5581b3 Change-Id: Idef4ea28eb3d17b0807ab7dc6849433ddc5581b3 --- system/stack/btm/btm_sec.cc | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/system/stack/btm/btm_sec.cc b/system/stack/btm/btm_sec.cc index 4a83cde8842..92ede3616a9 100644 --- a/system/stack/btm/btm_sec.cc +++ b/system/stack/btm/btm_sec.cc @@ -207,6 +207,25 @@ static bool btm_dev_16_digit_authenticated(tBTM_SEC_DEV_REC* p_dev_rec) { return (false); } +/******************************************************************************* + * + * Function access_secure_service_from_temp_bond + * + * Description a utility function to test whether an access to + * secure service from temp bonding is happening + * + * Returns true if the aforementioned condition holds, + * false otherwise + * + ******************************************************************************/ +static bool access_secure_service_from_temp_bond(const tBTM_SEC_DEV_REC* p_dev_rec, + bool locally_initiated, + uint16_t security_req) { + return !locally_initiated && (security_req & BTM_SEC_IN_AUTHENTICATE) && + p_dev_rec->is_device_authenticated() && + p_dev_rec->is_bond_type_temporary(); +} + /******************************************************************************* * * Function BTM_SecRegister @@ -1609,9 +1628,14 @@ tBTM_STATUS btm_sec_l2cap_access_req_by_requirement( } if (rc == BTM_SUCCESS) { + if (access_secure_service_from_temp_bond(p_dev_rec, is_originator, security_required)) { + LOG_ERROR("Trying to access a secure service from a temp bonding, rejecting"); + rc = BTM_FAILED_ON_SECURITY; + } + if (p_callback) - (*p_callback)(&bd_addr, transport, (void*)p_ref_data, BTM_SUCCESS); - return (BTM_SUCCESS); + (*p_callback)(&bd_addr, transport, (void*)p_ref_data, rc); + return (rc); } } @@ -4430,6 +4454,13 @@ tBTM_STATUS btm_sec_execute_procedure(tBTM_SEC_DEV_REC* p_dev_rec) { return (BTM_FAILED_ON_SECURITY); } + if (access_secure_service_from_temp_bond(p_dev_rec, + p_dev_rec->IsLocallyInitiated(), + p_dev_rec->security_required)) { + LOG_ERROR("Trying to access a secure service from a temp bonding, rejecting"); + return (BTM_FAILED_ON_SECURITY); + } + /* All required security procedures already established */ p_dev_rec->security_required &= ~(BTM_SEC_OUT_AUTHENTICATE | BTM_SEC_IN_AUTHENTICATE | -- GitLab From f4e439c22354f0aa868a982bc88bcc9de3bc37f7 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Mon, 28 Aug 2023 15:59:32 -0700 Subject: [PATCH 03/31] Reject access to secure services authenticated from temp bonding [2] Reject access to service running on rfcomm this is a backport of I10fcc2dcd78fc22ffbe3c425669fc9889b94a166 Bug: 294854926 Test: m com.android.btservices Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:9878a84e7eebb49ba994a9bbdd2258ecf4b3abb8) Merged-In: I10fcc2dcd78fc22ffbe3c425669fc9889b94a166 Change-Id: I10fcc2dcd78fc22ffbe3c425669fc9889b94a166 --- system/stack/btm/btm_sec.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/system/stack/btm/btm_sec.cc b/system/stack/btm/btm_sec.cc index 92ede3616a9..d19a952b61b 100644 --- a/system/stack/btm/btm_sec.cc +++ b/system/stack/btm/btm_sec.cc @@ -1898,6 +1898,11 @@ tBTM_STATUS btm_sec_mx_access_request(const RawAddress& bd_addr, security_required, p_callback, p_ref_data); } else /* rc == BTM_SUCCESS */ { + if (access_secure_service_from_temp_bond(p_dev_rec, + is_originator, security_required)) { + LOG_ERROR("Trying to access a secure rfcomm service from a temp bonding, reject"); + rc = BTM_FAILED_ON_SECURITY; + } if (p_callback) { LOG_DEBUG("Notifying client that security access has been granted"); (*p_callback)(&bd_addr, transport, p_ref_data, rc); -- GitLab From a99edb35d6c044dbd607a74b88102bf2f36d5ef5 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Tue, 5 Sep 2023 14:04:39 -0700 Subject: [PATCH 04/31] Reject access to secure service authenticated from a temp bonding [3] Allow access to rfcomm PSM by default Original bug Bug: 294854926 Nearby regressions: Bug: 298539299 Test: m com.android.btservices Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:9e4cef217f1d1e11fb7b74765ec17200e618bc24) Merged-In: If1f7c9278a9e877f64ae78b6f067c597fb5d0e66 Change-Id: If1f7c9278a9e877f64ae78b6f067c597fb5d0e66 --- system/stack/btm/btm_sec.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/system/stack/btm/btm_sec.cc b/system/stack/btm/btm_sec.cc index d19a952b61b..d50d69a43d8 100644 --- a/system/stack/btm/btm_sec.cc +++ b/system/stack/btm/btm_sec.cc @@ -1651,15 +1651,15 @@ tBTM_STATUS btm_sec_l2cap_access_req_by_requirement( btm_cb.security_mode == BTM_SEC_MODE_SC) { if (BTM_SEC_IS_SM4(p_dev_rec->sm4)) { if (is_originator) { - /* SM4 to SM4 -> always authenticate & encrypt */ - security_required |= (BTM_SEC_OUT_AUTHENTICATE | BTM_SEC_OUT_ENCRYPT); + /* SM4 to SM4 -> always encrypt */ + security_required |= BTM_SEC_OUT_ENCRYPT; } else /* acceptor */ { /* SM4 to SM4: the acceptor needs to make sure the authentication is * already done */ chk_acp_auth_done = true; - /* SM4 to SM4 -> always authenticate & encrypt */ - security_required |= (BTM_SEC_IN_AUTHENTICATE | BTM_SEC_IN_ENCRYPT); + /* SM4 to SM4 -> always encrypt */ + security_required |= BTM_SEC_IN_ENCRYPT; } } else if (!(BTM_SM4_KNOWN & p_dev_rec->sm4)) { /* the remote features are not known yet */ -- GitLab From 9194524a92e0f5859caeab1ff487d21d9b513d0b Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Fri, 8 Sep 2023 17:11:10 +0000 Subject: [PATCH 05/31] Reorganize the code for checking auth requirement Original bug Bug: 294854926 regressions: Bug: 299570702 Test: Test: m com.android.btservices Test: QA validation (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:6bacbe908e8ba71422badc6ebff47d3f021e8824) Merged-In: I976a5a6d7bb819fd6accdc71eb1501b9606f3ae4 Change-Id: I976a5a6d7bb819fd6accdc71eb1501b9606f3ae4 --- system/stack/btm/btm_sec.cc | 95 ++++++++++++++++++++++--------------- 1 file changed, 56 insertions(+), 39 deletions(-) diff --git a/system/stack/btm/btm_sec.cc b/system/stack/btm/btm_sec.cc index d50d69a43d8..d2111a7de10 100644 --- a/system/stack/btm/btm_sec.cc +++ b/system/stack/btm/btm_sec.cc @@ -4390,48 +4390,65 @@ tBTM_STATUS btm_sec_execute_procedure(tBTM_SEC_DEV_REC* p_dev_rec) { /* If connection is not authenticated and authentication is required */ /* start authentication and return PENDING to the caller */ - if ((((!(p_dev_rec->sec_flags & BTM_SEC_AUTHENTICATED)) && - ((p_dev_rec->IsLocallyInitiated() && - (p_dev_rec->security_required & BTM_SEC_OUT_AUTHENTICATE)) || - (!p_dev_rec->IsLocallyInitiated() && - (p_dev_rec->security_required & BTM_SEC_IN_AUTHENTICATE)))) || - (!(p_dev_rec->sec_flags & BTM_SEC_16_DIGIT_PIN_AUTHED) && - (!p_dev_rec->IsLocallyInitiated() && - (p_dev_rec->security_required & BTM_SEC_IN_MIN_16_DIGIT_PIN)))) && - (p_dev_rec->hci_handle != HCI_INVALID_HANDLE)) { - /* - * We rely on BTM_SEC_16_DIGIT_PIN_AUTHED being set if MITM is in use, - * as 16 DIGIT is only needed if MITM is not used. Unfortunately, the - * BTM_SEC_AUTHENTICATED is used for both MITM and non-MITM - * authenticated connections, hence we cannot distinguish here. - */ - - LOG_DEBUG("Security Manager: Start authentication"); + if (p_dev_rec->hci_handle != HCI_INVALID_HANDLE) { + bool start_auth = false; + + // Check link status of BR/EDR + if (!(p_dev_rec->sec_flags & BTM_SEC_AUTHENTICATED)) { + if (p_dev_rec->IsLocallyInitiated()) { + if (p_dev_rec->security_required & BTM_SEC_OUT_AUTHENTICATE) { + LOG_DEBUG("Outgoing authentication Required"); + start_auth = true; + } + } else { + if (p_dev_rec->security_required & BTM_SEC_IN_AUTHENTICATE) { + LOG_DEBUG("Incoming authentication Required"); + start_auth = true; + } + } + } - /* - * If we do have a link-key, but we end up here because we need an - * upgrade, then clear the link-key known and authenticated flag before - * restarting authentication. - * WARNING: If the controller has link-key, it is optional and - * recommended for the controller to send a Link_Key_Request. - * In case we need an upgrade, the only alternative would be to delete - * the existing link-key. That could lead to very bad user experience - * or even IOP issues, if a reconnect causes a new connection that - * requires an upgrade. - */ - if ((p_dev_rec->sec_flags & BTM_SEC_LINK_KEY_KNOWN) && - (!(p_dev_rec->sec_flags & BTM_SEC_16_DIGIT_PIN_AUTHED) && - (!p_dev_rec->IsLocallyInitiated() && - (p_dev_rec->security_required & BTM_SEC_IN_MIN_16_DIGIT_PIN)))) { - p_dev_rec->sec_flags &= - ~(BTM_SEC_LINK_KEY_KNOWN | BTM_SEC_LINK_KEY_AUTHED | - BTM_SEC_AUTHENTICATED); + if (!(p_dev_rec->sec_flags & BTM_SEC_16_DIGIT_PIN_AUTHED)) { + /* + * We rely on BTM_SEC_16_DIGIT_PIN_AUTHED being set if MITM is in use, + * as 16 DIGIT is only needed if MITM is not used. Unfortunately, the + * BTM_SEC_AUTHENTICATED is used for both MITM and non-MITM + * authenticated connections, hence we cannot distinguish here. + */ + if (!p_dev_rec->IsLocallyInitiated()) { + if (p_dev_rec->security_required & BTM_SEC_IN_MIN_16_DIGIT_PIN) { + LOG_DEBUG("BTM_SEC_IN_MIN_16_DIGIT_PIN Required"); + start_auth = true; + } + } } - btm_sec_wait_and_start_authentication(p_dev_rec); - return (BTM_CMD_STARTED); - } else { - LOG_DEBUG("Authentication not required"); + if (start_auth) { + LOG_DEBUG("Security Manager: Start authentication"); + + /* + * If we do have a link-key, but we end up here because we need an + * upgrade, then clear the link-key known and authenticated flag before + * restarting authentication. + * WARNING: If the controller has link-key, it is optional and + * recommended for the controller to send a Link_Key_Request. + * In case we need an upgrade, the only alternative would be to delete + * the existing link-key. That could lead to very bad user experience + * or even IOP issues, if a reconnect causes a new connection that + * requires an upgrade. + */ + if ((p_dev_rec->sec_flags & BTM_SEC_LINK_KEY_KNOWN) && + (!(p_dev_rec->sec_flags & BTM_SEC_16_DIGIT_PIN_AUTHED) && + (!p_dev_rec->IsLocallyInitiated() && + (p_dev_rec->security_required & BTM_SEC_IN_MIN_16_DIGIT_PIN)))) { + p_dev_rec->sec_flags &= + ~(BTM_SEC_LINK_KEY_KNOWN | BTM_SEC_LINK_KEY_AUTHED | + BTM_SEC_AUTHENTICATED); + } + + btm_sec_wait_and_start_authentication(p_dev_rec); + return (BTM_CMD_STARTED); + } } /* If connection is not encrypted and encryption is required */ -- GitLab From 5673b3c6bbe8c6c9edb8afb5e9499dc3a41d3943 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Fri, 8 Sep 2023 10:26:33 -0700 Subject: [PATCH 06/31] Enforce authentication if encryption is required Original bug Bug: 294854926 regressions: Bug: 299570702 Bug: 299561281 Test: m com.android.btservices Test: QA validation Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:0a8c39cda12639f0b08f5ca79bff6b5515ab20d9) Merged-In: I0370ed2e3166d56f708e1981c2126526e1db9eaa Change-Id: I0370ed2e3166d56f708e1981c2126526e1db9eaa --- system/stack/btm/btm_sec.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/system/stack/btm/btm_sec.cc b/system/stack/btm/btm_sec.cc index d2111a7de10..c77c40535fb 100644 --- a/system/stack/btm/btm_sec.cc +++ b/system/stack/btm/btm_sec.cc @@ -4396,13 +4396,15 @@ tBTM_STATUS btm_sec_execute_procedure(tBTM_SEC_DEV_REC* p_dev_rec) { // Check link status of BR/EDR if (!(p_dev_rec->sec_flags & BTM_SEC_AUTHENTICATED)) { if (p_dev_rec->IsLocallyInitiated()) { - if (p_dev_rec->security_required & BTM_SEC_OUT_AUTHENTICATE) { - LOG_DEBUG("Outgoing authentication Required"); + if (p_dev_rec->security_required & + (BTM_SEC_OUT_AUTHENTICATE | BTM_SEC_OUT_ENCRYPT)) { + LOG_DEBUG("Outgoing authentication/encryption Required"); start_auth = true; } } else { - if (p_dev_rec->security_required & BTM_SEC_IN_AUTHENTICATE) { - LOG_DEBUG("Incoming authentication Required"); + if (p_dev_rec->security_required & + (BTM_SEC_IN_AUTHENTICATE | BTM_SEC_IN_ENCRYPT)) { + LOG_DEBUG("Incoming authentication/encryption Required"); start_auth = true; } } -- GitLab From ea81185c89097500559d61b3d49fb9633899e848 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Wed, 16 Aug 2023 15:07:58 -0700 Subject: [PATCH 07/31] Factor out duplicate code for parsing gap data This change is intended to be used to factor out dup code for parsing GapData in StartAdvertisingSet and make it easier to be tested. Backport of Ia39886c415218353b6f9d59d7d3f6d1160477d6c Bug: 296291440 Test: atest net_test_main_shim (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:08690d66322386d506818b298ad067622d4d5686) Merged-In: Ia39886c415218353b6f9d59d7d3f6d1160477d6c Change-Id: Ia39886c415218353b6f9d59d7d3f6d1160477d6c --- system/main/Android.bp | 1 + system/main/shim/Android.bp | 3 +- system/main/shim/BUILD.gn | 1 + system/main/shim/le_advertising_manager.cc | 109 ++------------------- system/main/shim/utils.cc | 40 ++++++++ system/main/shim/utils.h | 32 ++++++ 6 files changed, 85 insertions(+), 101 deletions(-) create mode 100644 system/main/shim/utils.cc create mode 100644 system/main/shim/utils.h diff --git a/system/main/Android.bp b/system/main/Android.bp index 8f8a245a095..e64f62d3055 100644 --- a/system/main/Android.bp +++ b/system/main/Android.bp @@ -188,6 +188,7 @@ cc_test { "shim/metrics_api.cc", "shim/shim.cc", "shim/stack.cc", + "shim/utils.cc", "test/common_stack_test.cc", "test/main_shim_dumpsys_test.cc", "test/main_shim_test.cc", diff --git a/system/main/shim/Android.bp b/system/main/shim/Android.bp index b8aca32fa18..ce39d300ac3 100644 --- a/system/main/shim/Android.bp +++ b/system/main/shim/Android.bp @@ -29,5 +29,6 @@ filegroup { "metrics_api.cc", "shim.cc", "stack.cc", - ] + "utils.cc", + ], } diff --git a/system/main/shim/BUILD.gn b/system/main/shim/BUILD.gn index 8362dd8c95f..2c6a88b8648 100644 --- a/system/main/shim/BUILD.gn +++ b/system/main/shim/BUILD.gn @@ -35,6 +35,7 @@ source_set("LibBluetoothShimSources") { "metrics_api.cc", "shim.cc", "stack.cc", + "utils.cc", ] include_dirs = [ diff --git a/system/main/shim/le_advertising_manager.cc b/system/main/shim/le_advertising_manager.cc index dc8f81a8328..0fec4e0b020 100644 --- a/system/main/shim/le_advertising_manager.cc +++ b/system/main/shim/le_advertising_manager.cc @@ -17,6 +17,7 @@ #define LOG_TAG "bt_shim_advertiser" #include "le_advertising_manager.h" +#include "utils.h" #include #include @@ -43,6 +44,7 @@ using bluetooth::hci::AddressType; using bluetooth::hci::ErrorCode; using bluetooth::hci::GapData; using bluetooth::hci::OwnAddressType; +using bluetooth::shim::parse_gap_data; using std::vector; namespace { @@ -88,23 +90,8 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface, void SetData(int advertiser_id, bool set_scan_rsp, vector data, StatusCallback cb) override { LOG(INFO) << __func__ << " in shim layer"; - - size_t offset = 0; std::vector advertising_data = {}; - - while (offset < data.size()) { - GapData gap_data; - uint8_t len = data[offset]; - auto begin = data.begin() + offset; - auto end = begin + len + 1; // 1 byte for len - auto data_copy = std::make_shared>(begin, end); - bluetooth::packet::PacketView packet( - data_copy); - GapData::Parse(&gap_data, packet.begin()); - advertising_data.push_back(gap_data); - offset += len + 1; // 1 byte for len - } - + parse_gap_data(data, advertising_data); bluetooth::shim::GetAdvertising()->SetData(advertiser_id, set_scan_rsp, advertising_data); } @@ -128,33 +115,8 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface, bluetooth::hci::ExtendedAdvertisingConfig config{}; parse_parameter(config, params); - size_t offset = 0; - while (offset < advertise_data.size()) { - GapData gap_data; - uint8_t len = advertise_data[offset]; - auto begin = advertise_data.begin() + offset; - auto end = begin + len + 1; // 1 byte for len - auto data_copy = std::make_shared>(begin, end); - bluetooth::packet::PacketView packet( - data_copy); - GapData::Parse(&gap_data, packet.begin()); - config.advertisement.push_back(gap_data); - offset += len + 1; // 1 byte for len - } - - offset = 0; - while (offset < scan_response_data.size()) { - GapData gap_data; - uint8_t len = scan_response_data[offset]; - auto begin = scan_response_data.begin() + offset; - auto end = begin + len + 1; // 1 byte for len - auto data_copy = std::make_shared>(begin, end); - bluetooth::packet::PacketView packet( - data_copy); - GapData::Parse(&gap_data, packet.begin()); - config.scan_response.push_back(gap_data); - offset += len + 1; // 1 byte for len - } + parse_gap_data(advertise_data, config.advertisement); + parse_gap_data(scan_response_data, config.scan_response); bluetooth::shim::GetAdvertising()->StartAdvertising( advertiser_id, config, timeout_s * 100, cb, timeout_cb, scan_callback, @@ -180,47 +142,9 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface, periodic_params.periodic_advertising_properties; config.periodic_advertising_parameters = periodic_parameters; - size_t offset = 0; - while (offset < advertise_data.size()) { - GapData gap_data; - uint8_t len = advertise_data[offset]; - auto begin = advertise_data.begin() + offset; - auto end = begin + len + 1; // 1 byte for len - auto data_copy = std::make_shared>(begin, end); - bluetooth::packet::PacketView packet( - data_copy); - GapData::Parse(&gap_data, packet.begin()); - config.advertisement.push_back(gap_data); - offset += len + 1; // 1 byte for len - } - - offset = 0; - while (offset < scan_response_data.size()) { - GapData gap_data; - uint8_t len = scan_response_data[offset]; - auto begin = scan_response_data.begin() + offset; - auto end = begin + len + 1; // 1 byte for len - auto data_copy = std::make_shared>(begin, end); - bluetooth::packet::PacketView packet( - data_copy); - GapData::Parse(&gap_data, packet.begin()); - config.scan_response.push_back(gap_data); - offset += len + 1; // 1 byte for len - } - - offset = 0; - while (offset < periodic_data.size()) { - GapData gap_data; - uint8_t len = periodic_data[offset]; - auto begin = periodic_data.begin() + offset; - auto end = begin + len + 1; // 1 byte for len - auto data_copy = std::make_shared>(begin, end); - bluetooth::packet::PacketView packet( - data_copy); - GapData::Parse(&gap_data, packet.begin()); - config.periodic_data.push_back(gap_data); - offset += len + 1; // 1 byte for len - } + parse_gap_data(advertise_data, config.advertisement); + parse_gap_data(scan_response_data, config.scan_response); + parse_gap_data(periodic_data, config.periodic_data); bluetooth::hci::AdvertiserId id = bluetooth::shim::GetAdvertising()->ExtendedCreateAdvertiser( @@ -249,23 +173,8 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface, void SetPeriodicAdvertisingData(int advertiser_id, std::vector data, StatusCallback cb) override { LOG(INFO) << __func__ << " in shim layer"; - - size_t offset = 0; std::vector advertising_data = {}; - - while (offset < data.size()) { - GapData gap_data; - uint8_t len = data[offset]; - auto begin = data.begin() + offset; - auto end = begin + len + 1; // 1 byte for len - auto data_copy = std::make_shared>(begin, end); - bluetooth::packet::PacketView packet( - data_copy); - GapData::Parse(&gap_data, packet.begin()); - advertising_data.push_back(gap_data); - offset += len + 1; // 1 byte for len - } - + parse_gap_data(data, advertising_data); bluetooth::shim::GetAdvertising()->SetPeriodicData(advertiser_id, advertising_data); } diff --git a/system/main/shim/utils.cc b/system/main/shim/utils.cc new file mode 100644 index 00000000000..dcf1725beb1 --- /dev/null +++ b/system/main/shim/utils.cc @@ -0,0 +1,40 @@ +/* + * Copyright 2023 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 "utils.h" + +namespace bluetooth { +namespace shim { +void parse_gap_data(const std::vector &raw_data, + std::vector &output) { + size_t offset = 0; + while (offset < raw_data.size()) { + hci::GapData gap_data; + uint8_t len = raw_data[offset]; + + auto begin = raw_data.begin() + offset; + auto end = begin + len + 1; // 1 byte for len + auto data_copy = std::make_shared>(begin, end); + bluetooth::packet::PacketView packet( + data_copy); + hci::GapData::Parse(&gap_data, packet.begin()); + output.push_back(gap_data); + offset += len + 1; // 1 byte for len + } +} + +} // namespace shim +} // namespace bluetooth diff --git a/system/main/shim/utils.h b/system/main/shim/utils.h new file mode 100644 index 00000000000..56da2a0a0ae --- /dev/null +++ b/system/main/shim/utils.h @@ -0,0 +1,32 @@ +/* + * Copyright 2023 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. + */ +#pragma once +#include + +#include "hci/hci_packets.h" + +namespace bluetooth { +namespace shim { +/** + * @brief Parsing gap data from raw bytes + * + * @param raw_data input, raw bytes + * @param output vector of GapData + */ +void parse_gap_data(const std::vector &raw_data, + std::vector &output); +} // namespace shim +} // namespace bluetooth -- GitLab From a218e5be5e4049eae3b321f2a535a128d65d00b6 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Mon, 21 Aug 2023 10:40:17 -0700 Subject: [PATCH 08/31] Fix an OOB bug in parse_gap_data Bug: 277590580 bug: 275553827 Test: atest net_test_main_shim Ignore-AOSP-First: security Tag: #security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:0d7e3d8fd96389f1435b76f37064c69ae61df6e7) Merged-In: I7fcb7c46f668f48560a72399a3c5087c6da3827f Change-Id: I7fcb7c46f668f48560a72399a3c5087c6da3827f --- system/main/shim/utils.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/system/main/shim/utils.cc b/system/main/shim/utils.cc index dcf1725beb1..9f18ddc4f76 100644 --- a/system/main/shim/utils.cc +++ b/system/main/shim/utils.cc @@ -25,6 +25,10 @@ void parse_gap_data(const std::vector &raw_data, hci::GapData gap_data; uint8_t len = raw_data[offset]; + if (offset + len + 1 > raw_data.size()) { + break; + } + auto begin = raw_data.begin() + offset; auto end = begin + len + 1; // 1 byte for len auto data_copy = std::make_shared>(begin, end); -- GitLab From 243fdf1c0d53bda9e829b4bec9f7c2a824b4d3d1 Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Fri, 19 May 2023 21:46:53 +0000 Subject: [PATCH 09/31] Add bounds checks in btif_avrcp_audio_track.cc Fuzz testing reveals that the transcodeQ*ToFloat family of functions are not bounds checked, causing a potential OOB write. Check these functions against bounds of the destination array. Bug: 275895309 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:46803ae95d63ee133eae83d885e7c051964dc8ed) Merged-In: I7a13261429797769cf5b913912a30e249668ac93 Change-Id: I7a13261429797769cf5b913912a30e249668ac93 --- system/btif/src/btif_avrcp_audio_track.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/system/btif/src/btif_avrcp_audio_track.cc b/system/btif/src/btif_avrcp_audio_track.cc index 8ca5c979855..e17f80f5f74 100644 --- a/system/btif/src/btif_avrcp_audio_track.cc +++ b/system/btif/src/btif_avrcp_audio_track.cc @@ -23,6 +23,8 @@ #include #include +#include + #include "bt_target.h" #include "osi/include/log.h" @@ -152,7 +154,7 @@ static size_t transcodeQ15ToFloat(uint8_t* buffer, size_t length, BtifAvrcpAudioTrack* trackHolder) { size_t sampleSize = sampleSizeFor(trackHolder); size_t i = 0; - for (; i <= length / sampleSize; i++) { + for (; i < std::min(trackHolder->bufferLength, length / sampleSize); i++) { trackHolder->buffer[i] = ((int16_t*)buffer)[i] * kScaleQ15ToFloat; } return i * sampleSize; @@ -162,7 +164,7 @@ static size_t transcodeQ23ToFloat(uint8_t* buffer, size_t length, BtifAvrcpAudioTrack* trackHolder) { size_t sampleSize = sampleSizeFor(trackHolder); size_t i = 0; - for (; i <= length / sampleSize; i++) { + for (; i < std::min(trackHolder->bufferLength, length / sampleSize); i++) { size_t offset = i * sampleSize; int32_t sample = *((int32_t*)(buffer + offset - 1)) & 0x00FFFFFF; trackHolder->buffer[i] = sample * kScaleQ23ToFloat; @@ -174,7 +176,7 @@ static size_t transcodeQ31ToFloat(uint8_t* buffer, size_t length, BtifAvrcpAudioTrack* trackHolder) { size_t sampleSize = sampleSizeFor(trackHolder); size_t i = 0; - for (; i <= length / sampleSize; i++) { + for (; i < std::min(trackHolder->bufferLength, length / sampleSize); i++) { trackHolder->buffer[i] = ((int32_t*)buffer)[i] * kScaleQ31ToFloat; } return i * sampleSize; -- GitLab From 5bfd817719fcf55cbb3476e6b5539a3db4c437fc Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Tue, 8 Aug 2023 00:55:17 +0000 Subject: [PATCH 10/31] Fix UAF in ~CallbackEnv com_android_bluetooth_btservice_AdapterService does not null its local JNI environment variable after detaching the thread (which frees the environment context), allowing UAF under certain conditions. Null the variable in this case. Testing here was done through a custom unit test; see patchsets 4-6 for contents. However, unit testing of the JNI layer is problematic in production, so that part of the patch is omitted for final merge. Bug: 291500341 Test: atest bluetooth_test_gd_unit, atest net_test_stack_btm Tag: #security Ignore-AOSP-First: Security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:7a5c71c32d382c0e14083f0d093ae4f5420968ff) Merged-In: I3e5e3c51412640aa19f0981caaa809313d6ad030 Change-Id: I3e5e3c51412640aa19f0981caaa809313d6ad030 --- .../app/jni/com_android_bluetooth_btservice_AdapterService.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/android/app/jni/com_android_bluetooth_btservice_AdapterService.cpp b/android/app/jni/com_android_bluetooth_btservice_AdapterService.cpp index 1c86ca889ee..2bff59b6bcd 100644 --- a/android/app/jni/com_android_bluetooth_btservice_AdapterService.cpp +++ b/android/app/jni/com_android_bluetooth_btservice_AdapterService.cpp @@ -644,6 +644,7 @@ static void callback_thread_event(bt_cb_thread_evt event) { } vm->DetachCurrentThread(); sHaveCallbackThread = false; + callbackEnv = NULL; } } -- GitLab From 495417bd068c35de0729d9a332639bd0699153ff Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Tue, 11 Jul 2023 04:27:10 +0000 Subject: [PATCH 11/31] Fix timing attack in BTM_BleVerifySignature BTM_BleVerifySignature uses a stock memcmp, allowing signature contents to be deduced through a side-channel attack. Change to CRYPTO_memcmp, which is hardened against this attack, to eliminate this attack. Bug: 274478807 Test: atest bluetooth_test_gd_unit Tag: #security Ignore-AOSP-First: Security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:7a960ac1c0cbc6d3949b6eaa7a86302a0b20c04f) Merged-In: Iddeff055d9064f51a1e0cfb851d8b74135a714c2 Change-Id: Iddeff055d9064f51a1e0cfb851d8b74135a714c2 --- system/stack/btm/btm_ble.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/system/stack/btm/btm_ble.cc b/system/stack/btm/btm_ble.cc index 3a18f258da9..fb488ed0a59 100644 --- a/system/stack/btm/btm_ble.cc +++ b/system/stack/btm/btm_ble.cc @@ -25,12 +25,15 @@ #define LOG_TAG "bt_btm_ble" +#include + #include #include "device/include/controller.h" #include "main/shim/btm_api.h" #include "main/shim/l2c_api.h" #include "main/shim/shim.h" +#include "openssl/mem.h" #include "osi/include/allocator.h" #include "osi/include/properties.h" #include "stack/btm/btm_dev.h" @@ -48,8 +51,6 @@ #include "stack/include/smp_api.h" #include "types/raw_address.h" -#include - extern tBTM_CB btm_cb; extern bool btm_ble_init_pseudo_addr(tBTM_SEC_DEV_REC* p_dev_rec, @@ -1991,7 +1992,7 @@ bool BTM_BleVerifySignature(const RawAddress& bd_addr, uint8_t* p_orig, crypto_toolbox::aes_cmac(p_rec->ble.keys.pcsrk, p_orig, len, BTM_CMAC_TLEN_SIZE, p_mac); - if (memcmp(p_mac, p_comp, BTM_CMAC_TLEN_SIZE) == 0) { + if (CRYPTO_memcmp(p_mac, p_comp, BTM_CMAC_TLEN_SIZE) == 0) { btm_ble_increment_sign_ctr(bd_addr, false); verified = true; } -- GitLab From 1d7ba7c8a205522f384e8d5c7c9f26a421cab5f1 Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Tue, 23 May 2023 23:09:48 +0000 Subject: [PATCH 12/31] 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 --- system/stack/btm/btm_ble_gap.cc | 62 +++++++++++++++++++++++++-------- 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 7083c88b35b..12a27376bb9 100644 --- a/system/stack/btm/btm_ble_gap.cc +++ b/system/stack/btm/btm_ble_gap.cc @@ -1418,6 +1418,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); @@ -2515,20 +2525,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; } @@ -2548,8 +2565,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; } @@ -2582,18 +2602,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; } @@ -2605,8 +2635,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 4d24c481504..9c317726257 100644 --- a/system/stack/btu/btu_hcif.cc +++ b/system/stack/btu/btu_hcif.cc @@ -1680,6 +1680,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 */ -- GitLab From 9c41dce444ab9808f9f42afdf1bd93419cd1f020 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Sat, 29 Apr 2023 18:04:37 +0000 Subject: [PATCH 13/31] Fix an OOB bug in btif_to_bta_response and attp_build_value_cmd 1. The size of `p_src->attr_value.value` is dependent on `p_src->attr_value.len`. While copying `p_src->attr_value.value`, to `p_dest->attr_value.value`, it always copies GATT_MAX_ATTR_LEN bytes, it may result in OOB read in `p_src->attr_value.value`; 2. As the `p_dest->attr_value.len` does not map the length of `p_dest->attr_value.value`, it may result in OOB read in attp_build_value_cmd; Bug: 276898739 Test: manual Tag: #security Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:59c9e84bd31d4935a875d588bf4d2cc5bfb07d59) Merged-In: Iefa66f3a293ac2072ba79853a9ec23cdfe4c1368 Change-Id: Iefa66f3a293ac2072ba79853a9ec23cdfe4c1368 --- system/btif/src/btif_gatt_util.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/system/btif/src/btif_gatt_util.cc b/system/btif/src/btif_gatt_util.cc index 290c4315a43..404dc6ae729 100644 --- a/system/btif/src/btif_gatt_util.cc +++ b/system/btif/src/btif_gatt_util.cc @@ -18,6 +18,8 @@ #define LOG_TAG "bt_btif_gatt" +#include + #include "btif_gatt_util.h" #include @@ -51,9 +53,9 @@ using bluetooth::Uuid; void btif_to_bta_response(tGATTS_RSP* p_dest, btgatt_response_t* p_src) { p_dest->attr_value.auth_req = p_src->attr_value.auth_req; p_dest->attr_value.handle = p_src->attr_value.handle; - p_dest->attr_value.len = p_src->attr_value.len; + p_dest->attr_value.len = std::min(p_src->attr_value.len, GATT_MAX_ATTR_LEN); p_dest->attr_value.offset = p_src->attr_value.offset; - memcpy(p_dest->attr_value.value, p_src->attr_value.value, GATT_MAX_ATTR_LEN); + memcpy(p_dest->attr_value.value, p_src->attr_value.value, p_dest->attr_value.len); } /******************************************************************************* -- GitLab From 1e6e0b5ff2a72780c8f0cc71c04e6504d627bc21 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Tue, 17 Oct 2023 00:39:31 +0000 Subject: [PATCH 14/31] Fix an OOB write bug in attp_build_read_by_type_value_cmd Bug: 297524203 Test: m com.android.btservices Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:140c41e3553bc59fe97e3f5ee96c64e2251971e2) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:e9b40c3dfd81c3fa99b3f115135de7e2c356ece9) Merged-In: I2a95bbcce9a16ac84dd714eb4561428711a9872e Change-Id: I2a95bbcce9a16ac84dd714eb4561428711a9872e --- system/stack/gatt/att_protocol.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/system/stack/gatt/att_protocol.cc b/system/stack/gatt/att_protocol.cc index 63847db72da..29ceec26733 100644 --- a/system/stack/gatt/att_protocol.cc +++ b/system/stack/gatt/att_protocol.cc @@ -164,7 +164,13 @@ static BT_HDR* attp_build_read_by_type_value_cmd( uint16_t payload_size, tGATT_FIND_TYPE_VALUE* p_value_type) { uint8_t* p; uint16_t len = p_value_type->value_len; - BT_HDR* p_buf = + BT_HDR* p_buf = nullptr; + + if (payload_size < 5) { + return nullptr; + } + + p_buf = (BT_HDR*)osi_malloc(sizeof(BT_HDR) + payload_size + L2CAP_MIN_OFFSET); p = (uint8_t*)(p_buf + 1) + L2CAP_MIN_OFFSET; -- GitLab From 17044ccf3a2858633cad8f87926e752edfe0d8d8 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Tue, 29 Aug 2023 12:05:45 -0700 Subject: [PATCH 15/31] Fix an OOB write bug in attp_build_value_cmd Bug: 295887535 Test: m com.android.btservices Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:b927f3fb660dafaf97b2fa0398353a8c39125efc) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:a0d4425c3964f99f589d449deed2f1bbe520218c) Merged-In: Ie16251c3a2b7c0f807ecb53bbf125d1e8c276e48 Change-Id: Ie16251c3a2b7c0f807ecb53bbf125d1e8c276e48 --- system/stack/gatt/att_protocol.cc | 55 ++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 11 deletions(-) diff --git a/system/stack/gatt/att_protocol.cc b/system/stack/gatt/att_protocol.cc index 29ceec26733..f055fb815ba 100644 --- a/system/stack/gatt/att_protocol.cc +++ b/system/stack/gatt/att_protocol.cc @@ -286,46 +286,79 @@ static BT_HDR* attp_build_opcode_cmd(uint8_t op_code) { static BT_HDR* attp_build_value_cmd(uint16_t payload_size, uint8_t op_code, uint16_t handle, uint16_t offset, uint16_t len, uint8_t* p_data) { - uint8_t *p, *pp, pair_len, *p_pair_len; + uint8_t *p, *pp, *p_pair_len; + size_t pair_len; + size_t size_now = 1; + + #define CHECK_SIZE() do { \ + if (size_now > payload_size) { \ + LOG(ERROR) << "payload size too small"; \ + osi_free(p_buf); \ + return nullptr; \ + } \ + } while (false) + BT_HDR* p_buf = (BT_HDR*)osi_malloc(sizeof(BT_HDR) + payload_size + L2CAP_MIN_OFFSET); p = pp = (uint8_t*)(p_buf + 1) + L2CAP_MIN_OFFSET; + + CHECK_SIZE(); UINT8_TO_STREAM(p, op_code); p_buf->offset = L2CAP_MIN_OFFSET; - p_buf->len = 1; if (op_code == GATT_RSP_READ_BY_TYPE) { p_pair_len = p; pair_len = len + 2; - UINT8_TO_STREAM(p, pair_len); - p_buf->len += 1; + size_now += 1; + CHECK_SIZE(); + // this field will be backfilled in the end of this function } + if (op_code != GATT_RSP_READ_BLOB && op_code != GATT_RSP_READ) { + size_now += 2; + CHECK_SIZE(); UINT16_TO_STREAM(p, handle); - p_buf->len += 2; } if (op_code == GATT_REQ_PREPARE_WRITE || op_code == GATT_RSP_PREPARE_WRITE) { + size_now += 2; + CHECK_SIZE(); UINT16_TO_STREAM(p, offset); - p_buf->len += 2; } - if (len > 0 && p_data != NULL) { + if (len > 0 && p_data != NULL && payload_size > size_now) { /* ensure data not exceed MTU size */ - if (payload_size - p_buf->len < len) { - len = payload_size - p_buf->len; + if (payload_size - size_now < len) { + len = payload_size - size_now; /* update handle value pair length */ - if (op_code == GATT_RSP_READ_BY_TYPE) *p_pair_len = (len + 2); + if (op_code == GATT_RSP_READ_BY_TYPE) { + pair_len = (len + 2); + } LOG(WARNING) << StringPrintf( "attribute value too long, to be truncated to %d", len); } + size_now += len; + CHECK_SIZE(); ARRAY_TO_STREAM(p, p_data, len); - p_buf->len += len; } + // backfill pair len field + if (op_code == GATT_RSP_READ_BY_TYPE) { + if (pair_len > UINT8_MAX) { + LOG(ERROR) << "pair_len greater than" << UINT8_MAX; + osi_free(p_buf); + return nullptr; + } + + *p_pair_len = (uint8_t) pair_len; + } + + #undef CHECK_SIZE + + p_buf->len = (uint16_t) size_now; return p_buf; } -- GitLab From f0f35273101518d1f3a660b151804e90d0249af3 Mon Sep 17 00:00:00 2001 From: Mehmet Murat Sevim Date: Wed, 6 Dec 2023 14:54:05 +0000 Subject: [PATCH 16/31] Revert "Fix an OOB write bug in attp_build_value_cmd" This reverts commit a0d4425c3964f99f589d449deed2f1bbe520218c. Reason for revert: LE Device name is incorrect after the change. See b/315127634 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:6dbe94fe556ef67f3bbb7d7bb2da3320d68619df) Merged-In: I93906e7ab768b4015fe3491e171fdb0ec8cf3077 Change-Id: I93906e7ab768b4015fe3491e171fdb0ec8cf3077 --- system/stack/gatt/att_protocol.cc | 55 +++++++------------------------ 1 file changed, 11 insertions(+), 44 deletions(-) diff --git a/system/stack/gatt/att_protocol.cc b/system/stack/gatt/att_protocol.cc index f055fb815ba..29ceec26733 100644 --- a/system/stack/gatt/att_protocol.cc +++ b/system/stack/gatt/att_protocol.cc @@ -286,79 +286,46 @@ static BT_HDR* attp_build_opcode_cmd(uint8_t op_code) { static BT_HDR* attp_build_value_cmd(uint16_t payload_size, uint8_t op_code, uint16_t handle, uint16_t offset, uint16_t len, uint8_t* p_data) { - uint8_t *p, *pp, *p_pair_len; - size_t pair_len; - size_t size_now = 1; - - #define CHECK_SIZE() do { \ - if (size_now > payload_size) { \ - LOG(ERROR) << "payload size too small"; \ - osi_free(p_buf); \ - return nullptr; \ - } \ - } while (false) - + uint8_t *p, *pp, pair_len, *p_pair_len; BT_HDR* p_buf = (BT_HDR*)osi_malloc(sizeof(BT_HDR) + payload_size + L2CAP_MIN_OFFSET); p = pp = (uint8_t*)(p_buf + 1) + L2CAP_MIN_OFFSET; - - CHECK_SIZE(); UINT8_TO_STREAM(p, op_code); p_buf->offset = L2CAP_MIN_OFFSET; + p_buf->len = 1; if (op_code == GATT_RSP_READ_BY_TYPE) { p_pair_len = p; pair_len = len + 2; - size_now += 1; - CHECK_SIZE(); - // this field will be backfilled in the end of this function + UINT8_TO_STREAM(p, pair_len); + p_buf->len += 1; } - if (op_code != GATT_RSP_READ_BLOB && op_code != GATT_RSP_READ) { - size_now += 2; - CHECK_SIZE(); UINT16_TO_STREAM(p, handle); + p_buf->len += 2; } if (op_code == GATT_REQ_PREPARE_WRITE || op_code == GATT_RSP_PREPARE_WRITE) { - size_now += 2; - CHECK_SIZE(); UINT16_TO_STREAM(p, offset); + p_buf->len += 2; } - if (len > 0 && p_data != NULL && payload_size > size_now) { + if (len > 0 && p_data != NULL) { /* ensure data not exceed MTU size */ - if (payload_size - size_now < len) { - len = payload_size - size_now; + if (payload_size - p_buf->len < len) { + len = payload_size - p_buf->len; /* update handle value pair length */ - if (op_code == GATT_RSP_READ_BY_TYPE) { - pair_len = (len + 2); - } + if (op_code == GATT_RSP_READ_BY_TYPE) *p_pair_len = (len + 2); LOG(WARNING) << StringPrintf( "attribute value too long, to be truncated to %d", len); } - size_now += len; - CHECK_SIZE(); ARRAY_TO_STREAM(p, p_data, len); + p_buf->len += len; } - // backfill pair len field - if (op_code == GATT_RSP_READ_BY_TYPE) { - if (pair_len > UINT8_MAX) { - LOG(ERROR) << "pair_len greater than" << UINT8_MAX; - osi_free(p_buf); - return nullptr; - } - - *p_pair_len = (uint8_t) pair_len; - } - - #undef CHECK_SIZE - - p_buf->len = (uint16_t) size_now; return p_buf; } -- GitLab From 34f488339b0c72102876b130698e8edc4ee23874 Mon Sep 17 00:00:00 2001 From: weidengke Date: Fri, 24 Feb 2023 17:59:59 +0800 Subject: [PATCH 17/31] Fix jni crash when get field of adType The type of field 'ad_type' in com.android.bluetooth.gatt.ScanFilterQueue.Entry is int. Test: Use eng version to verity. Change-Id: Ieee0496d790ab14d86439fdadc330540bedd3ebc Signed-off-by: weidengke --- android/app/jni/com_android_bluetooth_gatt.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/android/app/jni/com_android_bluetooth_gatt.cpp b/android/app/jni/com_android_bluetooth_gatt.cpp index 03517fc645e..d553e795d13 100644 --- a/android/app/jni/com_android_bluetooth_gatt.cpp +++ b/android/app/jni/com_android_bluetooth_gatt.cpp @@ -1714,7 +1714,7 @@ static void gattClientScanFilterAddNative(JNIEnv* env, jobject object, curr.company_mask = env->GetIntField(current.get(), companyMaskFid); - curr.ad_type = env->GetByteField(current.get(), adTypeFid); + curr.ad_type = env->GetIntField(current.get(), adTypeFid); ScopedLocalRef data( env, (jbyteArray)env->GetObjectField(current.get(), dataFid)); -- GitLab From 7d0f696f450241d8ba7a168ba14fa7b75032f0c9 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Fri, 20 Oct 2023 00:11:24 +0000 Subject: [PATCH 18/31] Fix an OOB bug in smp_proc_sec_req Bug: 300903400 Test: m com.android.btservices Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:f20a759c149b739f8dfc3790287ad1b954115c18) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:a4704e7519d0a02c1caf8b4d8ed874bc201a4b91) Merged-In: I400cfa3523c6d8b25c233205748c2db5dc803d1d Change-Id: I400cfa3523c6d8b25c233205748c2db5dc803d1d --- system/stack/smp/smp_act.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/system/stack/smp/smp_act.cc b/system/stack/smp/smp_act.cc index 6d8f007c72d..a4552d1300d 100644 --- a/system/stack/smp/smp_act.cc +++ b/system/stack/smp/smp_act.cc @@ -431,6 +431,13 @@ void smp_send_ltk_reply(tSMP_CB* p_cb, tSMP_INT_DATA* p_data) { * Description process security request. ******************************************************************************/ void smp_proc_sec_req(tSMP_CB* p_cb, tSMP_INT_DATA* p_data) { + if (smp_command_has_invalid_length(p_cb)) { + tSMP_INT_DATA smp_int_data; + smp_int_data.status = SMP_INVALID_PARAMETERS; + smp_sm_event(p_cb, SMP_AUTH_CMPL_EVT, &smp_int_data); + return; + } + tBTM_LE_AUTH_REQ auth_req = *(tBTM_LE_AUTH_REQ*)p_data->p_data; tBTM_BLE_SEC_REQ_ACT sec_req_act; -- GitLab From f24536f50a719f60c2727b6b6f70dd1ee3d023e7 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Tue, 29 Aug 2023 12:05:45 -0700 Subject: [PATCH 19/31] Fix an OOB write bug in attp_build_value_cmd Bug: 295887535 Test: m com.android.btservices Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:b927f3fb660dafaf97b2fa0398353a8c39125efc) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:a0d4425c3964f99f589d449deed2f1bbe520218c) Merged-In: Ie16251c3a2b7c0f807ecb53bbf125d1e8c276e48 Change-Id: Ie16251c3a2b7c0f807ecb53bbf125d1e8c276e48 --- system/stack/gatt/att_protocol.cc | 55 ++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 11 deletions(-) diff --git a/system/stack/gatt/att_protocol.cc b/system/stack/gatt/att_protocol.cc index 29ceec26733..f055fb815ba 100644 --- a/system/stack/gatt/att_protocol.cc +++ b/system/stack/gatt/att_protocol.cc @@ -286,46 +286,79 @@ static BT_HDR* attp_build_opcode_cmd(uint8_t op_code) { static BT_HDR* attp_build_value_cmd(uint16_t payload_size, uint8_t op_code, uint16_t handle, uint16_t offset, uint16_t len, uint8_t* p_data) { - uint8_t *p, *pp, pair_len, *p_pair_len; + uint8_t *p, *pp, *p_pair_len; + size_t pair_len; + size_t size_now = 1; + + #define CHECK_SIZE() do { \ + if (size_now > payload_size) { \ + LOG(ERROR) << "payload size too small"; \ + osi_free(p_buf); \ + return nullptr; \ + } \ + } while (false) + BT_HDR* p_buf = (BT_HDR*)osi_malloc(sizeof(BT_HDR) + payload_size + L2CAP_MIN_OFFSET); p = pp = (uint8_t*)(p_buf + 1) + L2CAP_MIN_OFFSET; + + CHECK_SIZE(); UINT8_TO_STREAM(p, op_code); p_buf->offset = L2CAP_MIN_OFFSET; - p_buf->len = 1; if (op_code == GATT_RSP_READ_BY_TYPE) { p_pair_len = p; pair_len = len + 2; - UINT8_TO_STREAM(p, pair_len); - p_buf->len += 1; + size_now += 1; + CHECK_SIZE(); + // this field will be backfilled in the end of this function } + if (op_code != GATT_RSP_READ_BLOB && op_code != GATT_RSP_READ) { + size_now += 2; + CHECK_SIZE(); UINT16_TO_STREAM(p, handle); - p_buf->len += 2; } if (op_code == GATT_REQ_PREPARE_WRITE || op_code == GATT_RSP_PREPARE_WRITE) { + size_now += 2; + CHECK_SIZE(); UINT16_TO_STREAM(p, offset); - p_buf->len += 2; } - if (len > 0 && p_data != NULL) { + if (len > 0 && p_data != NULL && payload_size > size_now) { /* ensure data not exceed MTU size */ - if (payload_size - p_buf->len < len) { - len = payload_size - p_buf->len; + if (payload_size - size_now < len) { + len = payload_size - size_now; /* update handle value pair length */ - if (op_code == GATT_RSP_READ_BY_TYPE) *p_pair_len = (len + 2); + if (op_code == GATT_RSP_READ_BY_TYPE) { + pair_len = (len + 2); + } LOG(WARNING) << StringPrintf( "attribute value too long, to be truncated to %d", len); } + size_now += len; + CHECK_SIZE(); ARRAY_TO_STREAM(p, p_data, len); - p_buf->len += len; } + // backfill pair len field + if (op_code == GATT_RSP_READ_BY_TYPE) { + if (pair_len > UINT8_MAX) { + LOG(ERROR) << "pair_len greater than" << UINT8_MAX; + osi_free(p_buf); + return nullptr; + } + + *p_pair_len = (uint8_t) pair_len; + } + + #undef CHECK_SIZE + + p_buf->len = (uint16_t) size_now; return p_buf; } -- GitLab From 8d1f02addc492d65c50ec56e4cfdd2b6e33ae865 Mon Sep 17 00:00:00 2001 From: Mehmet Murat Sevim Date: Wed, 6 Dec 2023 14:54:05 +0000 Subject: [PATCH 20/31] Revert "Fix an OOB write bug in attp_build_value_cmd" This reverts commit a0d4425c3964f99f589d449deed2f1bbe520218c. Reason for revert: LE Device name is incorrect after the change. See b/315127634 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:6dbe94fe556ef67f3bbb7d7bb2da3320d68619df) Merged-In: I93906e7ab768b4015fe3491e171fdb0ec8cf3077 Change-Id: I93906e7ab768b4015fe3491e171fdb0ec8cf3077 --- system/stack/gatt/att_protocol.cc | 55 +++++++------------------------ 1 file changed, 11 insertions(+), 44 deletions(-) diff --git a/system/stack/gatt/att_protocol.cc b/system/stack/gatt/att_protocol.cc index f055fb815ba..29ceec26733 100644 --- a/system/stack/gatt/att_protocol.cc +++ b/system/stack/gatt/att_protocol.cc @@ -286,79 +286,46 @@ static BT_HDR* attp_build_opcode_cmd(uint8_t op_code) { static BT_HDR* attp_build_value_cmd(uint16_t payload_size, uint8_t op_code, uint16_t handle, uint16_t offset, uint16_t len, uint8_t* p_data) { - uint8_t *p, *pp, *p_pair_len; - size_t pair_len; - size_t size_now = 1; - - #define CHECK_SIZE() do { \ - if (size_now > payload_size) { \ - LOG(ERROR) << "payload size too small"; \ - osi_free(p_buf); \ - return nullptr; \ - } \ - } while (false) - + uint8_t *p, *pp, pair_len, *p_pair_len; BT_HDR* p_buf = (BT_HDR*)osi_malloc(sizeof(BT_HDR) + payload_size + L2CAP_MIN_OFFSET); p = pp = (uint8_t*)(p_buf + 1) + L2CAP_MIN_OFFSET; - - CHECK_SIZE(); UINT8_TO_STREAM(p, op_code); p_buf->offset = L2CAP_MIN_OFFSET; + p_buf->len = 1; if (op_code == GATT_RSP_READ_BY_TYPE) { p_pair_len = p; pair_len = len + 2; - size_now += 1; - CHECK_SIZE(); - // this field will be backfilled in the end of this function + UINT8_TO_STREAM(p, pair_len); + p_buf->len += 1; } - if (op_code != GATT_RSP_READ_BLOB && op_code != GATT_RSP_READ) { - size_now += 2; - CHECK_SIZE(); UINT16_TO_STREAM(p, handle); + p_buf->len += 2; } if (op_code == GATT_REQ_PREPARE_WRITE || op_code == GATT_RSP_PREPARE_WRITE) { - size_now += 2; - CHECK_SIZE(); UINT16_TO_STREAM(p, offset); + p_buf->len += 2; } - if (len > 0 && p_data != NULL && payload_size > size_now) { + if (len > 0 && p_data != NULL) { /* ensure data not exceed MTU size */ - if (payload_size - size_now < len) { - len = payload_size - size_now; + if (payload_size - p_buf->len < len) { + len = payload_size - p_buf->len; /* update handle value pair length */ - if (op_code == GATT_RSP_READ_BY_TYPE) { - pair_len = (len + 2); - } + if (op_code == GATT_RSP_READ_BY_TYPE) *p_pair_len = (len + 2); LOG(WARNING) << StringPrintf( "attribute value too long, to be truncated to %d", len); } - size_now += len; - CHECK_SIZE(); ARRAY_TO_STREAM(p, p_data, len); + p_buf->len += len; } - // backfill pair len field - if (op_code == GATT_RSP_READ_BY_TYPE) { - if (pair_len > UINT8_MAX) { - LOG(ERROR) << "pair_len greater than" << UINT8_MAX; - osi_free(p_buf); - return nullptr; - } - - *p_pair_len = (uint8_t) pair_len; - } - - #undef CHECK_SIZE - - p_buf->len = (uint16_t) size_now; return p_buf; } -- GitLab From 015c618a0461def93138173a53daaf27ca0630c9 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Tue, 29 Aug 2023 12:05:45 -0700 Subject: [PATCH 21/31] Reland: Fix an OOB write bug in attp_build_value_cmd Bug: 295887535 Bug: 315127634 Test: m com.android.btservices Test: atest net_test_stack_gatt Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:4ae5e736813bf2928bfc8c71e3dacf3b78394046) Merged-In: I291fd665a68d90813b8c21c80d23cc438f84f285 Change-Id: I291fd665a68d90813b8c21c80d23cc438f84f285 --- system/stack/gatt/att_protocol.cc | 56 +++++++++++++++++++++++++------ 1 file changed, 45 insertions(+), 11 deletions(-) diff --git a/system/stack/gatt/att_protocol.cc b/system/stack/gatt/att_protocol.cc index 29ceec26733..c8c6f2422a2 100644 --- a/system/stack/gatt/att_protocol.cc +++ b/system/stack/gatt/att_protocol.cc @@ -286,46 +286,80 @@ static BT_HDR* attp_build_opcode_cmd(uint8_t op_code) { static BT_HDR* attp_build_value_cmd(uint16_t payload_size, uint8_t op_code, uint16_t handle, uint16_t offset, uint16_t len, uint8_t* p_data) { - uint8_t *p, *pp, pair_len, *p_pair_len; + uint8_t *p, *pp, *p_pair_len; + size_t pair_len; + size_t size_now = 1; + +#define CHECK_SIZE() \ + do { \ + if (size_now > payload_size) { \ + LOG_ERROR("payload size too small"); \ + osi_free(p_buf); \ + return nullptr; \ + } \ + } while (false) + BT_HDR* p_buf = (BT_HDR*)osi_malloc(sizeof(BT_HDR) + payload_size + L2CAP_MIN_OFFSET); p = pp = (uint8_t*)(p_buf + 1) + L2CAP_MIN_OFFSET; + + CHECK_SIZE(); UINT8_TO_STREAM(p, op_code); p_buf->offset = L2CAP_MIN_OFFSET; - p_buf->len = 1; if (op_code == GATT_RSP_READ_BY_TYPE) { - p_pair_len = p; + p_pair_len = p++; pair_len = len + 2; - UINT8_TO_STREAM(p, pair_len); - p_buf->len += 1; + size_now += 1; + CHECK_SIZE(); + // this field will be backfilled in the end of this function } + if (op_code != GATT_RSP_READ_BLOB && op_code != GATT_RSP_READ) { + size_now += 2; + CHECK_SIZE(); UINT16_TO_STREAM(p, handle); - p_buf->len += 2; } if (op_code == GATT_REQ_PREPARE_WRITE || op_code == GATT_RSP_PREPARE_WRITE) { + size_now += 2; + CHECK_SIZE(); UINT16_TO_STREAM(p, offset); - p_buf->len += 2; } if (len > 0 && p_data != NULL) { /* ensure data not exceed MTU size */ - if (payload_size - p_buf->len < len) { - len = payload_size - p_buf->len; + if (payload_size - size_now < len) { + len = payload_size - size_now; /* update handle value pair length */ - if (op_code == GATT_RSP_READ_BY_TYPE) *p_pair_len = (len + 2); + if (op_code == GATT_RSP_READ_BY_TYPE) { + pair_len = (len + 2); + } LOG(WARNING) << StringPrintf( "attribute value too long, to be truncated to %d", len); } + size_now += len; + CHECK_SIZE(); ARRAY_TO_STREAM(p, p_data, len); - p_buf->len += len; } + // backfill pair len field + if (op_code == GATT_RSP_READ_BY_TYPE) { + if (pair_len > UINT8_MAX) { + LOG_ERROR("pair_len greater than %d", UINT8_MAX); + osi_free(p_buf); + return nullptr; + } + + *p_pair_len = (uint8_t)pair_len; + } + +#undef CHECK_SIZE + + p_buf->len = (uint16_t)size_now; return p_buf; } -- GitLab From c5c528beb6e1cfed3ec93a3a264084df32ce83c2 Mon Sep 17 00:00:00 2001 From: Hui Peng Date: Thu, 4 Jan 2024 06:27:52 +0000 Subject: [PATCH 22/31] Fix a security bypass issue in access_secure_service_from_temp_bond Bug: 318374503 Test: m com.android.btservices | manual test against PoC | QA Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:62944f39f502b28687a5142ec2d77585525591bc) Merged-In: I48df2c2d77810077e97d4131540277273d441998 Change-Id: I48df2c2d77810077e97d4131540277273d441998 --- system/stack/btm/btm_sec.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/system/stack/btm/btm_sec.cc b/system/stack/btm/btm_sec.cc index c77c40535fb..6ec6d16297a 100644 --- a/system/stack/btm/btm_sec.cc +++ b/system/stack/btm/btm_sec.cc @@ -222,8 +222,7 @@ static bool access_secure_service_from_temp_bond(const tBTM_SEC_DEV_REC* p_dev_r bool locally_initiated, uint16_t security_req) { return !locally_initiated && (security_req & BTM_SEC_IN_AUTHENTICATE) && - p_dev_rec->is_device_authenticated() && - p_dev_rec->is_bond_type_temporary(); + p_dev_rec->is_bond_type_temporary(); } /******************************************************************************* -- GitLab From 9dc349d70145e102005539339a9e05da58899b63 Mon Sep 17 00:00:00 2001 From: Aaron Kling Date: Fri, 9 Feb 2024 22:54:48 -0600 Subject: [PATCH 23/31] BondStateMachine: Allow skipping confirm for some remotes When pairing two of the Nvidia Shield accessories, a popup would show up stating that the accessory was an incoming pairing request and needs to be accepted. The official Nvidia firmware has a whitelist of remotes that skip this confirmation if pairing request is marked as originating from the Android device. This change takes a similar approach, but in a more flexible manner. The main intent is to allow these accessories to be paired via the pairing intent, which needs to complete with no user interaction. Previously, the popup would prevent this from succeeding. Change-Id: Ib5a0226858f5745a20e4cd166500aecdcf1f3354 --- .../bluetooth/btservice/BondStateMachine.java | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/android/app/src/com/android/bluetooth/btservice/BondStateMachine.java b/android/app/src/com/android/bluetooth/btservice/BondStateMachine.java index c6633daab22..6d64cfd1113 100644 --- a/android/app/src/com/android/bluetooth/btservice/BondStateMachine.java +++ b/android/app/src/com/android/bluetooth/btservice/BondStateMachine.java @@ -32,6 +32,7 @@ import android.os.Bundle; import android.os.Message; import android.os.UserHandle; import android.util.Log; +import android.util.Pair; import com.android.bluetooth.BluetoothStatsLog; import com.android.bluetooth.Utils; @@ -48,6 +49,7 @@ import com.android.internal.util.StateMachine; import java.util.ArrayList; import java.util.HashSet; +import java.util.List; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -384,7 +386,37 @@ final class BondStateMachine extends StateMachine { return false; } + // Defining these properly would break current api + private static int PERIPHERAL_GAMEPAD = BluetoothClass.Device.Major.PERIPHERAL | 0x08; + private static int PERIPHERAL_REMOTE = BluetoothClass.Device.Major.PERIPHERAL | 0x0C; + + private static List> accConfirmSkip = new ArrayList<>(); + + static { + // Jarvis, SHIELD Remote 2015 + accConfirmSkip.add(new Pair<>("SHIELD Remote", PERIPHERAL_REMOTE)); + // Thunderstrike, SHIELD Controller 2017 + accConfirmSkip.add(new Pair<>("NVIDIA Controller v01.04", PERIPHERAL_GAMEPAD)); + }; + + private boolean isSkipConfirmationAccessory(BluetoothDevice device) { + for (Pair entry : accConfirmSkip) { + if (device.getName().equals(entry.first) + && device.getBluetoothClass().getDeviceClass() == entry.second) { + return true; + } + } + + return false; + } + private void sendDisplayPinIntent(byte[] address, Optional maybePin, int variant) { + BluetoothDevice device = mRemoteDevices.getDevice(address); + if (device != null && device.isBondingInitiatedLocally() + && isSkipConfirmationAccessory(device)) { + device.setPairingConfirmation(true); + return; + } Intent intent = new Intent(BluetoothDevice.ACTION_PAIRING_REQUEST); intent.putExtra(BluetoothDevice.EXTRA_DEVICE, mRemoteDevices.getDevice(address)); maybePin.ifPresent(pin -> intent.putExtra(BluetoothDevice.EXTRA_PAIRING_KEY, pin)); -- GitLab From 456f705b9acc78d8184536baff3d21b0bc11c957 Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Mon, 22 Apr 2024 21:10:09 +0000 Subject: [PATCH 24/31] Fix an authentication bypass bug in SMP When pairing with BLE legacy pairing initiated from remote, authentication can be bypassed. This change fixes it. Bug: 251514170 Test: m com.android.btservices Test: manual run against PoC Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:25a3fcd487c799d5d9029b8646159a0b10143d97) Merged-In: I369a8fdd675eca731a7a488ed6a2be645058b795 Change-Id: I369a8fdd675eca731a7a488ed6a2be645058b795 --- system/stack/smp/smp_act.cc | 12 ++++++++++++ system/stack/smp/smp_int.h | 1 + 2 files changed, 13 insertions(+) diff --git a/system/stack/smp/smp_act.cc b/system/stack/smp/smp_act.cc index a4552d1300d..bbbf3dc29ba 100644 --- a/system/stack/smp/smp_act.cc +++ b/system/stack/smp/smp_act.cc @@ -286,6 +286,7 @@ void smp_send_pair_rsp(tSMP_CB* p_cb, tSMP_INT_DATA* p_data) { void smp_send_confirm(tSMP_CB* p_cb, tSMP_INT_DATA* p_data) { SMP_TRACE_DEBUG("%s", __func__); smp_send_cmd(SMP_OPCODE_CONFIRM, p_cb); + p_cb->flags |= SMP_PAIR_FLAGS_CMD_CONFIRM_SENT; } /******************************************************************************* @@ -654,6 +655,17 @@ void smp_proc_init(tSMP_CB* p_cb, tSMP_INT_DATA* p_data) { return; } + if (!((p_cb->loc_auth_req & SMP_SC_SUPPORT_BIT) && + (p_cb->peer_auth_req & SMP_SC_SUPPORT_BIT)) && + !(p_cb->flags & SMP_PAIR_FLAGS_CMD_CONFIRM_SENT)) { + // in legacy pairing, the peer should send its rand after + // we send our confirm + tSMP_INT_DATA smp_int_data{}; + smp_int_data.status = SMP_INVALID_PARAMETERS; + smp_sm_event(p_cb, SMP_AUTH_CMPL_EVT, &smp_int_data); + return; + } + /* save the SRand for comparison */ STREAM_TO_ARRAY(p_cb->rrand.data(), p, OCTET16_LEN); } diff --git a/system/stack/smp/smp_int.h b/system/stack/smp/smp_int.h index c4ddf3ec5ef..cb88476ba29 100644 --- a/system/stack/smp/smp_int.h +++ b/system/stack/smp/smp_int.h @@ -213,6 +213,7 @@ typedef union { (1 << 7) /* used to resolve race condition */ #define SMP_PAIR_FLAG_HAVE_LOCAL_PUBL_KEY \ (1 << 8) /* used on peripheral to resolve race condition */ +#define SMP_PAIR_FLAGS_CMD_CONFIRM_SENT (1 << 9) /* check if authentication requirement need MITM protection */ #define SMP_NO_MITM_REQUIRED(x) (((x)&SMP_AUTH_YN_BIT) == 0) -- GitLab From ed87ee31dc6b32ea0e99e863f51413d911f038e6 Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Mon, 6 May 2024 17:32:14 +0000 Subject: [PATCH 25/31] Fix permission bypasses to multiple methods Researcher reports that some BT calls across Binder are validating only BT's own permissions and not the calling app's permissions. On investigation this seems to be due to a missing null check in several BT permissions checks, which allows a malicious app to pass in a null AttributionSource and therefore produce a stub AttributionSource chain which does not properly check for the caller's permissions. Add null checks. Bug: 242996380 Test: atest UtilsTest Test: researcher POC Tag: #security Ignore-AOSP-First: Security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:ed63d97fd6537f539fdde1413bff86a30f80a7b5) Merged-In: I7a11e11257b85dc0752396490abfc79b1c383204 Change-Id: I7a11e11257b85dc0752396490abfc79b1c383204 --- .../app/src/com/android/bluetooth/Utils.java | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/android/app/src/com/android/bluetooth/Utils.java b/android/app/src/com/android/bluetooth/Utils.java index abe63bafe59..57879c0b9b2 100644 --- a/android/app/src/com/android/bluetooth/Utils.java +++ b/android/app/src/com/android/bluetooth/Utils.java @@ -462,10 +462,10 @@ public final class Utils { AttributionSource attributionSource, String message) { // STOPSHIP(b/188391719): enable this security enforcement // attributionSource.enforceCallingUid(); - AttributionSource currentAttribution = new AttributionSource - .Builder(context.getAttributionSource()) - .setNext(attributionSource) - .build(); + AttributionSource currentAttribution = + new AttributionSource.Builder(context.getAttributionSource()) + .setNext(Objects.requireNonNull(attributionSource)) + .build(); PermissionManager pm = context.getSystemService(PermissionManager.class); if (pm == null) { return false; @@ -711,10 +711,10 @@ public final class Utils { Log.e(TAG, "Permission denial: Location is off."); return false; } - AttributionSource currentAttribution = new AttributionSource - .Builder(context.getAttributionSource()) - .setNext(attributionSource) - .build(); + AttributionSource currentAttribution = + new AttributionSource.Builder(context.getAttributionSource()) + .setNext(Objects.requireNonNull(attributionSource)) + .build(); // STOPSHIP(b/188391719): enable this security enforcement // attributionSource.enforceCallingUid(); PermissionManager pm = context.getSystemService(PermissionManager.class); @@ -745,10 +745,10 @@ public final class Utils { return false; } - final AttributionSource currentAttribution = new AttributionSource - .Builder(context.getAttributionSource()) - .setNext(attributionSource) - .build(); + final AttributionSource currentAttribution = + new AttributionSource.Builder(context.getAttributionSource()) + .setNext(Objects.requireNonNull(attributionSource)) + .build(); // STOPSHIP(b/188391719): enable this security enforcement // attributionSource.enforceCallingUid(); PermissionManager pm = context.getSystemService(PermissionManager.class); @@ -783,10 +783,10 @@ public final class Utils { return false; } - AttributionSource currentAttribution = new AttributionSource - .Builder(context.getAttributionSource()) - .setNext(attributionSource) - .build(); + AttributionSource currentAttribution = + new AttributionSource.Builder(context.getAttributionSource()) + .setNext(Objects.requireNonNull(attributionSource)) + .build(); // STOPSHIP(b/188391719): enable this security enforcement // attributionSource.enforceCallingUid(); PermissionManager pm = context.getSystemService(PermissionManager.class); -- GitLab From 3e5374c94f4375f96b6ff3cd834efb2260694d82 Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Mon, 22 Apr 2024 17:21:30 +0000 Subject: [PATCH 26/31] Fix heap-buffer overflow in sdp_utils.cc Fuzzer identifies a case where sdpu_compare_uuid_with_attr crashes with an out of bounds comparison. Although the bug claims this is due to a comparison of a uuid with a smaller data field thana the discovery attribute, my research suggests that this instead stems from a comparison of a 128 bit UUID with a discovery attribute of some other, invalid size. Add checks for discovery attribute size. Bug: 287184435 Test: atest bluetooth_test_gd_unit, net_test_stack_sdp Tag: #security Ignore-AOSP-First: Security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:7bbdb139bf91dca86c72c33a74c0e3407938c487) Merged-In: I8e16ae525815bcdd47a2379ee8e5a6de47a3ac43 Change-Id: I8e16ae525815bcdd47a2379ee8e5a6de47a3ac43 --- system/stack/sdp/sdp_utils.cc | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/system/stack/sdp/sdp_utils.cc b/system/stack/sdp/sdp_utils.cc index 761883e60e4..51bb2d0b63a 100644 --- a/system/stack/sdp/sdp_utils.cc +++ b/system/stack/sdp/sdp_utils.cc @@ -955,8 +955,28 @@ bool sdpu_compare_uuid_arrays(const uint8_t* p_uuid1, uint32_t len1, ******************************************************************************/ bool sdpu_compare_uuid_with_attr(const Uuid& uuid, tSDP_DISC_ATTR* p_attr) { int len = uuid.GetShortestRepresentationSize(); - if (len == 2) return uuid.As16Bit() == p_attr->attr_value.v.u16; - if (len == 4) return uuid.As32Bit() == p_attr->attr_value.v.u32; + if (len == 2) { + if (SDP_DISC_ATTR_LEN(p_attr->attr_len_type) == Uuid::kNumBytes16) { + return uuid.As16Bit() == p_attr->attr_value.v.u16; + } else { + LOG(ERROR) << "invalid length for discovery attribute"; + return (false); + } + } + if (len == 4) { + if (SDP_DISC_ATTR_LEN(p_attr->attr_len_type) == Uuid::kNumBytes32) { + return uuid.As32Bit() == p_attr->attr_value.v.u32; + } else { + LOG(ERROR) << "invalid length for discovery attribute"; + return (false); + } + } + + if (SDP_DISC_ATTR_LEN(p_attr->attr_len_type) != Uuid::kNumBytes128) { + LOG(ERROR) << "invalid length for discovery attribute"; + return (false); + } + if (memcmp(uuid.To128BitBE().data(), (void*)p_attr->attr_value.v.array, Uuid::kNumBytes128) == 0) return (true); -- GitLab From 8c36bb8ecdeb72ddb06297547393fbec43caeecf Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Mon, 22 Apr 2024 21:41:53 +0000 Subject: [PATCH 27/31] Add support for checking security downgrade As a guard against the BLUFFS attack, we will need to check the security parameters of incoming connections against cached values and disallow connection if these parameters are downgraded or changed from their cached values. Future CLs will add checks during connection. This CL adds the functions that will be needed to perform those checks and the necessary mocks. Currently supported checks are : IO capabilities (must be an exact match), Secure Connections capability (must not be a downgrade), and session key length (must not be a downgrade). Maximum session key length, which was previously not cached, has been added to the device security manager cache. To QA: This CL is a logical no-op by itself. Tests should be performed as described in ag/25815924 and ag/25815925/ Bug: 314331379 Test: m libbluetooth Tag: #security Ignore-AOSP-First: Security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:3cf3d9d98909787748e6135733b42be0c67e9333) Merged-In: I972fd4a3a4d4566968d097df9f27396a821fb24f Change-Id: I972fd4a3a4d4566968d097df9f27396a821fb24f --- system/btif/src/btif_storage.cc | 31 ++++++ system/include/hardware/bluetooth.h | 14 +++ system/service/logging_helpers.cc | 2 + system/stack/btm/btm_sec.cc | 103 ++++++++++++++++++ system/stack/btm/btm_sec.h | 23 ++++ system/stack/include/sec_hci_link_interface.h | 3 + system/test/mock/mock_stack_btm_sec.cc | 8 ++ 7 files changed, 184 insertions(+) diff --git a/system/btif/src/btif_storage.cc b/system/btif/src/btif_storage.cc index 41fa4df5fbe..ae81bd1a38b 100644 --- a/system/btif/src/btif_storage.cc +++ b/system/btif/src/btif_storage.cc @@ -92,10 +92,13 @@ using bluetooth::groups::DeviceGroups; #define BTIF_STORAGE_KEY_ADAPTER_SCANMODE "ScanMode" #define BTIF_STORAGE_KEY_LOCAL_IO_CAPS "LocalIOCaps" #define BTIF_STORAGE_KEY_LOCAL_IO_CAPS_BLE "LocalIOCapsBLE" +#define BTIF_STORAGE_KEY_MAX_SESSION_KEY_SIZE "MaxSessionKeySize" #define BTIF_STORAGE_KEY_ADAPTER_DISC_TIMEOUT "DiscoveryTimeout" #define BTIF_STORAGE_KEY_GATT_CLIENT_SUPPORTED "GattClientSupportedFeatures" #define BTIF_STORAGE_KEY_GATT_CLIENT_DB_HASH "GattClientDatabaseHash" #define BTIF_STORAGE_KEY_GATT_SERVER_SUPPORTED "GattServerSupportedFeatures" +#define BTIF_STORAGE_KEY_SECURE_CONNECTIONS_SUPPORTED \ + "SecureConnectionsSupported" #define BTIF_STORAGE_DEVICE_GROUP_BIN "DeviceGroupBin" #define BTIF_STORAGE_CSIS_AUTOCONNECT "CsisAutoconnect" #define BTIF_STORAGE_CSIS_SET_INFO_BIN "CsisSetInfoBin" @@ -281,6 +284,14 @@ static int prop2cfg(const RawAddress* remote_bd_addr, bt_property_t* prop) { btif_config_set_int(bdstr, BT_CONFIG_KEY_REMOTE_VER_SUBVER, info->sub_ver); } break; + case BT_PROPERTY_REMOTE_SECURE_CONNECTIONS_SUPPORTED: + btif_config_set_int(bdstr, BTIF_STORAGE_KEY_SECURE_CONNECTIONS_SUPPORTED, + *(uint8_t*)prop->val); + break; + case BT_PROPERTY_REMOTE_MAX_SESSION_KEY_SIZE: + btif_config_set_int(bdstr, BTIF_STORAGE_KEY_MAX_SESSION_KEY_SIZE, + *(uint8_t*)prop->val); + break; default: BTIF_TRACE_ERROR("Unknown prop type:%d", prop->type); @@ -407,6 +418,26 @@ static int cfg2prop(const RawAddress* remote_bd_addr, bt_property_t* prop) { } } break; + case BT_PROPERTY_REMOTE_SECURE_CONNECTIONS_SUPPORTED: { + int val; + + if (prop->len >= (int)sizeof(uint8_t)) { + ret = btif_config_get_int( + bdstr, BTIF_STORAGE_KEY_SECURE_CONNECTIONS_SUPPORTED, &val); + *(uint8_t*)prop->val = (uint8_t)val; + } + } break; + + case BT_PROPERTY_REMOTE_MAX_SESSION_KEY_SIZE: { + int val; + + if (prop->len >= (int)sizeof(uint8_t)) { + ret = btif_config_get_int(bdstr, BTIF_STORAGE_KEY_MAX_SESSION_KEY_SIZE, + &val); + *(uint8_t*)prop->val = (uint8_t)val; + } + } break; + default: BTIF_TRACE_ERROR("Unknow prop type:%d", prop->type); return false; diff --git a/system/include/hardware/bluetooth.h b/system/include/hardware/bluetooth.h index 96e425ca242..81a410832f5 100644 --- a/system/include/hardware/bluetooth.h +++ b/system/include/hardware/bluetooth.h @@ -339,6 +339,20 @@ typedef enum { */ BT_PROPERTY_REMOTE_IS_COORDINATED_SET_MEMBER, + /** + * Description - Whether remote device supports Secure Connections mode + * Access mode - GET and SET. + * Data Type - uint8_t. + */ + BT_PROPERTY_REMOTE_SECURE_CONNECTIONS_SUPPORTED, + + /** + * Description - Maximum observed session key for remote device + * Access mode - GET and SET. + * Data Type - uint8_t. + */ + BT_PROPERTY_REMOTE_MAX_SESSION_KEY_SIZE, + BT_PROPERTY_REMOTE_DEVICE_TIMESTAMP = 0xFF, } bt_property_type_t; diff --git a/system/service/logging_helpers.cc b/system/service/logging_helpers.cc index 78a24e600f5..0c3644d2baa 100644 --- a/system/service/logging_helpers.cc +++ b/system/service/logging_helpers.cc @@ -118,6 +118,8 @@ const char* BtPropertyText(const bt_property_type_t prop) { CASE_RETURN_TEXT(BT_PROPERTY_REMOTE_VERSION_INFO); CASE_RETURN_TEXT(BT_PROPERTY_LOCAL_LE_FEATURES); CASE_RETURN_TEXT(BT_PROPERTY_REMOTE_DEVICE_TIMESTAMP); + CASE_RETURN_TEXT(BT_PROPERTY_REMOTE_SECURE_CONNECTIONS_SUPPORTED); + CASE_RETURN_TEXT(BT_PROPERTY_REMOTE_MAX_SESSION_KEY_SIZE); default: return "Invalid property"; } diff --git a/system/stack/btm/btm_sec.cc b/system/stack/btm/btm_sec.cc index 6ec6d16297a..0716bcd4868 100644 --- a/system/stack/btm/btm_sec.cc +++ b/system/stack/btm/btm_sec.cc @@ -207,6 +207,109 @@ static bool btm_dev_16_digit_authenticated(tBTM_SEC_DEV_REC* p_dev_rec) { return (false); } +/******************************************************************************* + * + * Function btm_sec_is_device_sc_downgrade + * + * Description Check for a stored device record matching the candidate + * device, and return true if the stored device has reported + * that it supports Secure Connections mode and the candidate + * device reports that it does not. Otherwise, return false. + * + * Returns bool + * + ******************************************************************************/ +static bool btm_sec_is_device_sc_downgrade(uint16_t hci_handle, + bool secure_connections_supported) { + if (secure_connections_supported) return false; + + tBTM_SEC_DEV_REC* p_dev_rec = btm_find_dev_by_handle(hci_handle); + if (p_dev_rec == nullptr) return false; + + uint8_t property_val = 0; + bt_property_t property = { + .type = BT_PROPERTY_REMOTE_SECURE_CONNECTIONS_SUPPORTED, + .len = sizeof(uint8_t), + .val = &property_val}; + + bt_status_t cached = + btif_storage_get_remote_device_property(&p_dev_rec->bd_addr, &property); + + if (cached == BT_STATUS_FAIL) return false; + + return (bool)property_val; +} + +/******************************************************************************* + * + * Function btm_sec_store_device_sc_support + * + * Description Save Secure Connections support for this device to file + * + ******************************************************************************/ + +static void btm_sec_store_device_sc_support(uint16_t hci_handle, + bool secure_connections_supported) { + tBTM_SEC_DEV_REC* p_dev_rec = btm_find_dev_by_handle(hci_handle); + if (p_dev_rec == nullptr) return; + + uint8_t property_val = (uint8_t)secure_connections_supported; + bt_property_t property = { + .type = BT_PROPERTY_REMOTE_SECURE_CONNECTIONS_SUPPORTED, + .len = sizeof(uint8_t), + .val = &property_val}; + + btif_storage_set_remote_device_property(&p_dev_rec->bd_addr, &property); +} + +/******************************************************************************* + * + * Function btm_sec_is_session_key_size_downgrade + * + * Description Check if there is a stored device record matching this + * handle, and return true if the stored record has a lower + * session key size than the candidate device. + * + * Returns bool + * + ******************************************************************************/ +bool btm_sec_is_session_key_size_downgrade(uint16_t hci_handle, + uint8_t key_size) { + tBTM_SEC_DEV_REC* p_dev_rec = btm_find_dev_by_handle(hci_handle); + if (p_dev_rec == nullptr) return false; + + uint8_t property_val = 0; + bt_property_t property = {.type = BT_PROPERTY_REMOTE_MAX_SESSION_KEY_SIZE, + .len = sizeof(uint8_t), + .val = &property_val}; + + bt_status_t cached = + btif_storage_get_remote_device_property(&p_dev_rec->bd_addr, &property); + + if (cached == BT_STATUS_FAIL) return false; + + return property_val > key_size; +} + +/******************************************************************************* + * + * Function btm_sec_update_session_key_size + * + * Description Store the max session key size to disk, if possible. + * + ******************************************************************************/ +void btm_sec_update_session_key_size(uint16_t hci_handle, uint8_t key_size) { + tBTM_SEC_DEV_REC* p_dev_rec = btm_find_dev_by_handle(hci_handle); + if (p_dev_rec == nullptr) return; + + uint8_t property_val = key_size; + bt_property_t property = {.type = BT_PROPERTY_REMOTE_MAX_SESSION_KEY_SIZE, + .len = sizeof(uint8_t), + .val = &property_val}; + + btif_storage_set_remote_device_property(&p_dev_rec->bd_addr, &property); +} + /******************************************************************************* * * Function access_secure_service_from_temp_bond diff --git a/system/stack/btm/btm_sec.h b/system/stack/btm/btm_sec.h index 8b92d5d78cd..a48dbb22825 100644 --- a/system/stack/btm/btm_sec.h +++ b/system/stack/btm/btm_sec.h @@ -805,5 +805,28 @@ void btm_sec_set_peer_sec_caps(uint16_t hci_handle, bool ssp_supported, void btm_sec_cr_loc_oob_data_cback_event(const RawAddress& address, tSMP_LOC_OOB_DATA loc_oob_data); +/******************************************************************************* + * + * Function btm_sec_is_session_key_size_downgrade + * + * Description Check if there is a stored device record matching this + * handle, and return true if the stored record has a lower + * session key size than the candidate device. + * + * Returns bool + * + ******************************************************************************/ +bool btm_sec_is_session_key_size_downgrade(uint16_t hci_handle, + uint8_t key_size); + +/******************************************************************************* + * + * Function btm_sec_update_session_key_size + * + * Description Store the max session key size to disk, if possible. + * + ******************************************************************************/ +void btm_sec_update_session_key_size(uint16_t hci_handle, uint8_t key_size); + // Return DEV_CLASS (uint8_t[3]) of bda. If record doesn't exist, create one. const uint8_t* btm_get_dev_class(const RawAddress& bda); diff --git a/system/stack/include/sec_hci_link_interface.h b/system/stack/include/sec_hci_link_interface.h index b5dda7f407c..6d859fd82f3 100644 --- a/system/stack/include/sec_hci_link_interface.h +++ b/system/stack/include/sec_hci_link_interface.h @@ -37,6 +37,8 @@ void btm_sec_auth_complete(uint16_t handle, tHCI_STATUS status); void btm_sec_disconnected(uint16_t handle, tHCI_STATUS reason, std::string); void btm_sec_encrypt_change(uint16_t handle, tHCI_STATUS status, uint8_t encr_enable); +bool btm_sec_is_session_key_size_downgrade(uint16_t hci_handle, + uint8_t key_size); void btm_sec_link_key_notification(const RawAddress& p_bda, const Octet16& link_key, uint8_t key_type); void btm_sec_link_key_request(const uint8_t* p_event); @@ -46,4 +48,5 @@ void btm_sec_rmt_name_request_complete(const RawAddress* bd_addr, const uint8_t* bd_name, tHCI_STATUS status); void btm_sec_update_clock_offset(uint16_t handle, uint16_t clock_offset); +void btm_sec_update_session_key_size(uint16_t hci_handle, uint8_t key_size); void btm_simple_pair_complete(const uint8_t* p); diff --git a/system/test/mock/mock_stack_btm_sec.cc b/system/test/mock/mock_stack_btm_sec.cc index 56a0db187b5..7b5f9e4d4c9 100644 --- a/system/test/mock/mock_stack_btm_sec.cc +++ b/system/test/mock/mock_stack_btm_sec.cc @@ -113,6 +113,11 @@ bool btm_sec_is_a_bonded_dev(const RawAddress& bda) { mock_function_count_map[__func__]++; return false; } +bool btm_sec_is_session_key_size_downgrade(uint16_t hci_handle, + uint8_t key_size) { + mock_function_count_map[__func__]++; + return false; +} bool is_sec_state_equal(void* data, void* context) { mock_function_count_map[__func__]++; return false; @@ -313,6 +318,9 @@ void btm_sec_set_peer_sec_caps(uint16_t hci_handle, bool ssp_supported, void btm_sec_update_clock_offset(uint16_t handle, uint16_t clock_offset) { mock_function_count_map[__func__]++; } +void btm_sec_update_session_key_size(uint16_t hci_handle, uint8_t key_size) { + mock_function_count_map[__func__]++; +} void btm_simple_pair_complete(const uint8_t* p) { mock_function_count_map[__func__]++; } -- GitLab From bfc38f93dbf52550ccd1ef6eca748e3d4e984792 Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Mon, 22 Apr 2024 21:46:27 +0000 Subject: [PATCH 28/31] Disallow connect with Secure Connections downgrade As a guard against the BLUFFS attack, check security parameters of incoming connections against cached values and disallow connection if these parameters are downgraded or changed from their cached values. This CL adds the connection-time check for Secure Connections mode. Bug: 314331379 Test: m libbluetooth Test: manual To test this CL, please ensure that BR/EDR initial connections and reconnections (after cycling remote devices, cycling Bluetooth, restarting the phone, etc.) work against remote devices which both support and do not support Secure Connections mode, and with all supported bonding types. Basic validation of LE bonding functionality should be done as well. Tag: #security Ignore-AOSP-First: Security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:f20fdd9b3225a6084f6b666172817fe0a89f0679) Merged-In: I9130476600d31b59608e0e419b5136d255174265 Change-Id: I9130476600d31b59608e0e419b5136d255174265 --- system/stack/btm/btm_sec.cc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/system/stack/btm/btm_sec.cc b/system/stack/btm/btm_sec.cc index 0716bcd4868..73c98aa868c 100644 --- a/system/stack/btm/btm_sec.cc +++ b/system/stack/btm/btm_sec.cc @@ -4077,6 +4077,13 @@ void btm_sec_link_key_notification(const RawAddress& p_bda, } } + if (p_dev_rec->is_bond_type_persistent() && + (p_dev_rec->is_device_type_br_edr() || + p_dev_rec->is_device_type_dual_mode())) { + btm_sec_store_device_sc_support(p_dev_rec->get_br_edr_hci_handle(), + p_dev_rec->SupportsSecureConnections()); + } + /* If name is not known at this point delay calling callback until the name is */ /* resolved. Unless it is a HID Device and we really need to send all link @@ -5153,6 +5160,16 @@ void btm_sec_set_peer_sec_caps(uint16_t hci_handle, bool ssp_supported, tBTM_SEC_DEV_REC* p_dev_rec = btm_find_dev_by_handle(hci_handle); if (p_dev_rec == nullptr) return; + // Drop the connection here if the remote attempts to downgrade from Secure + // Connections mode. + if (btm_sec_is_device_sc_downgrade(hci_handle, sc_supported)) { + acl_set_disconnect_reason(HCI_ERR_HOST_REJECT_SECURITY); + btm_sec_send_hci_disconnect( + p_dev_rec, HCI_ERR_AUTH_FAILURE, hci_handle, + "attempted to downgrade from Secure Connections mode"); + return; + } + p_dev_rec->remote_feature_received = true; p_dev_rec->remote_supports_hci_role_switch = hci_role_switch_supported; -- GitLab From 1ef66c4cc29e13fae7986a6a01b3ae5c717f2ee4 Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Mon, 22 Apr 2024 21:56:48 +0000 Subject: [PATCH 29/31] Disallow connect with key length downgrade As a guard against the BLUFFS attack, check security parameters of incoming connections against cached values and disallow connection if these parameters are downgraded or changed from their cached values. This CL adds the connection-time check for session key length. To test, please validate that bonding can be established and reestablished against devices with session key lengths of 7 and 16 bits, that session key lengths of less than 7 bits are refused, and that basic LE bonding functionality still works. If it is possible to configure a remote device to establish a bond with a session key length of 16 bits and then reduce that key length to <16 bits before reconnection, this should fail. Bug: 314331379 Test: m libbluetooth Test: manual Tag: #security Ignore-AOSP-First: Security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:d6e9fdf182afb57cecac6c56603aa20d758090a4) Merged-In: I27be1f93598820a0f2a7154ba83f5b041878c21f Change-Id: I27be1f93598820a0f2a7154ba83f5b041878c21f --- system/stack/btu/btu_hcif.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/system/stack/btu/btu_hcif.cc b/system/stack/btu/btu_hcif.cc index 9c317726257..6a4aecd667b 100644 --- a/system/stack/btu/btu_hcif.cc +++ b/system/stack/btu/btu_hcif.cc @@ -1038,6 +1038,20 @@ static void read_encryption_key_size_complete_after_encryption_change(uint8_t st return; } + if (btm_sec_is_session_key_size_downgrade(handle, key_size)) { + LOG_ERROR( + "encryption key size lower than cached value, disconnecting. " + "handle: 0x%x attempted key size: %d", + handle, key_size); + acl_disconnect_from_handle( + handle, HCI_ERR_HOST_REJECT_SECURITY, + "stack::btu::btu_hcif::read_encryption_key_size_complete_after_" + "encryption_change Key Size Downgrade"); + return; + } + + btm_sec_update_session_key_size(handle, key_size); + // good key size - succeed btm_acl_encrypt_change(handle, static_cast(status), 1 /* enable */); -- GitLab From c291a639c191334749f20df44de9fc6530902031 Mon Sep 17 00:00:00 2001 From: Himanshu Rawat Date: Mon, 8 Apr 2024 17:46:18 +0000 Subject: [PATCH 30/31] RESTRICT AUTOMERGE Disallow unexpected incoming HID connections HID profile accepted any new incoming HID connection. Even when the connection policy disabled HID connection, remote devices could initiate HID connection. This change ensures that incoming HID connection are accepted only if application was interested in that HID connection. This vulnerarbility no longer exists on the main because of feature request b/324093729. Test: mmm packages/modules/Bluetooth Test: Manual | Pair and connect a HID device, disable HID connection from Bluetooth device setting, attempt to connect from the HID device. Bug: 308429049 Ignore-AOSP-First: security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:03dca3305311096f157da3ab9cfed5cc30f2c135) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:431ef0346302dec8fa8c7d89c4696931e2bbac9a) Merged-In: I013d0528fb18ee87195fb3c8aab553c6a8da5ae4 Change-Id: I013d0528fb18ee87195fb3c8aab553c6a8da5ae4 --- .../jni/com_android_bluetooth_hid_host.cpp | 8 +- .../android/bluetooth/hid/HidHostService.java | 7 +- system/btif/include/btif_hh.h | 4 +- system/btif/include/btif_storage.h | 23 +++++ system/btif/src/btif_hh.cc | 86 +++++++++++++++++-- system/btif/src/btif_storage.cc | 51 ++++++++++- system/gd/rust/linux/stack/src/bluetooth.rs | 2 +- .../gd/rust/topshim/src/profiles/hid_host.rs | 2 +- system/include/hardware/bt_hh.h | 2 +- 9 files changed, 170 insertions(+), 15 deletions(-) diff --git a/android/app/jni/com_android_bluetooth_hid_host.cpp b/android/app/jni/com_android_bluetooth_hid_host.cpp index 7a164233bc1..18ba315129c 100644 --- a/android/app/jni/com_android_bluetooth_hid_host.cpp +++ b/android/app/jni/com_android_bluetooth_hid_host.cpp @@ -282,7 +282,8 @@ static jboolean connectHidNative(JNIEnv* env, jobject object, } static jboolean disconnectHidNative(JNIEnv* env, jobject object, - jbyteArray address) { + jbyteArray address, + jboolean reconnect_allowed) { jbyte* addr; jboolean ret = JNI_TRUE; if (!sBluetoothHidInterface) return JNI_FALSE; @@ -293,7 +294,8 @@ static jboolean disconnectHidNative(JNIEnv* env, jobject object, return JNI_FALSE; } - bt_status_t status = sBluetoothHidInterface->disconnect((RawAddress*)addr); + bt_status_t status = + sBluetoothHidInterface->disconnect((RawAddress*)addr, reconnect_allowed); if (status != BT_STATUS_SUCCESS) { ALOGE("Failed disconnect hid channel, status: %d", status); ret = JNI_FALSE; @@ -509,7 +511,7 @@ static JNINativeMethod sMethods[] = { {"initializeNative", "()V", (void*)initializeNative}, {"cleanupNative", "()V", (void*)cleanupNative}, {"connectHidNative", "([B)Z", (void*)connectHidNative}, - {"disconnectHidNative", "([B)Z", (void*)disconnectHidNative}, + {"disconnectHidNative", "([BZ)Z", (void*)disconnectHidNative}, {"getProtocolModeNative", "([B)Z", (void*)getProtocolModeNative}, {"virtualUnPlugNative", "([B)Z", (void*)virtualUnPlugNative}, {"setProtocolModeNative", "([BB)Z", (void*)setProtocolModeNative}, diff --git a/android/app/src/com/android/bluetooth/hid/HidHostService.java b/android/app/src/com/android/bluetooth/hid/HidHostService.java index 0c2c127061e..70a3befa20b 100644 --- a/android/app/src/com/android/bluetooth/hid/HidHostService.java +++ b/android/app/src/com/android/bluetooth/hid/HidHostService.java @@ -186,7 +186,10 @@ public class HidHostService extends ProfileService { break; case MESSAGE_DISCONNECT: { BluetoothDevice device = (BluetoothDevice) msg.obj; - if (!disconnectHidNative(getByteAddress(device))) { + int connectionPolicy = getConnectionPolicy(device); + boolean reconnectAllowed = + connectionPolicy == BluetoothProfile.CONNECTION_POLICY_ALLOWED; + if (!disconnectHidNative(getByteAddress(device), reconnectAllowed)) { broadcastConnectionState(device, BluetoothProfile.STATE_DISCONNECTING); broadcastConnectionState(device, BluetoothProfile.STATE_DISCONNECTED); break; @@ -1019,7 +1022,7 @@ public class HidHostService extends ProfileService { private native boolean connectHidNative(byte[] btAddress); - private native boolean disconnectHidNative(byte[] btAddress); + private native boolean disconnectHidNative(byte[] btAddress, boolean reconnectAllowed); private native boolean getProtocolModeNative(byte[] btAddress); diff --git a/system/btif/include/btif_hh.h b/system/btif/include/btif_hh.h index b2e125053bc..6e5fc806796 100644 --- a/system/btif/include/btif_hh.h +++ b/system/btif/include/btif_hh.h @@ -106,6 +106,7 @@ typedef struct { uint8_t dev_handle; RawAddress bd_addr; tBTA_HH_ATTR_MASK attr_mask; + bool reconnect_allowed; } btif_hh_added_device_t; /** @@ -130,7 +131,8 @@ extern btif_hh_cb_t btif_hh_cb; extern btif_hh_device_t* btif_hh_find_connected_dev_by_handle(uint8_t handle); extern void btif_hh_remove_device(RawAddress bd_addr); extern bool btif_hh_add_added_dev(const RawAddress& bda, - tBTA_HH_ATTR_MASK attr_mask); + tBTA_HH_ATTR_MASK attr_mask, + bool reconnect_allowed); extern bt_status_t btif_hh_virtual_unplug(const RawAddress* bd_addr); extern void btif_hh_disconnect(RawAddress* bd_addr); extern void btif_hh_setreport(btif_hh_device_t* p_dev, diff --git a/system/btif/include/btif_storage.h b/system/btif/include/btif_storage.h index 3e154b1d757..2675f723dda 100644 --- a/system/btif/include/btif_storage.h +++ b/system/btif/include/btif_storage.h @@ -196,6 +196,29 @@ void btif_storage_load_consolidate_devices(void); ******************************************************************************/ bt_status_t btif_storage_load_bonded_devices(void); +/******************************************************************************* + * + * Function btif_storage_set_hid_connection_policy + * + * Description Stores connection policy info in nvram + * + * Returns BT_STATUS_SUCCESS + * + ******************************************************************************/ +bt_status_t btif_storage_set_hid_connection_policy(const RawAddress& addr, + bool reconnect_allowed); +/******************************************************************************* + * + * Function btif_storage_get_hid_connection_policy + * + * Description get connection policy info from nvram + * + * Returns BT_STATUS_SUCCESS + * + ******************************************************************************/ +bt_status_t btif_storage_get_hid_connection_policy(const RawAddress& addr, + bool* reconnect_allowed); + /******************************************************************************* * * Function btif_storage_add_hid_device_info diff --git a/system/btif/src/btif_hh.cc b/system/btif/src/btif_hh.cc index 3280a957d25..f7e37692df8 100644 --- a/system/btif/src/btif_hh.cc +++ b/system/btif/src/btif_hh.cc @@ -308,6 +308,24 @@ btif_hh_device_t* btif_hh_find_connected_dev_by_handle(uint8_t handle) { return &btif_hh_cb.devices[i]; } } + return nullptr; +} + +/******************************************************************************* + * + * Function btif_hh_find_added_dev + * + * Description Return the added device pointer of the specified address + * + * Returns Added device entry + ******************************************************************************/ +btif_hh_added_device_t* btif_hh_find_added_dev(const RawAddress& addr) { + for (int i = 0; i < BTIF_HH_MAX_ADDED_DEV; i++) { + btif_hh_added_device_t* added_dev = &btif_hh_cb.added_devices[i]; + if (added_dev->bd_addr == addr) { + return added_dev; + } + } return NULL; } @@ -351,6 +369,23 @@ static btif_hh_device_t* btif_hh_find_connected_dev_by_bda( return NULL; } +static bool btif_hh_connection_allowed(const RawAddress& bda) { + /* Accept connection only if reconnection is allowed for the known device, or + * outgoing connection was requested */ + btif_hh_added_device_t* added_dev = btif_hh_find_added_dev(bda); + if (added_dev != nullptr && added_dev->reconnect_allowed) { + LOG_VERBOSE("Connection allowed %s", PRIVATE_ADDRESS(bda)); + return true; + } else if (btif_hh_cb.pending_conn_address == bda) { + LOG_VERBOSE("Device connection was pending for: %s, status: %s", + PRIVATE_ADDRESS(bda), + btif_hh_status_text(btif_hh_cb.status).c_str()); + return true; + } + + return false; +} + /******************************************************************************* * * Function btif_hh_stop_vup_timer @@ -396,7 +431,8 @@ void btif_hh_start_vup_timer(const RawAddress* bd_addr) { * * Returns true if add successfully, otherwise false. ******************************************************************************/ -bool btif_hh_add_added_dev(const RawAddress& bda, tBTA_HH_ATTR_MASK attr_mask) { +bool btif_hh_add_added_dev(const RawAddress& bda, tBTA_HH_ATTR_MASK attr_mask, + bool reconnect_allowed) { int i; for (i = 0; i < BTIF_HH_MAX_ADDED_DEV; i++) { if (btif_hh_cb.added_devices[i].bd_addr == bda) { @@ -410,6 +446,7 @@ bool btif_hh_add_added_dev(const RawAddress& bda, tBTA_HH_ATTR_MASK attr_mask) { btif_hh_cb.added_devices[i].bd_addr = bda; btif_hh_cb.added_devices[i].dev_handle = BTA_HH_INVALID_HANDLE; btif_hh_cb.added_devices[i].attr_mask = attr_mask; + btif_hh_cb.added_devices[i].reconnect_allowed = reconnect_allowed; return true; } } @@ -775,9 +812,26 @@ static void btif_hh_upstreams_evt(uint16_t event, char* p_param) { p_data->status); break; - case BTA_HH_OPEN_EVT: + case BTA_HH_OPEN_EVT: { BTIF_TRACE_WARNING("%s: BTA_HH_OPN_EVT: handle=%d, status =%d", __func__, p_data->conn.handle, p_data->conn.status); + + if (!btif_hh_connection_allowed(p_data->conn.bda)) { + LOG_WARN("Reject incoming HID Connection, device: %s", + PRIVATE_ADDRESS(p_data->conn.bda)); + btif_hh_device_t* p_dev = + btif_hh_find_connected_dev_by_handle(p_data->conn.handle); + if (p_dev != nullptr) { + p_dev->dev_status = BTHH_CONN_STATE_DISCONNECTED; + } + + btif_hh_cb.status = (BTIF_HH_STATUS)BTIF_HH_DEV_DISCONNECTED; + BTA_HhClose(p_data->conn.handle); + HAL_CBACK(bt_hh_callbacks, connection_state_cb, &p_data->conn.bda, + BTHH_CONN_STATE_DISCONNECTED); + return; + } + btif_hh_cb.pending_conn_address = RawAddress::kEmpty; if (p_data->conn.status == BTA_HH_OK) { p_dev = btif_hh_find_connected_dev_by_handle(p_data->conn.handle); @@ -836,6 +890,7 @@ static void btif_hh_upstreams_evt(uint16_t event, char* p_param) { btif_hh_cb.status = (BTIF_HH_STATUS)BTIF_HH_DEV_DISCONNECTED; } break; + } case BTA_HH_CLOSE_EVT: BTIF_TRACE_DEBUG("BTA_HH_CLOSE_EVT: status = %d, handle = %d", @@ -1009,7 +1064,7 @@ static void btif_hh_upstreams_evt(uint16_t event, char* p_param) { p_data->dscp_info.version, p_data->dscp_info.ctry_code, len, p_data->dscp_info.descriptor.dsc_list); - if (btif_hh_add_added_dev(p_dev->bd_addr, p_dev->attr_mask)) { + if (btif_hh_add_added_dev(p_dev->bd_addr, p_dev->attr_mask, true)) { tBTA_HH_DEV_DSCP_INFO dscp_info; bt_status_t ret; btif_hh_copy_hid_info(&dscp_info, &p_data->dscp_info); @@ -1025,6 +1080,8 @@ static void btif_hh_upstreams_evt(uint16_t event, char* p_param) { p_data->dscp_info.ssr_min_tout, len, p_data->dscp_info.descriptor.dsc_list); + btif_storage_set_hid_connection_policy(p_dev->bd_addr, true); + ASSERTC(ret == BT_STATUS_SUCCESS, "storing hid info failed", ret); BTIF_TRACE_WARNING("BTA_HH_GET_DSCP_EVT: Called add device"); @@ -1312,6 +1369,13 @@ static bt_status_t init(bthh_callbacks_t* callbacks) { ******************************************************************************/ static bt_status_t connect(RawAddress* bd_addr) { if (btif_hh_cb.status != BTIF_HH_DEV_CONNECTING) { + /* If the device was already added, ensure that reconnections are allowed */ + btif_hh_added_device_t* added_dev = btif_hh_find_added_dev(*bd_addr); + if (added_dev != nullptr && !added_dev->reconnect_allowed) { + added_dev->reconnect_allowed = true; + btif_storage_set_hid_connection_policy(*bd_addr, true); + } + btif_transfer_context(btif_hh_handle_evt, BTIF_HH_CONNECT_REQ_EVT, (char*)bd_addr, sizeof(RawAddress), NULL); return BT_STATUS_SUCCESS; @@ -1332,7 +1396,7 @@ static bt_status_t connect(RawAddress* bd_addr) { * Returns bt_status_t * ******************************************************************************/ -static bt_status_t disconnect(RawAddress* bd_addr) { +static bt_status_t disconnect(RawAddress* bd_addr, bool reconnect_allowed) { CHECK_BTHH_INIT(); BTIF_TRACE_EVENT("BTHH: %s", __func__); btif_hh_device_t* p_dev; @@ -1342,6 +1406,17 @@ static bt_status_t disconnect(RawAddress* bd_addr) { btif_hh_cb.status); return BT_STATUS_FAIL; } + + if (!reconnect_allowed) { + LOG_INFO("Incoming reconnections disabled for device %s", + PRIVATE_ADDRESS((*bd_addr))); + btif_hh_added_device_t* added_dev = btif_hh_find_added_dev(*bd_addr); + if (added_dev != nullptr && added_dev->reconnect_allowed) { + added_dev->reconnect_allowed = false; + btif_storage_set_hid_connection_policy(added_dev->bd_addr, false); + } + } + p_dev = btif_hh_find_connected_dev_by_bda(*bd_addr); if (p_dev != NULL) { return btif_transfer_context(btif_hh_handle_evt, BTIF_HH_DISCONNECT_REQ_EVT, @@ -1473,9 +1548,10 @@ static bt_status_t set_info(RawAddress* bd_addr, bthh_hid_info_t hid_info) { (uint8_t*)osi_malloc(dscp_info.descriptor.dl_len); memcpy(dscp_info.descriptor.dsc_list, &(hid_info.dsc_list), hid_info.dl_len); - if (btif_hh_add_added_dev(*bd_addr, hid_info.attr_mask)) { + if (btif_hh_add_added_dev(*bd_addr, hid_info.attr_mask, true)) { BTA_HhAddDev(*bd_addr, hid_info.attr_mask, hid_info.sub_class, hid_info.app_id, dscp_info); + btif_storage_set_hid_connection_policy(*bd_addr, true); } osi_free_and_reset((void**)&dscp_info.descriptor.dsc_list); diff --git a/system/btif/src/btif_storage.cc b/system/btif/src/btif_storage.cc index ae81bd1a38b..c3f28d809d1 100644 --- a/system/btif/src/btif_storage.cc +++ b/system/btif/src/btif_storage.cc @@ -104,6 +104,8 @@ using bluetooth::groups::DeviceGroups; #define BTIF_STORAGE_CSIS_SET_INFO_BIN "CsisSetInfoBin" #define BTIF_STORAGE_LEAUDIO_AUTOCONNECT "LeAudioAutoconnect" +#define BTIF_STORAGE_KEY_HID_RECONNECT_ALLOWED "HidReConnectAllowed" + /* This is a local property to add a device found */ #define BT_PROPERTY_REMOTE_DEVICE_TIMESTAMP 0xFF @@ -1498,6 +1500,49 @@ bool btif_storage_get_remote_device_type(const RawAddress& remote_bd_addr, return ret; } +/******************************************************************************* + * + * Function btif_storage_set_hid_connection_policy + * + * Description Stores connection policy info in nvram + * + * Returns BT_STATUS_SUCCESS + * + ******************************************************************************/ +bt_status_t btif_storage_set_hid_connection_policy(const RawAddress& addr, + bool reconnect_allowed) { + std::string bdstr = addr.ToString(); + + if (btif_config_set_int(bdstr, BTIF_STORAGE_KEY_HID_RECONNECT_ALLOWED, + reconnect_allowed)) { + return BT_STATUS_SUCCESS; + } else { + return BT_STATUS_FAIL; + } +} + +/******************************************************************************* + * + * Function btif_storage_get_hid_connection_policy + * + * Description get connection policy info from nvram + * + * Returns BT_STATUS_SUCCESS + * + ******************************************************************************/ +bt_status_t btif_storage_get_hid_connection_policy(const RawAddress& addr, + bool* reconnect_allowed) { + std::string bdstr = addr.ToString(); + + // For backward compatibility, assume that the reconnection is allowed in the + // absence of the key + int value = 1; + btif_config_get_int(bdstr, BTIF_STORAGE_KEY_HID_RECONNECT_ALLOWED, &value); + *reconnect_allowed = (value != 0); + + return BT_STATUS_SUCCESS; +} + /******************************************************************************* * * Function btif_storage_add_hid_device_info @@ -1593,8 +1638,11 @@ bt_status_t btif_storage_load_bonded_hid_info(void) { (uint8_t*)dscp_info.descriptor.dsc_list, &len); } + bool reconnect_allowed = false; + btif_storage_get_hid_connection_policy(bd_addr, &reconnect_allowed); + // add extracted information to BTA HH - if (btif_hh_add_added_dev(bd_addr, attr_mask)) { + if (btif_hh_add_added_dev(bd_addr, attr_mask, reconnect_allowed)) { BTA_HhAddDev(bd_addr, attr_mask, sub_class, app_id, dscp_info); } } @@ -1626,6 +1674,7 @@ bt_status_t btif_storage_remove_hid_info(const RawAddress& remote_bd_addr) { btif_config_remove(bdstr, "HidSSRMaxLatency"); btif_config_remove(bdstr, "HidSSRMinTimeout"); btif_config_remove(bdstr, "HidDescriptor"); + btif_config_remove(bdstr, BTIF_STORAGE_KEY_HID_RECONNECT_ALLOWED); btif_config_save(); return BT_STATUS_SUCCESS; } diff --git a/system/gd/rust/linux/stack/src/bluetooth.rs b/system/gd/rust/linux/stack/src/bluetooth.rs index 28216ff39ac..a58e68e7d86 100644 --- a/system/gd/rust/linux/stack/src/bluetooth.rs +++ b/system/gd/rust/linux/stack/src/bluetooth.rs @@ -1412,7 +1412,7 @@ impl IBluetooth for Bluetooth { if self.uuid_helper.is_profile_enabled(&p) { match p { Profile::Hid | Profile::Hogp => { - self.hh.as_ref().unwrap().disconnect(&mut addr.unwrap()); + self.hh.as_ref().unwrap().disconnect(&mut addr.unwrap(), true); } Profile::A2dpSink | Profile::A2dpSource => { diff --git a/system/gd/rust/topshim/src/profiles/hid_host.rs b/system/gd/rust/topshim/src/profiles/hid_host.rs index db447be9d24..15f1f27a1b3 100644 --- a/system/gd/rust/topshim/src/profiles/hid_host.rs +++ b/system/gd/rust/topshim/src/profiles/hid_host.rs @@ -208,7 +208,7 @@ impl HidHost { pub fn disconnect(&self, addr: &mut RawAddress) -> BtStatus { let ffi_addr = cast_to_ffi_address!(addr as *mut RawAddress); - BtStatus::from(ccall!(self, disconnect, ffi_addr)) + BtStatus::from(ccall!(self, disconnect, ffi_addr, true)) } pub fn virtual_unplug(&self, addr: &mut RawAddress) -> BtStatus { diff --git a/system/include/hardware/bt_hh.h b/system/include/hardware/bt_hh.h index dfe47f778aa..c64e465e374 100644 --- a/system/include/hardware/bt_hh.h +++ b/system/include/hardware/bt_hh.h @@ -173,7 +173,7 @@ typedef struct { bt_status_t (*connect)(RawAddress* bd_addr); /** dis-connect from hid device */ - bt_status_t (*disconnect)(RawAddress* bd_addr); + bt_status_t (*disconnect)(RawAddress* bd_addr, bool reconnect_allowed); /** Virtual UnPlug (VUP) the specified HID device */ bt_status_t (*virtual_unplug)(RawAddress* bd_addr); -- GitLab From a164c36d3d5312456a29d4ce8f2336227eae18cd Mon Sep 17 00:00:00 2001 From: Mohammed Althaf Thayyil Date: Mon, 21 Oct 2024 08:11:09 +0000 Subject: [PATCH 31/31] Update .gitlab-ci.yml with upstream --- .gitlab-ci.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 00eb1e4baf6..51503631100 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1,6 +1,11 @@ stages: - auto-merge-main + - update-from-upstream + include: - project: 'e/templates' ref: master file: '/gitlab-ci/.gitlab-ci-auto-merge-main.yml' + - project: 'e/templates' + ref: master + file: '/gitlab-ci/.gitlab-ci-import-updates-from-upstream.yml' -- GitLab