From abcf06b00640b5fe7d45318c601796660751879b Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Mon, 23 Sep 2024 18:22:36 +0000 Subject: [PATCH 1/2] Use encrypted link for avdtp and avctp channels This is a backport of the AOSP changes for b/345258562. Test: mmm packages/modules/Bluetooth Bug: 345258562 Ignore-AOSP-First: security Tag: #security (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:d26e79ab057057cbfcafc5eaeaaf1581afebd007) Merged-In: I4ef23f9dec4aaae6a526c11a7c2489159bd7fdf8 Change-Id: I4ef23f9dec4aaae6a526c11a7c2489159bd7fdf8 --- system/stack/avct/avct_api.cc | 6 ++++-- system/stack/avct/avct_bcb_act.cc | 5 +++-- system/stack/avct/avct_lcb_act.cc | 3 ++- system/stack/avdt/avdt_ad.cc | 3 ++- system/stack/avdt/avdt_api.cc | 3 ++- 5 files changed, 13 insertions(+), 7 deletions(-) diff --git a/system/stack/avct/avct_api.cc b/system/stack/avct/avct_api.cc index b52e1afdf45..32542640926 100644 --- a/system/stack/avct/avct_api.cc +++ b/system/stack/avct/avct_api.cc @@ -63,9 +63,11 @@ void AVCT_Register() { /* initialize AVCTP data structures */ memset(&avct_cb, 0, sizeof(tAVCT_CB)); + uint16_t sec = BTA_SEC_AUTHENTICATE | BTA_SEC_ENCRYPT; + /* register PSM with L2CAP */ L2CA_Register2(AVCT_PSM, avct_l2c_appl, true /* enable_snoop */, nullptr, - kAvrcMtu, 0, BTA_SEC_AUTHENTICATE); + kAvrcMtu, 0, sec); /* Include the browsing channel which uses eFCR */ tL2CAP_ERTM_INFO ertm_info; @@ -73,7 +75,7 @@ void AVCT_Register() { L2CA_Register2(AVCT_BR_PSM, avct_l2c_br_appl, true /*enable_snoop*/, &ertm_info, kAvrcBrMtu, AVCT_MIN_BROWSE_MTU, - BTA_SEC_AUTHENTICATE); + sec); avct_cb.trace_level = avct_trace_level; } diff --git a/system/stack/avct/avct_bcb_act.cc b/system/stack/avct/avct_bcb_act.cc index c278f48a834..5ba20397a0e 100644 --- a/system/stack/avct/avct_bcb_act.cc +++ b/system/stack/avct/avct_bcb_act.cc @@ -115,8 +115,9 @@ void avct_bcb_chnl_open(tAVCT_BCB* p_bcb, UNUSED_ATTR tAVCT_LCB_EVT* p_data) { /* call l2cap connect req */ p_bcb->ch_state = AVCT_CH_CONN; - p_bcb->ch_lcid = - L2CA_ConnectReq2(AVCT_BR_PSM, p_lcb->peer_addr, BTA_SEC_AUTHENTICATE); + p_bcb->ch_lcid = L2CA_ConnectReq2(AVCT_BR_PSM, p_lcb->peer_addr, + BTA_SEC_AUTHENTICATE | BTA_SEC_ENCRYPT); + if (p_bcb->ch_lcid == 0) { /* if connect req failed, send ourselves close event */ tAVCT_LCB_EVT avct_lcb_evt; diff --git a/system/stack/avct/avct_lcb_act.cc b/system/stack/avct/avct_lcb_act.cc index 58f40b8e8f9..d7a8f1283d9 100644 --- a/system/stack/avct/avct_lcb_act.cc +++ b/system/stack/avct/avct_lcb_act.cc @@ -190,7 +190,8 @@ void avct_lcb_chnl_open(tAVCT_LCB* p_lcb, UNUSED_ATTR tAVCT_LCB_EVT* p_data) { p_lcb->ch_state = AVCT_CH_CONN; p_lcb->ch_lcid = - L2CA_ConnectReq2(AVCT_PSM, p_lcb->peer_addr, BTA_SEC_AUTHENTICATE); + L2CA_ConnectReq2(AVCT_PSM, p_lcb->peer_addr, + BTA_SEC_AUTHENTICATE | BTA_SEC_ENCRYPT); if (p_lcb->ch_lcid == 0) { /* if connect req failed, send ourselves close event */ tAVCT_LCB_EVT avct_lcb_evt; diff --git a/system/stack/avdt/avdt_ad.cc b/system/stack/avdt/avdt_ad.cc index ccfb12945cb..9d2d6779ca3 100644 --- a/system/stack/avdt/avdt_ad.cc +++ b/system/stack/avdt/avdt_ad.cc @@ -548,7 +548,8 @@ void avdt_ad_open_req(uint8_t type, AvdtpCcb* p_ccb, AvdtpScb* p_scb, /* call l2cap connect req */ lcid = - L2CA_ConnectReq2(AVDT_PSM, p_ccb->peer_addr, BTM_SEC_OUT_AUTHENTICATE); + L2CA_ConnectReq2(AVDT_PSM, p_ccb->peer_addr, + BTM_SEC_OUT_AUTHENTICATE | BTM_SEC_OUT_ENCRYPT); if (lcid != 0) { /* if connect req ok, store tcid in lcid table */ avdtp_cb.ad.lcid_tbl[lcid] = avdt_ad_tc_tbl_to_idx(p_tbl); diff --git a/system/stack/avdt/avdt_api.cc b/system/stack/avdt/avdt_api.cc index a5f53885e84..ea4d2acd007 100644 --- a/system/stack/avdt/avdt_api.cc +++ b/system/stack/avdt/avdt_api.cc @@ -95,9 +95,10 @@ void avdt_scb_transport_channel_timer_timeout(void* data) { * ******************************************************************************/ void AVDT_Register(AvdtpRcb* p_reg, tAVDT_CTRL_CBACK* p_cback) { + uint16_t sec = BTA_SEC_AUTHENTICATE | BTA_SEC_ENCRYPT; /* register PSM with L2CAP */ L2CA_Register2(AVDT_PSM, avdt_l2c_appl, true /* enable_snoop */, nullptr, - kAvdtpMtu, 0, BTA_SEC_AUTHENTICATE); + kAvdtpMtu, 0, sec); /* initialize AVDTP data structures */ avdt_scb_init(); -- GitLab From b2739368e7160de3a5c5fc33a0dfb10220226997 Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Wed, 9 Oct 2024 20:55:36 +0000 Subject: [PATCH 2/2] Fix UAF in sdp_discovery.cc It is possible with modifications to a client to open two connections against the same SDP discovery database. If this happens, it becomes possible to reference a freed instance of the discovery database in the second connection once the first one is closed. To guard against this, check during discovery if a database has already been allocated, and abort iff it has. Also, add a null check to process_service_search_attr_rsp to guard against unchecked calls to the SDP discovery database. Bug: 291281168 Bug: 356201480 Flag: com.android.bluetooth.flags.btsec_check_valid_discovery_database 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:2c3b80e41630d9a252e63a3e30cc2997488fc3e4) Merged-In: I754bf8292e1e0d8e90e78fa87889284e26aa5818 Change-Id: I754bf8292e1e0d8e90e78fa87889284e26aa5818 --- system/bta/hf_client/bta_hf_client_sdp.cc | 13 +++++++++++++ system/stack/sdp/sdp_discovery.cc | 9 +++++++++ 2 files changed, 22 insertions(+) diff --git a/system/bta/hf_client/bta_hf_client_sdp.cc b/system/bta/hf_client/bta_hf_client_sdp.cc index 96b7a6d14bc..72e0a441b0c 100755 --- a/system/bta/hf_client/bta_hf_client_sdp.cc +++ b/system/bta/hf_client/bta_hf_client_sdp.cc @@ -320,6 +320,19 @@ void bta_hf_client_do_disc(tBTA_HF_CLIENT_CB* client_cb) { uuid_list[0] = Uuid::From16Bit(UUID_SERVCLASS_AG_HANDSFREE); } + /* If we already have a non-null discovery database at this point, we can get + * into a race condition leading to UAF once this connection is closed. + * This should only happen with malicious modifications to a client. */ + if (client_cb->p_disc_db != NULL) { + APPL_TRACE_ERROR( + "Tried to set up a HF client with a preexisting discovery database."); + client_cb->p_disc_db = NULL; + // We manually set the state here because it's possible to call this from an + // OPEN state, in which case the discovery fail event will be ignored. + client_cb->state = 0; // BTA_HF_CLIENT_INIT_ST + return; + } + /* allocate buffer for sdp database */ client_cb->p_disc_db = (tSDP_DISCOVERY_DB*)osi_malloc(BT_DEFAULT_BUFFER_SIZE); diff --git a/system/stack/sdp/sdp_discovery.cc b/system/stack/sdp/sdp_discovery.cc index 38db6350b2e..85d3dd3c24d 100644 --- a/system/stack/sdp/sdp_discovery.cc +++ b/system/stack/sdp/sdp_discovery.cc @@ -604,6 +604,15 @@ static void process_service_search_attr_rsp(tCONN_CB* p_ccb, uint8_t* p_reply, uint8_t* p; uint16_t bytes_left = SDP_DATA_BUF_SIZE; + /* If we don't have a valid discovery database, we can't do anything. */ + if (p_ccb->p_db == NULL) { + SDP_TRACE_WARNING( + "Attempted continuation or first time request with invalid discovery " + "database"); + sdp_disconnect(p_ccb, tSDP_STATUS::SDP_INVALID_CONT_STATE); + return; + } + p_msg->offset = L2CAP_MIN_OFFSET; p = p_start = (uint8_t*)(p_msg + 1) + L2CAP_MIN_OFFSET; -- GitLab