From 3168386ce9ffa0b731ae066043c4da1e6052df2d Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Fri, 12 Aug 2022 17:26:19 +0000 Subject: [PATCH 1/7] Add negative length check in process_service_search_rsp Bug: 225876506 Test: run supplied POC (updated to Android T) Tag: #security Ignore-AOSP-First: Security Change-Id: I0054806e47ed9d6eb8b034a41c8c872fee7f1eca (cherry picked from commit eac9616fc32f0bf40d2d2e6d1ff7b453edffc01c) Merged-In: I0054806e47ed9d6eb8b034a41c8c872fee7f1eca --- stack/sdp/sdp_discovery.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stack/sdp/sdp_discovery.cc b/stack/sdp/sdp_discovery.cc index e6075407e0..fc53fc7408 100644 --- a/stack/sdp/sdp_discovery.cc +++ b/stack/sdp/sdp_discovery.cc @@ -282,7 +282,7 @@ static void process_service_search_rsp(tCONN_CB* p_ccb, uint8_t* p_reply, orig = p_ccb->num_handles; p_ccb->num_handles += cur_handles; - if (p_ccb->num_handles == 0) { + if (p_ccb->num_handles == 0 || p_ccb->num_handles < orig) { SDP_TRACE_WARNING("SDP - Rcvd ServiceSearchRsp, no matches"); sdp_disconnect(p_ccb, SDP_NO_RECS_MATCH); return; From 681a61ed11edbf00dbaaae4f30580356af5337ab Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Wed, 7 Sep 2022 05:39:58 +0000 Subject: [PATCH 2/7] resolve merge conflicts of 9c93920de7a8d8106a1bc2e11ebfed79df66946b to sc-dev Ignore-AOSP-First: Security Test: I solemnly swear I tested this conflict resolution. Bug: 245420598 Change-Id: I029b5e310485e0180e4b7c7280db022848fcd646 (cherry picked from commit be2f542a6a1fb7f89b4562bc149cfdb8ac02748d) Merged-In: I029b5e310485e0180e4b7c7280db022848fcd646 --- btif/src/bluetooth.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/btif/src/bluetooth.cc b/btif/src/bluetooth.cc index a72f787f54..e684c1235d 100644 --- a/btif/src/bluetooth.cc +++ b/btif/src/bluetooth.cc @@ -359,11 +359,14 @@ static int get_connection_state(const RawAddress* bd_addr) { static int pin_reply(const RawAddress* bd_addr, uint8_t accept, uint8_t pin_len, bt_pin_code_t* pin_code) { + bt_pin_code_t tmp_pin_code; + /* sanity check */ if (!interface_ready()) return BT_STATUS_NOT_READY; if (pin_code == nullptr || pin_len > PIN_CODE_LEN) return BT_STATUS_FAIL; + memcpy(&tmp_pin_code, pin_code, pin_len); do_in_main_thread(FROM_HERE, base::BindOnce(btif_dm_pin_reply, *bd_addr, - accept, pin_len, *pin_code)); + accept, pin_len, tmp_pin_code)); return BT_STATUS_SUCCESS; } From 43a8cd063d35d87cf78b22e0e41b83d20bc8ef8d Mon Sep 17 00:00:00 2001 From: Ted Wang Date: Thu, 4 Aug 2022 09:41:24 +0800 Subject: [PATCH 3/7] Add length check when copy AVDTP packet Bug: 232023771 Test: make Tag: #security Ignore-AOSP-First: Security Change-Id: I68dd78c747eeafee5190dc56d7c71e9eeed08a5b Merged-In: I68dd78c747eeafee5190dc56d7c71e9eeed08a5b (cherry picked from commit a75b650a2a4b6b62be1ceb2040c598b0feb0dacb) Merged-In: I68dd78c747eeafee5190dc56d7c71e9eeed08a5b --- stack/avdt/avdt_msg.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/stack/avdt/avdt_msg.cc b/stack/avdt/avdt_msg.cc index bb49ede3e9..41b9f9a89d 100644 --- a/stack/avdt/avdt_msg.cc +++ b/stack/avdt/avdt_msg.cc @@ -1251,6 +1251,10 @@ BT_HDR* avdt_msg_asmbl(AvdtpCcb* p_ccb, BT_HDR* p_buf) { * would have allocated smaller buffer. */ p_ccb->p_rx_msg = (BT_HDR*)osi_malloc(BT_DEFAULT_BUFFER_SIZE); + if (sizeof(BT_HDR) + p_buf->offset + p_buf->len > BT_DEFAULT_BUFFER_SIZE) { + android_errorWriteLog(0x534e4554, "232023771"); + return NULL; + } memcpy(p_ccb->p_rx_msg, p_buf, sizeof(BT_HDR) + p_buf->offset + p_buf->len); /* Free original buffer */ From 17e38a1cf2d2f1a755ee36b108c743b32f69a2ba Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Thu, 25 Aug 2022 18:52:28 +0000 Subject: [PATCH 4/7] Added max buffer length check Bug: 230867224 Test: Manual -- paired Bluetooth headset and played audio Tags: #security Ignore-AOSP-First: Security Change-Id: I740038288143715a1c06db781efd674b269a7f3e (cherry picked from commit 2992109ab975def57192c5e3d40078e69b1e8717) Merged-In: I740038288143715a1c06db781efd674b269a7f3e --- stack/avct/avct_lcb_act.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/stack/avct/avct_lcb_act.cc b/stack/avct/avct_lcb_act.cc index 9e32ee4e9a..5c100ca395 100644 --- a/stack/avct/avct_lcb_act.cc +++ b/stack/avct/avct_lcb_act.cc @@ -67,7 +67,12 @@ static BT_HDR* avct_lcb_msg_asmbl(tAVCT_LCB* p_lcb, BT_HDR* p_buf) { pkt_type = AVCT_PKT_TYPE(p); /* quick sanity check on length */ - if (p_buf->len < avct_lcb_pkt_type_len[pkt_type]) { + if (p_buf->len < avct_lcb_pkt_type_len[pkt_type] || + (sizeof(BT_HDR) + p_buf->offset + p_buf->len) > BT_DEFAULT_BUFFER_SIZE) { + if ((sizeof(BT_HDR) + p_buf->offset + p_buf->len) > + BT_DEFAULT_BUFFER_SIZE) { + android_errorWriteWithInfoLog(0x534e4554, "230867224", -1, NULL, 0); + } osi_free(p_buf); AVCT_TRACE_WARNING("Bad length during reassembly"); p_ret = NULL; From f35ca9d4a35e1e855ccb3e99732001955eb3af44 Mon Sep 17 00:00:00 2001 From: Brian Delwiche Date: Thu, 25 Aug 2022 20:39:08 +0000 Subject: [PATCH 5/7] Add missing increment in bnep_api.cc Bug: 228450451 Test: manual, pair BT and play audio Tag: #security Ignore-AOSP-First: Security Change-Id: I681878508feae3d0526ed3e928af7a415e7d5c36 (cherry picked from commit 0fa54c7d8a2c061202e61d75b805661c1e89a76d) Merged-In: I681878508feae3d0526ed3e928af7a415e7d5c36 --- stack/bnep/bnep_api.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/stack/bnep/bnep_api.cc b/stack/bnep/bnep_api.cc index 455dc168d6..0996370897 100644 --- a/stack/bnep/bnep_api.cc +++ b/stack/bnep/bnep_api.cc @@ -256,6 +256,7 @@ tBNEP_RESULT BNEP_ConnectResp(uint16_t handle, tBNEP_RESULT resp) { p = (uint8_t*)(p_bcb->p_pending_data + 1) + p_bcb->p_pending_data->offset; while (extension_present && p && rem_len) { ext_type = *p++; + rem_len--; extension_present = ext_type >> 7; ext_type &= 0x7F; From 359f0a779d4bc96ccc131da034a9d99cb9a4af0d Mon Sep 17 00:00:00 2001 From: Keith Mok Date: Tue, 16 Aug 2022 21:41:03 +0000 Subject: [PATCH 6/7] Add length check when copy AVDT and AVCT packet Previous fix for AVDT causing memory leak. And missing similar fix for AVCT packet. Bug: 232023771 Test: make Tag: #security Ignore-AOSP-First: Security Merged-In: Ifa8ed1cd9ea118acba78bdfdf6d5861fad254a90 Change-Id: Ifa8ed1cd9ea118acba78bdfdf6d5861fad254a90 (cherry picked from commit 62986e6a11a7340925d79c4282513aebc28da176) Merged-In: Ifa8ed1cd9ea118acba78bdfdf6d5861fad254a90 --- stack/avct/avct_lcb_act.cc | 8 +++++++- stack/avdt/avdt_msg.cc | 6 ++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/stack/avct/avct_lcb_act.cc b/stack/avct/avct_lcb_act.cc index 5c100ca395..1b4197861d 100644 --- a/stack/avct/avct_lcb_act.cc +++ b/stack/avct/avct_lcb_act.cc @@ -93,13 +93,19 @@ static BT_HDR* avct_lcb_msg_asmbl(tAVCT_LCB* p_lcb, BT_HDR* p_buf) { if (p_lcb->p_rx_msg != NULL) AVCT_TRACE_WARNING("Got start during reassembly"); - osi_free(p_lcb->p_rx_msg); + osi_free_and_reset((void**)&p_lcb->p_rx_msg); /* * Allocate bigger buffer for reassembly. As lower layers are * not aware of possible packet size after reassembly, they * would have allocated smaller buffer. */ + if (sizeof(BT_HDR) + p_buf->offset + p_buf->len > BT_DEFAULT_BUFFER_SIZE) { + android_errorWriteLog(0x534e4554, "232023771"); + osi_free(p_buf); + p_ret = NULL; + return p_ret; + } p_lcb->p_rx_msg = (BT_HDR*)osi_malloc(BT_DEFAULT_BUFFER_SIZE); memcpy(p_lcb->p_rx_msg, p_buf, sizeof(BT_HDR) + p_buf->offset + p_buf->len); diff --git a/stack/avdt/avdt_msg.cc b/stack/avdt/avdt_msg.cc index 41b9f9a89d..d0c54343c3 100644 --- a/stack/avdt/avdt_msg.cc +++ b/stack/avdt/avdt_msg.cc @@ -1250,11 +1250,13 @@ BT_HDR* avdt_msg_asmbl(AvdtpCcb* p_ccb, BT_HDR* p_buf) { * not aware of possible packet size after reassembly, they * would have allocated smaller buffer. */ - p_ccb->p_rx_msg = (BT_HDR*)osi_malloc(BT_DEFAULT_BUFFER_SIZE); if (sizeof(BT_HDR) + p_buf->offset + p_buf->len > BT_DEFAULT_BUFFER_SIZE) { android_errorWriteLog(0x534e4554, "232023771"); - return NULL; + osi_free(p_buf); + p_ret = NULL; + return p_ret; } + p_ccb->p_rx_msg = (BT_HDR*)osi_malloc(BT_DEFAULT_BUFFER_SIZE); memcpy(p_ccb->p_rx_msg, p_buf, sizeof(BT_HDR) + p_buf->offset + p_buf->len); /* Free original buffer */ From 62c3813b432cf642c8ebcb5d082333b7b7f42558 Mon Sep 17 00:00:00 2001 From: Keith Mok Date: Sun, 14 Aug 2022 23:08:20 +0000 Subject: [PATCH 7/7] Fix integer overflow when parsing avrc response Convert min_len from 16 bits to 32 bits to avoid length checking overflow. Also, use calloc instead of malloc for list allocation since caller need to clean up string memory in the list items Bug: 242459126 Test: fuzz_avrc Tag: #security Ignore-AOSP-First: Security Merged-In: I7250509f2b320774926a8b24fd28828c5217d8a4 Change-Id: I7250509f2b320774926a8b24fd28828c5217d8a4 (cherry picked from commit 44df45d0385f501150b2221c1c7a02a4d7f5b6d1) Merged-In: I7250509f2b320774926a8b24fd28828c5217d8a4 --- stack/avdt/avdt_scb_act.cc | 2 +- stack/avrc/avrc_pars_ct.cc | 39 +++++++++++--------------------------- stack/avrc/avrc_pars_tg.cc | 2 +- 3 files changed, 13 insertions(+), 30 deletions(-) diff --git a/stack/avdt/avdt_scb_act.cc b/stack/avdt/avdt_scb_act.cc index fdf5570abd..fcb082bc19 100644 --- a/stack/avdt/avdt_scb_act.cc +++ b/stack/avdt/avdt_scb_act.cc @@ -310,7 +310,7 @@ uint8_t* avdt_scb_hdl_report(AvdtpScb* p_scb, uint8_t* p, uint16_t len) { uint8_t* p_start = p; uint32_t ssrc; uint8_t o_v, o_p, o_cc; - uint16_t min_len = 0; + uint32_t min_len = 0; AVDT_REPORT_TYPE pt; tAVDT_REPORT_DATA report; diff --git a/stack/avrc/avrc_pars_ct.cc b/stack/avrc/avrc_pars_ct.cc index db3ee75947..2d6f8b2642 100644 --- a/stack/avrc/avrc_pars_ct.cc +++ b/stack/avrc/avrc_pars_ct.cc @@ -141,7 +141,7 @@ static tAVRC_STS avrc_pars_vendor_rsp(tAVRC_MSG_VENDOR* p_msg, tAVRC_STS avrc_parse_notification_rsp(uint8_t* p_stream, uint16_t len, tAVRC_REG_NOTIF_RSP* p_rsp) { - uint16_t min_len = 1; + uint32_t min_len = 1; if (len < min_len) goto length_error; BE_STREAM_TO_UINT8(p_rsp->event_id, p_stream); @@ -237,7 +237,7 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, } BE_STREAM_TO_UINT8(pdu, p); uint16_t pkt_len; - uint16_t min_len = 0; + uint32_t min_len = 0; /* read the entire packet len */ BE_STREAM_TO_UINT16(pkt_len, p); @@ -279,7 +279,7 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, get_item_rsp->uid_counter, get_item_rsp->item_count); /* get each of the items */ - get_item_rsp->p_item_list = (tAVRC_ITEM*)osi_malloc( + get_item_rsp->p_item_list = (tAVRC_ITEM*)osi_calloc( get_item_rsp->item_count * (sizeof(tAVRC_ITEM))); tAVRC_ITEM* curr_item = get_item_rsp->p_item_list; for (int i = 0; i < get_item_rsp->item_count; i++) { @@ -369,7 +369,7 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, __func__, media->type, media->name.charset_id, media->name.str_len, media->attr_count); - media->p_attr_list = (tAVRC_ATTR_ENTRY*)osi_malloc( + media->p_attr_list = (tAVRC_ATTR_ENTRY*)osi_calloc( media->attr_count * sizeof(tAVRC_ATTR_ENTRY)); for (int jk = 0; jk < media->attr_count; jk++) { tAVRC_ATTR_ENTRY* attr_entry = &(media->p_attr_list[jk]); @@ -380,14 +380,8 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, /* Parse the name now */ BE_STREAM_TO_UINT16(attr_entry->name.charset_id, p); BE_STREAM_TO_UINT16(attr_entry->name.str_len, p); - if (static_cast(min_len + attr_entry->name.str_len) < - min_len) { - // Check for overflow - android_errorWriteLog(0x534e4554, "205570663"); - } - if (pkt_len - min_len < attr_entry->name.str_len) - goto browse_length_error; min_len += attr_entry->name.str_len; + if (pkt_len < min_len) goto browse_length_error; attr_entry->name.p_str = (uint8_t*)osi_malloc( attr_entry->name.str_len * sizeof(uint8_t)); BE_STREAM_TO_ARRAY(p, attr_entry->name.p_str, @@ -441,7 +435,7 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, } BE_STREAM_TO_UINT8(get_attr_rsp->status, p) BE_STREAM_TO_UINT8(get_attr_rsp->num_attrs, p); - get_attr_rsp->p_attrs = (tAVRC_ATTR_ENTRY*)osi_malloc( + get_attr_rsp->p_attrs = (tAVRC_ATTR_ENTRY*)osi_calloc( get_attr_rsp->num_attrs * sizeof(tAVRC_ATTR_ENTRY)); for (int i = 0; i < get_attr_rsp->num_attrs; i++) { tAVRC_ATTR_ENTRY* attr_entry = &(get_attr_rsp->p_attrs[i]); @@ -450,14 +444,8 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, BE_STREAM_TO_UINT32(attr_entry->attr_id, p); BE_STREAM_TO_UINT16(attr_entry->name.charset_id, p); BE_STREAM_TO_UINT16(attr_entry->name.str_len, p); - if (static_cast(min_len + attr_entry->name.str_len) < - min_len) { - // Check for overflow - android_errorWriteLog(0x534e4554, "205570663"); - } - if (pkt_len - min_len < attr_entry->name.str_len) - goto browse_length_error; min_len += attr_entry->name.str_len; + if (pkt_len < min_len) goto browse_length_error; attr_entry->name.p_str = (uint8_t*)osi_malloc(attr_entry->name.str_len * sizeof(uint8_t)); BE_STREAM_TO_ARRAY(p, attr_entry->name.p_str, attr_entry->name.str_len); @@ -493,7 +481,7 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, __func__, set_br_pl_rsp->status, set_br_pl_rsp->num_items, set_br_pl_rsp->charset_id, set_br_pl_rsp->folder_depth); - set_br_pl_rsp->p_folders = (tAVRC_NAME*)osi_malloc( + set_br_pl_rsp->p_folders = (tAVRC_NAME*)osi_calloc( set_br_pl_rsp->folder_depth * sizeof(tAVRC_NAME)); /* Read each of the folder in the depth */ @@ -553,7 +541,7 @@ static tAVRC_STS avrc_ctrl_pars_vendor_rsp(tAVRC_MSG_VENDOR* p_msg, p++; /* skip the reserved/packe_type byte */ uint16_t len; - uint16_t min_len = 0; + uint32_t min_len = 0; BE_STREAM_TO_UINT16(len, p); AVRC_TRACE_DEBUG("%s ctype:0x%x pdu:0x%x, len:%d vendor_len=0x%x", __func__, p_msg->hdr.ctype, p_result->pdu, len, p_msg->vendor_len); @@ -827,12 +815,8 @@ static tAVRC_STS avrc_ctrl_pars_vendor_rsp(tAVRC_MSG_VENDOR* p_msg, BE_STREAM_TO_UINT32(p_attrs[i].attr_id, p); BE_STREAM_TO_UINT16(p_attrs[i].name.charset_id, p); BE_STREAM_TO_UINT16(p_attrs[i].name.str_len, p); - if (static_cast(min_len + p_attrs[i].name.str_len) < - min_len) { - // Check for overflow - android_errorWriteLog(0x534e4554, "205570663"); - } - if (len - min_len < p_attrs[i].name.str_len) { + min_len += p_attrs[i].name.str_len; + if (len < min_len) { for (int j = 0; j < i; j++) { osi_free(p_attrs[j].name.p_str); } @@ -840,7 +824,6 @@ static tAVRC_STS avrc_ctrl_pars_vendor_rsp(tAVRC_MSG_VENDOR* p_msg, p_result->get_attrs.num_attrs = 0; goto length_error; } - min_len += p_attrs[i].name.str_len; if (p_attrs[i].name.str_len > 0) { p_attrs[i].name.p_str = (uint8_t*)osi_calloc(p_attrs[i].name.str_len); diff --git a/stack/avrc/avrc_pars_tg.cc b/stack/avrc/avrc_pars_tg.cc index 8d239b21e1..fdd1c06cb3 100644 --- a/stack/avrc/avrc_pars_tg.cc +++ b/stack/avrc/avrc_pars_tg.cc @@ -437,7 +437,7 @@ static tAVRC_STS avrc_pars_browsing_cmd(tAVRC_MSG_BROWSE* p_msg, uint8_t* p = p_msg->p_browse_data; int count; - uint16_t min_len = 3; + uint32_t min_len = 3; RETURN_STATUS_IF_FALSE(AVRC_STS_BAD_CMD, (p_msg->browse_len >= min_len), "msg too short");