From 2e04897e9fcddd1debb6c380a4fbd255a3ead60f Mon Sep 17 00:00:00 2001 From: Hansong Zhang Date: Fri, 30 Mar 2018 16:27:37 -0700 Subject: DO NOT MERGE Fix unexpected behavior in smp_sm_event Bug: 74121126 Test: manual Change-Id: Ie5dd841d6461ad057c4ab572007f38c5446aba53 (cherry picked from commit 652798b2f2d6c90e0fc95c00ccfb91e2870b03d4) --- stack/smp/smp_main.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/stack/smp/smp_main.cc b/stack/smp/smp_main.cc index 829a5d48f..49e2ece7b 100644 --- a/stack/smp/smp_main.cc +++ b/stack/smp/smp_main.cc @@ -18,6 +18,7 @@ #include "bt_target.h" +#include #include #include "smp_int.h" @@ -954,6 +955,13 @@ void smp_sm_event(tSMP_CB* p_cb, tSMP_EVENT event, void* p_data) { uint8_t curr_state = p_cb->state; tSMP_SM_TBL state_table; uint8_t action, entry, i; + + if (p_cb->role >= 2) { + SMP_TRACE_DEBUG("Invalid role: %d", p_cb->role); + android_errorWriteLog(0x534e4554, "74121126"); + return; + } + tSMP_ENTRY_TBL entry_table = smp_entry_table[p_cb->role]; SMP_TRACE_EVENT("main smp_sm_event"); -- cgit v1.2.3 From 5635aa75510b91e44eed128a622d0da8c17f2711 Mon Sep 17 00:00:00 2001 From: Hansong Zhang Date: Mon, 2 Apr 2018 10:05:56 -0700 Subject: DO NOT MERGE Fix unexpected behavior in bta_dm_sdp_result Check the number of UUIDs from remote device Bug: 74016921 Test: manual Change-Id: I1ca1f66bfc935f5fd219e8147511bdac7d2789ef (cherry picked from commit 67ec216daa43f71adf103de6c4156c5a892c1460) --- bta/dm/bta_dm_act.cc | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/bta/dm/bta_dm_act.cc b/bta/dm/bta_dm_act.cc index 187bc68e0..175e3faf2 100644 --- a/bta/dm/bta_dm_act.cc +++ b/bta/dm/bta_dm_act.cc @@ -28,6 +28,7 @@ #include #include #include +#include #include #include "bt_common.h" @@ -146,6 +147,8 @@ static void bta_dm_ctrl_features_rd_cmpl_cback(tBTM_STATUS result); #define BTA_DM_SWITCH_DELAY_TIMER_MS 500 #endif +#define BTA_MAX_SERVICES 32 + static void bta_dm_reset_sec_dev_pending(const RawAddress& remote_bd_addr); static void bta_dm_remove_sec_dev_entry(const RawAddress& remote_bd_addr); static void bta_dm_observe_results_cb(tBTM_INQ_RESULTS* p_inq, uint8_t* p_eir, @@ -1486,7 +1489,7 @@ void bta_dm_sdp_result(tBTA_DM_MSG* p_data) { tBT_UUID service_uuid; uint32_t num_uuids = 0; - uint8_t uuid_list[32][MAX_UUID_SIZE]; // assuming a max of 32 services + uint8_t uuid_list[BTA_MAX_SERVICES][MAX_UUID_SIZE]; // assuming a max of 32 services if ((p_data->sdp_event.sdp_result == SDP_SUCCESS) || (p_data->sdp_event.sdp_result == SDP_NO_RECS_MATCH) || @@ -1554,8 +1557,12 @@ void bta_dm_sdp_result(tBTA_DM_MSG* p_data) { bta_service_id_to_uuid_lkup_tbl[bta_dm_search_cb.service_index - 1]; /* Add to the list of UUIDs */ - sdpu_uuid16_to_uuid128(tmp_svc, uuid_list[num_uuids]); - num_uuids++; + if (num_uuids < BTA_MAX_SERVICES) { + sdpu_uuid16_to_uuid128(tmp_svc, uuid_list[num_uuids]); + num_uuids++; + } else { + android_errorWriteLog(0x534e4554, "74016921"); + } } } } @@ -1587,8 +1594,12 @@ void bta_dm_sdp_result(tBTA_DM_MSG* p_data) { SDP_FindServiceInDb_128bit(bta_dm_search_cb.p_sdp_db, p_sdp_rec); if (p_sdp_rec) { if (SDP_FindServiceUUIDInRec_128bit(p_sdp_rec, &temp_uuid)) { - memcpy(uuid_list[num_uuids], temp_uuid.uu.uuid128, MAX_UUID_SIZE); - num_uuids++; + if (num_uuids < BTA_MAX_SERVICES) { + memcpy(uuid_list[num_uuids], temp_uuid.uu.uuid128, MAX_UUID_SIZE); + num_uuids++; + } else { + android_errorWriteLog(0x534e4554, "74016921"); + } } } } while (p_sdp_rec); -- cgit v1.2.3 From ac7150038a1050081971efe36d77af0d199ec730 Mon Sep 17 00:00:00 2001 From: Stanley Tng Date: Wed, 28 Mar 2018 17:12:28 -0700 Subject: DO NOT MERGE Drop LE CoC fragments when frame size is too big Drop the LE CoC data fragments when the received fragment size is too big. Test: Runs LE CoC SL4A test, BleCocTest. Bug: 75298652 Merged-In: I529944341e9e67a39e7ec7e740d5ada3db8cc23a Change-Id: I529944341e9e67a39e7ec7e740d5ada3db8cc23a (cherry picked from commit 8365a2ace5e89d8b81bab468f0f9bc1137d773b4) (cherry picked from commit 17db92e4fc3c7127c0ace625ff9735a9972eee70) --- stack/l2cap/l2c_fcr.cc | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/stack/l2cap/l2c_fcr.cc b/stack/l2cap/l2c_fcr.cc index 0e5a84a50..9c2742f56 100644 --- a/stack/l2cap/l2c_fcr.cc +++ b/stack/l2cap/l2c_fcr.cc @@ -24,6 +24,7 @@ ******************************************************************************/ #include +#include #include #include #include @@ -855,8 +856,24 @@ void l2c_lcc_proc_pdu(tL2C_CCB* p_ccb, BT_HDR* p_buf) { p_buf->offset += sizeof(sdu_length); p_data->offset = 0; - } else + } else { p_data = p_ccb->ble_sdu; + if (p_buf->len > (p_ccb->ble_sdu_length - p_data->len)) { + L2CAP_TRACE_ERROR("%s: buffer length=%d too big. max=%d. Dropped", + __func__, p_data->len, + (p_ccb->ble_sdu_length - p_data->len)); + android_errorWriteWithInfoLog(0x534e4554, "75298652", -1, NULL, 0); + osi_free(p_buf); + + /* Throw away all pending fragments and disconnects */ + p_ccb->is_first_seg = true; + osi_free(p_ccb->ble_sdu); + p_ccb->ble_sdu = NULL; + p_ccb->ble_sdu_length = 0; + l2cu_disconnect_chnl(p_ccb); + return; + } + } memcpy((uint8_t*)(p_data + 1) + p_data->offset + p_data->len, (uint8_t*)(p_buf + 1) + p_buf->offset, p_buf->len); @@ -869,9 +886,6 @@ void l2c_lcc_proc_pdu(tL2C_CCB* p_ccb, BT_HDR* p_buf) { p_ccb->ble_sdu_length = 0; } else if (p_data->len < p_ccb->ble_sdu_length) { p_ccb->is_first_seg = false; - } else { - L2CAP_TRACE_ERROR("%s Length in the SDU messed up", __func__); - // TODO: reset every thing may be??? } osi_free(p_buf); -- cgit v1.2.3 From df480029a5f5cb6405558763b29b9adb6b19688e Mon Sep 17 00:00:00 2001 From: Stanley Tng Date: Thu, 5 Apr 2018 09:54:13 -0700 Subject: DO NOT MERGE Handle bad packet length in gatts_process_read_req Added error check and handling code in gatts_process_read_req to make sure that the packet length is correct. Please note that there is another earlier CL that is reverted and this is the updated one. Bug: 73172115 Test: Run the test program, poc, that was attached in the bug report Merged-In: Ia9b4e502fa8f8384bf9767e68f73b48a0915141b Change-Id: Ia9b4e502fa8f8384bf9767e68f73b48a0915141b (cherry picked from commit cc9c7330d1c3507d745170ae7b2e0546197b7acb) (cherry picked from commit 16f4c21be5bd0ea1968eee8a0f00648b1e326253) --- stack/gatt/gatt_sr.cc | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/stack/gatt/gatt_sr.cc b/stack/gatt/gatt_sr.cc index 3af986639..06c9c5205 100644 --- a/stack/gatt/gatt_sr.cc +++ b/stack/gatt/gatt_sr.cc @@ -22,6 +22,7 @@ * ******************************************************************************/ +#include #include "bt_target.h" #include "bt_utils.h" #include "osi/include/osi.h" @@ -281,8 +282,8 @@ tGATT_STATUS gatt_sr_process_app_rsp(tGATT_TCB& tcb, tGATT_IF gatt_if, * Returns void * ******************************************************************************/ -void gatt_process_exec_write_req(tGATT_TCB& tcb, uint8_t op_code, - UNUSED_ATTR uint16_t len, uint8_t* p_data) { +void gatt_process_exec_write_req(tGATT_TCB& tcb, uint8_t op_code, uint16_t len, + uint8_t* p_data) { uint8_t *p = p_data, flag, i = 0; uint32_t trans_id = 0; tGATT_IF gatt_if; @@ -301,6 +302,13 @@ void gatt_process_exec_write_req(tGATT_TCB& tcb, uint8_t op_code, } #endif + if (len < sizeof(flag)) { + android_errorWriteLog(0x534e4554, "73172115"); + LOG(ERROR) << __func__ << "invalid length"; + gatt_send_error_rsp(tcb, GATT_INVALID_PDU, GATT_REQ_EXEC_WRITE, 0, false); + return; + } + STREAM_TO_UINT8(flag, p); /* mask the flag */ @@ -940,9 +948,19 @@ void gatts_process_write_req(tGATT_TCB& tcb, tGATT_SRV_LIST_ELEM& el, */ static void gatts_process_read_req(tGATT_TCB& tcb, tGATT_SRV_LIST_ELEM& el, uint8_t op_code, uint16_t handle, - UNUSED_ATTR uint16_t len, uint8_t* p_data) { + uint16_t len, uint8_t* p_data) { size_t buf_len = sizeof(BT_HDR) + tcb.payload_size + L2CAP_MIN_OFFSET; uint16_t offset = 0; + + if (op_code == GATT_REQ_READ_BLOB && len < sizeof(uint16_t)) { + /* Error: packet length is too short */ + LOG(ERROR) << __func__ << ": packet length=" << len + << " too short. min=" << sizeof(uint16_t); + android_errorWriteWithInfoLog(0x534e4554, "73172115", -1, NULL, 0); + gatt_send_error_rsp(tcb, GATT_INVALID_PDU, op_code, 0, false); + return; + } + BT_HDR* p_msg = (BT_HDR*)osi_calloc(buf_len); if (op_code == GATT_REQ_READ_BLOB) STREAM_TO_UINT16(offset, p_data); @@ -964,7 +982,7 @@ static void gatts_process_read_req(tGATT_TCB& tcb, tGATT_SRV_LIST_ELEM& el, if (reason != GATT_SUCCESS) { osi_free(p_msg); - /* in theroy BUSY is not possible(should already been checked), protected + /* in theory BUSY is not possible(should already been checked), protected * check */ if (reason != GATT_PENDING && reason != GATT_BUSY) gatt_send_error_rsp(tcb, reason, op_code, handle, false); -- cgit v1.2.3 From 9610a23181869d0fdafd1a80480da5986aeb9399 Mon Sep 17 00:00:00 2001 From: Myles Watson Date: Wed, 21 Mar 2018 16:45:32 -0700 Subject: PAN: Always allocate in bta_pan_data_buf_ind_cback Change I63b857d031c55d3a0754e4101e330843eb422b2a caused a double free. Move the free call to pan_data_buf_ind_cb(). Free the buffer before every return in pan_data_buf_ind_cb. Bug: 74950468 Test: manual tethering test with DUT sharing its connection Change-Id: If4526f3042699581e2cdde79a362eef0f83768eb Merged-In: If4526f3042699581e2cdde79a362eef0f83768eb (cherry picked from commit 98232b084c66368234d19fafe3076bc1c0f1b578) --- bta/pan/bta_pan_act.cc | 34 ++++++++++++++-------------------- stack/bnep/bnep_main.cc | 1 - stack/pan/pan_main.cc | 12 ++++-------- 3 files changed, 18 insertions(+), 29 deletions(-) diff --git a/bta/pan/bta_pan_act.cc b/bta/pan/bta_pan_act.cc index 41e0bf6b4..789cce8e3 100644 --- a/bta/pan/bta_pan_act.cc +++ b/bta/pan/bta_pan_act.cc @@ -171,31 +171,25 @@ static void bta_pan_data_flow_cb(uint16_t handle, tPAN_RESULT result) { static void bta_pan_data_buf_ind_cback(uint16_t handle, const RawAddress& src, const RawAddress& dst, uint16_t protocol, BT_HDR* p_buf, bool ext, bool forward) { - tBTA_PAN_SCB* p_scb; - BT_HDR* p_new_buf; - - p_scb = bta_pan_scb_by_handle(handle); + tBTA_PAN_SCB* p_scb = bta_pan_scb_by_handle(handle); if (p_scb == NULL) { return; } - if (sizeof(tBTA_PAN_DATA_PARAMS) > p_buf->offset) { - /* offset smaller than data structure in front of actual data */ - if (sizeof(BT_HDR) + sizeof(tBTA_PAN_DATA_PARAMS) + p_buf->len > - PAN_BUF_SIZE) { - android_errorWriteLog(0x534e4554, "63146237"); - APPL_TRACE_ERROR("%s: received buffer length too large: %d", __func__, - p_buf->len); - return; - } - p_new_buf = (BT_HDR*)osi_malloc(PAN_BUF_SIZE); - memcpy((uint8_t*)(p_new_buf + 1) + sizeof(tBTA_PAN_DATA_PARAMS), - (uint8_t*)(p_buf + 1) + p_buf->offset, p_buf->len); - p_new_buf->len = p_buf->len; - p_new_buf->offset = sizeof(tBTA_PAN_DATA_PARAMS); - } else { - p_new_buf = p_buf; + if (sizeof(BT_HDR) + sizeof(tBTA_PAN_DATA_PARAMS) + p_buf->len > + PAN_BUF_SIZE) { + android_errorWriteLog(0x534e4554, "63146237"); + APPL_TRACE_ERROR("%s: received buffer length too large: %d", __func__, + p_buf->len); + return; } + + BT_HDR* p_new_buf = (BT_HDR*)osi_malloc(PAN_BUF_SIZE); + memcpy((uint8_t*)(p_new_buf + 1) + sizeof(tBTA_PAN_DATA_PARAMS), + (uint8_t*)(p_buf + 1) + p_buf->offset, p_buf->len); + p_new_buf->len = p_buf->len; + p_new_buf->offset = sizeof(tBTA_PAN_DATA_PARAMS); + /* copy params into the space before the data */ ((tBTA_PAN_DATA_PARAMS*)p_new_buf)->src = src; ((tBTA_PAN_DATA_PARAMS*)p_new_buf)->dst = dst; diff --git a/stack/bnep/bnep_main.cc b/stack/bnep/bnep_main.cc index f621fdb64..463fca33b 100644 --- a/stack/bnep/bnep_main.cc +++ b/stack/bnep/bnep_main.cc @@ -607,7 +607,6 @@ static void bnep_data_ind(uint16_t l2cap_cid, BT_HDR* p_buf) { if (bnep_cb.p_data_buf_cb) { (*bnep_cb.p_data_buf_cb)(p_bcb->handle, *p_src_addr, *p_dst_addr, protocol, p_buf, fw_ext_present); - osi_free(p_buf); } else if (bnep_cb.p_data_ind_cb) { (*bnep_cb.p_data_ind_cb)(p_bcb->handle, *p_src_addr, *p_dst_addr, protocol, p, rem_len, fw_ext_present); diff --git a/stack/pan/pan_main.cc b/stack/pan/pan_main.cc index d7cd27b07..6a554231f 100644 --- a/stack/pan/pan_main.cc +++ b/stack/pan/pan_main.cc @@ -595,12 +595,11 @@ void pan_data_buf_ind_cb(uint16_t handle, const RawAddress& src, if (pan_cb.pan_data_buf_ind_cb) (*pan_cb.pan_data_buf_ind_cb)(pcb->handle, src, dst, protocol, p_buf, ext, forward); - else if (pan_cb.pan_data_ind_cb) { + else if (pan_cb.pan_data_ind_cb) (*pan_cb.pan_data_ind_cb)(pcb->handle, src, dst, protocol, p_data, len, ext, forward); - osi_free(p_buf); - } + osi_free(p_buf); return; } @@ -625,13 +624,10 @@ void pan_data_buf_ind_cb(uint16_t handle, const RawAddress& src, if (pan_cb.pan_data_buf_ind_cb) (*pan_cb.pan_data_buf_ind_cb)(pcb->handle, src, dst, protocol, p_buf, ext, forward); - else if (pan_cb.pan_data_ind_cb) { + else if (pan_cb.pan_data_ind_cb) (*pan_cb.pan_data_ind_cb)(pcb->handle, src, dst, protocol, p_data, len, ext, forward); - osi_free(p_buf); - } else - osi_free(p_buf); - + osi_free(p_buf); return; } -- cgit v1.2.3 From a787467d6e8f50907a188c9ebbad2fccb27fadcf Mon Sep 17 00:00:00 2001 From: Hansong Zhang Date: Thu, 12 Apr 2018 15:50:28 -0700 Subject: DO NOT MERGE Fix OOB read in process_l2cap_cmd Bug: 74202041 Bug: 74196706 Bug: 74201143 Test: manual Change-Id: Ic25f7f3777d0375f76cc91e4d129b1636f1c388d (cherry picked from commit ff15adf5150527db1012b9f7777066522835e2db) --- stack/l2cap/l2c_main.cc | 104 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/stack/l2cap/l2c_main.cc b/stack/l2cap/l2c_main.cc index 83d1737bc..7c1ef4837 100644 --- a/stack/l2cap/l2c_main.cc +++ b/stack/l2cap/l2c_main.cc @@ -320,8 +320,16 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { switch (cmd_code) { case L2CAP_CMD_REJECT: + if (p + 2 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(rej_reason, p); if (rej_reason == L2CAP_CMD_REJ_MTU_EXCEEDED) { + if (p + 2 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(rej_mtu, p); /* What to do with the MTU reject ? We have negotiated an MTU. For now */ @@ -332,6 +340,10 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { p_lcb->handle, rej_mtu); } if (rej_reason == L2CAP_CMD_REJ_INVALID_CID) { + if (p + 4 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(rcid, p); STREAM_TO_UINT16(lcid, p); @@ -365,6 +377,10 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { break; case L2CAP_CMD_CONN_REQ: + if (p + 4 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(con_info.psm, p); STREAM_TO_UINT16(rcid, p); p_rcb = l2cu_find_rcb_by_psm(con_info.psm); @@ -396,6 +412,10 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { break; case L2CAP_CMD_CONN_RSP: + if (p + 8 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(con_info.remote_cid, p); STREAM_TO_UINT16(lcid, p); STREAM_TO_UINT16(con_info.l2cap_result, p); @@ -427,6 +447,10 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { cfg_rej = false; cfg_rej_len = 0; + if (p + 4 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(lcid, p); STREAM_TO_UINT16(cfg_info.flags, p); @@ -437,22 +461,38 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { false; while (p < p_cfg_end) { + if (p + 2 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT8(cfg_code, p); STREAM_TO_UINT8(cfg_len, p); switch (cfg_code & 0x7F) { case L2CAP_CFG_TYPE_MTU: cfg_info.mtu_present = true; + if (p + 2 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(cfg_info.mtu, p); break; case L2CAP_CFG_TYPE_FLUSH_TOUT: cfg_info.flush_to_present = true; + if (p + 2 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(cfg_info.flush_to, p); break; case L2CAP_CFG_TYPE_QOS: cfg_info.qos_present = true; + if (p + 2 + 5 * 4 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT8(cfg_info.qos.qos_flags, p); STREAM_TO_UINT8(cfg_info.qos.service_type, p); STREAM_TO_UINT32(cfg_info.qos.token_rate, p); @@ -464,6 +504,10 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { case L2CAP_CFG_TYPE_FCR: cfg_info.fcr_present = true; + if (p + 3 + 3 * 2 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT8(cfg_info.fcr.mode, p); STREAM_TO_UINT8(cfg_info.fcr.tx_win_sz, p); STREAM_TO_UINT8(cfg_info.fcr.max_transmit, p); @@ -474,11 +518,19 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { case L2CAP_CFG_TYPE_FCS: cfg_info.fcs_present = true; + if (p + 1 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT8(cfg_info.fcs, p); break; case L2CAP_CFG_TYPE_EXT_FLOW: cfg_info.ext_flow_spec_present = true; + if (p + 2 + 2 + 3 * 4 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT8(cfg_info.ext_flow_spec.id, p); STREAM_TO_UINT8(cfg_info.ext_flow_spec.stype, p); STREAM_TO_UINT16(cfg_info.ext_flow_spec.max_sdu_size, p); @@ -523,6 +575,10 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { case L2CAP_CMD_CONFIG_RSP: p_cfg_end = p + cmd_len; + if (p + 6 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(lcid, p); STREAM_TO_UINT16(cfg_info.flags, p); STREAM_TO_UINT16(cfg_info.result, p); @@ -532,22 +588,38 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { false; while (p < p_cfg_end) { + if (p + 2 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT8(cfg_code, p); STREAM_TO_UINT8(cfg_len, p); switch (cfg_code & 0x7F) { case L2CAP_CFG_TYPE_MTU: cfg_info.mtu_present = true; + if (p + 2 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(cfg_info.mtu, p); break; case L2CAP_CFG_TYPE_FLUSH_TOUT: cfg_info.flush_to_present = true; + if (p + 2 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(cfg_info.flush_to, p); break; case L2CAP_CFG_TYPE_QOS: cfg_info.qos_present = true; + if (p + 2 + 5 * 4 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT8(cfg_info.qos.qos_flags, p); STREAM_TO_UINT8(cfg_info.qos.service_type, p); STREAM_TO_UINT32(cfg_info.qos.token_rate, p); @@ -559,6 +631,10 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { case L2CAP_CFG_TYPE_FCR: cfg_info.fcr_present = true; + if (p + 3 + 3 * 2 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT8(cfg_info.fcr.mode, p); STREAM_TO_UINT8(cfg_info.fcr.tx_win_sz, p); STREAM_TO_UINT8(cfg_info.fcr.max_transmit, p); @@ -569,11 +645,19 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { case L2CAP_CFG_TYPE_FCS: cfg_info.fcs_present = true; + if (p + 1 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT8(cfg_info.fcs, p); break; case L2CAP_CFG_TYPE_EXT_FLOW: cfg_info.ext_flow_spec_present = true; + if (p + 2 + 2 + 3 * 4 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT8(cfg_info.ext_flow_spec.id, p); STREAM_TO_UINT8(cfg_info.ext_flow_spec.stype, p); STREAM_TO_UINT16(cfg_info.ext_flow_spec.max_sdu_size, p); @@ -603,6 +687,10 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { break; case L2CAP_CMD_DISC_REQ: + if (p + 4 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(lcid, p); STREAM_TO_UINT16(rcid, p); @@ -618,6 +706,10 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { break; case L2CAP_CMD_DISC_RSP: + if (p + 4 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(rcid, p); STREAM_TO_UINT16(lcid, p); @@ -645,6 +737,10 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { break; case L2CAP_CMD_INFO_REQ: + if (p + 2 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(info_type, p); l2cu_send_peer_info_rsp(p_lcb, id, info_type); break; @@ -656,6 +752,10 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { p_lcb->w4_info_rsp = false; } + if (p + 4 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT16(info_type, p); STREAM_TO_UINT16(result, p); @@ -663,6 +763,10 @@ static void process_l2cap_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { if ((info_type == L2CAP_EXTENDED_FEATURES_INFO_TYPE) && (result == L2CAP_INFO_RESP_RESULT_SUCCESS)) { + if (p + 4 > p_next_cmd) { + android_errorWriteLog(0x534e4554, "74202041"); + return; + } STREAM_TO_UINT32(p_lcb->peer_ext_fea, p); #if (L2CAP_NUM_FIXED_CHNLS > 0) -- cgit v1.2.3 From 6bee1946995d5c550a497dd6436a5114511e91f2 Mon Sep 17 00:00:00 2001 From: Hansong Zhang Date: Thu, 12 Apr 2018 11:58:49 -0700 Subject: DO NOT MERGE Initialize local variable in gatts_process_read_by_type_req Bug: 73125709 Test: manual Change-Id: I8b3346f605e0820385ea5ed7401bbee664fd15aa (cherry picked from commit 0e34139d7fa338df6c99aaba13eb839a3dbc2548) --- stack/gatt/gatt_sr.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/stack/gatt/gatt_sr.cc b/stack/gatt/gatt_sr.cc index 06c9c5205..f9e8f537f 100644 --- a/stack/gatt/gatt_sr.cc +++ b/stack/gatt/gatt_sr.cc @@ -788,7 +788,8 @@ static void gatts_process_mtu_req(tGATT_TCB& tcb, uint16_t len, void gatts_process_read_by_type_req(tGATT_TCB& tcb, uint8_t op_code, uint16_t len, uint8_t* p_data) { tBT_UUID uuid; - uint16_t s_hdl, e_hdl, err_hdl = 0; + uint16_t s_hdl = 0, e_hdl = 0, err_hdl = 0; + if (len < 4) android_errorWriteLog(0x534e4554, "73125709"); tGATT_STATUS reason = gatts_validate_packet_format(op_code, len, p_data, &uuid, s_hdl, e_hdl); -- cgit v1.2.3 From b2ef3171a3fbdded006b8e4b6cbe8a985040d926 Mon Sep 17 00:00:00 2001 From: Hansong Zhang Date: Wed, 11 Apr 2018 16:04:51 -0700 Subject: DO NOT MERGE Add bounds check for BNEP_Write Bug: 74947856 Test: manual Change-Id: If5db8c6b6e509a330ae74808fc3f0ffac137af14 (cherry picked from commit ae9d06c1dc84db36c0c4a07fc56a1fbf008cd1ce) --- stack/bnep/bnep_api.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/stack/bnep/bnep_api.cc b/stack/bnep/bnep_api.cc index 923ce5093..e5d3c0948 100644 --- a/stack/bnep/bnep_api.cc +++ b/stack/bnep/bnep_api.cc @@ -23,6 +23,7 @@ ******************************************************************************/ #include "bnep_api.h" +#include #include #include "bnep_int.h" @@ -383,6 +384,10 @@ tBNEP_RESULT BNEP_WriteBuf(uint16_t handle, const RawAddress& p_dest_addr, protocol = 0; else { new_len += 4; + if (new_len > org_len) { + android_errorWriteLog(0x534e4554, "74947856"); + return BNEP_IGNORE_CMD; + } p_data[2] = 0; p_data[3] = 0; } @@ -479,6 +484,10 @@ tBNEP_RESULT BNEP_Write(uint16_t handle, const RawAddress& p_dest_addr, protocol = 0; else { new_len += 4; + if (new_len > org_len) { + android_errorWriteLog(0x534e4554, "74947856"); + return BNEP_IGNORE_CMD; + } p_data[2] = 0; p_data[3] = 0; } -- cgit v1.2.3 From c7b1b3430baa3344d7b8187ce1af6f961f3619fb Mon Sep 17 00:00:00 2001 From: Andre Eisenbach Date: Thu, 1 Mar 2018 13:27:01 -0800 Subject: DO NOT MERGE SMP: Validate remote elliptic curve points Fixes: 72377774 Test: net_test_stack_smp (where applicable) Change-Id: Iefcf97364493467075fadefd77d12716f71cd4f6 (cherry picked from commit 9181ec28da94705a763edbe60bd2a87e5f882beb) (cherry picked from commit e11ebfc21963ae905d58c034310efeca0e7cd2ee) --- stack/smp/p_256_ecc_pp.cc | 22 ++++++++++++++++++++++ stack/smp/p_256_ecc_pp.h | 3 +++ stack/smp/smp_act.cc | 12 ++++++++++++ 3 files changed, 37 insertions(+) diff --git a/stack/smp/p_256_ecc_pp.cc b/stack/smp/p_256_ecc_pp.cc index b416e1d3f..911dc5498 100644 --- a/stack/smp/p_256_ecc_pp.cc +++ b/stack/smp/p_256_ecc_pp.cc @@ -245,3 +245,25 @@ void ECC_PointMult_Bin_NAF(Point* q, Point* p, uint32_t* n, multiprecision_mersenns_mult_mod(q->z, q->z, minus_p.x, keyLength); multiprecision_mersenns_mult_mod(q->y, q->y, q->z, keyLength); } + +bool ECC_ValidatePoint(const Point& pt) { + const size_t kl = KEY_LENGTH_DWORDS_P256; + p_256_init_curve(kl); + + // Ensure y^2 = x^3 + a*x + b (mod p); a = -3 + + // y^2 mod p + uint32_t y2_mod[kl] = {0}; + multiprecision_mersenns_squa_mod(y2_mod, (uint32_t*)pt.y, kl); + + // Right hand side calculation + uint32_t rhs[kl] = {0}; + multiprecision_mersenns_squa_mod(rhs, (uint32_t*)pt.x, kl); + uint32_t three[kl] = {0}; + three[0] = 3; + multiprecision_sub_mod(rhs, rhs, three, kl); + multiprecision_mersenns_mult_mod(rhs, rhs, (uint32_t*)pt.x, kl); + multiprecision_add_mod(rhs, rhs, curve_p256.b, kl); + + return multiprecision_compare(rhs, y2_mod, kl) == 0; +} diff --git a/stack/smp/p_256_ecc_pp.h b/stack/smp/p_256_ecc_pp.h index dcc4211dc..b7a8e0066 100644 --- a/stack/smp/p_256_ecc_pp.h +++ b/stack/smp/p_256_ecc_pp.h @@ -25,6 +25,7 @@ #pragma once +#include #include "p_256_multprecision.h" typedef struct { @@ -55,6 +56,8 @@ typedef struct { extern elliptic_curve_t curve; extern elliptic_curve_t curve_p256; +bool ECC_ValidatePoint(const Point& p); + void ECC_PointMult_Bin_NAF(Point* q, Point* p, uint32_t* n, uint32_t keyLength); #define ECC_PointMult(q, p, n, keyLength) \ diff --git a/stack/smp/smp_act.cc b/stack/smp/smp_act.cc index df9fab9f1..2103776df 100644 --- a/stack/smp/smp_act.cc +++ b/stack/smp/smp_act.cc @@ -22,6 +22,7 @@ #include "include/bt_target.h" #include "stack/btm/btm_int.h" #include "stack/include/l2c_api.h" +#include "stack/smp/p_256_ecc_pp.h" #include "stack/smp/smp_int.h" #include "utils/include/bt_utils.h" @@ -655,6 +656,17 @@ void smp_process_pairing_public_key(tSMP_CB* p_cb, tSMP_INT_DATA* p_data) { STREAM_TO_ARRAY(p_cb->peer_publ_key.x, p, BT_OCTET32_LEN); STREAM_TO_ARRAY(p_cb->peer_publ_key.y, p, BT_OCTET32_LEN); + + Point pt; + memcpy(pt.x, p_cb->peer_publ_key.x, BT_OCTET32_LEN); + memcpy(pt.y, p_cb->peer_publ_key.y, BT_OCTET32_LEN); + + if (!ECC_ValidatePoint(pt)) { + android_errorWriteLog(0x534e4554, "72377774"); + smp_sm_event(p_cb, SMP_AUTH_CMPL_EVT, &reason); + return; + } + p_cb->flags |= SMP_PAIR_FLAG_HAVE_PEER_PUBL_KEY; smp_wait_for_both_public_keys(p_cb, NULL); -- cgit v1.2.3 From 03a0b87f9339bc41aac8a945a6d04a779040c679 Mon Sep 17 00:00:00 2001 From: Hansong Zhang Date: Thu, 26 Apr 2018 15:50:53 -0700 Subject: DO NOT MERGE Prevent stack overflow in btif_storage Bug: 73963551 Test: manual Change-Id: I5f7a583aad150ebf9e3d492181d80ca935c8aa3f (cherry picked from commit e8d311224277e9db5dc94cb94929125992f546f3) --- btif/src/btif_storage.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/btif/src/btif_storage.cc b/btif/src/btif_storage.cc index 9d4a84ed0..1c34787b2 100644 --- a/btif/src/btif_storage.cc +++ b/btif/src/btif_storage.cc @@ -235,6 +235,10 @@ static int prop2cfg(const RawAddress* remote_bd_addr, bt_property_t* prop) { bt_uuid_t* p_uuid = (bt_uuid_t*)prop->val + i; memset(buf, 0, sizeof(buf)); uuid_to_string_legacy(p_uuid, buf, sizeof(buf)); + if (strlen(value) + strlen(buf) + 1 > (int) sizeof(value) - 1) { + android_errorWriteLog(0x534e4554, "73963551"); + return false; + } strcat(value, buf); // strcat(value, ";"); strcat(value, " "); -- cgit v1.2.3 From accfb612a205692d2d919ab44d9bd9f6fea391a2 Mon Sep 17 00:00:00 2001 From: Hansong Zhang Date: Fri, 11 May 2018 11:36:29 -0700 Subject: DO NOT MERGE AVRC: Add bound check for AVRC_EVT_APP_SETTING_CHANGE Test: manual Bug: 73782082 Change-Id: I4e384a2f8c0d8c4af03bd5865b2e907321419c86 (cherry picked from commit 0061dd6ae30ebcebce695c212c8bc0ceb276710e) --- stack/avrc/avrc_pars_ct.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/stack/avrc/avrc_pars_ct.cc b/stack/avrc/avrc_pars_ct.cc index 334362449..fbfeeaf9f 100644 --- a/stack/avrc/avrc_pars_ct.cc +++ b/stack/avrc/avrc_pars_ct.cc @@ -119,6 +119,10 @@ void avrc_parse_notification_rsp(uint8_t* p_stream, case AVRC_EVT_APP_SETTING_CHANGE: BE_STREAM_TO_UINT8(p_rsp->param.player_setting.num_attr, p_stream); + if (p_rsp->param.player_setting.num_attr > AVRC_MAX_APP_SETTINGS) { + android_errorWriteLog(0x534e4554, "73782082"); + p_rsp->param.player_setting.num_attr = AVRC_MAX_APP_SETTINGS; + } for (int index = 0; index < p_rsp->param.player_setting.num_attr; index++) { BE_STREAM_TO_UINT8(p_rsp->param.player_setting.attr_id[index], -- cgit v1.2.3 From 884185790921cc8ada50d176e91952529e747ab8 Mon Sep 17 00:00:00 2001 From: Ajay Panicker Date: Fri, 11 May 2018 12:03:07 -0700 Subject: DO NOT MERGE: Check number of attributes before writing to a buffer Bug: 73824150 Test: Compile Change-Id: I2a28a503cd74758e707d1e591b55c278d2299f45 (cherry picked from commit f6db54f071f6974e18b10bb0c2cfcf397cd4c980) --- btif/src/btif_rc.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/btif/src/btif_rc.cc b/btif/src/btif_rc.cc index fc10b8b61..edd073062 100644 --- a/btif/src/btif_rc.cc +++ b/btif/src/btif_rc.cc @@ -45,6 +45,7 @@ #include "btif_util.h" #include "btu.h" #include "device/include/interop.h" +#include "log/log.h" #include "osi/include/list.h" #include "osi/include/osi.h" #include "osi/include/properties.h" @@ -3502,6 +3503,12 @@ static void handle_app_cur_val_response(tBTA_AV_META_MSG* pmeta_msg, RawAddress rc_addr = p_dev->rc_addr; app_settings.num_attr = p_rsp->num_val; + + if (app_settings.num_attr > BTRC_MAX_APP_SETTINGS) { + android_errorWriteLog(0x534e4554, "73824150"); + app_settings.num_attr = BTRC_MAX_APP_SETTINGS; + } + for (xx = 0; xx < app_settings.num_attr; xx++) { app_settings.attr_ids[xx] = p_rsp->p_vals[xx].attr_id; app_settings.attr_values[xx] = p_rsp->p_vals[xx].attr_val; -- cgit v1.2.3 From ef079db7a88afa2c2867dbdd8d0df45ec10425b7 Mon Sep 17 00:00:00 2001 From: Ajay Panicker Date: Thu, 12 Apr 2018 17:03:09 -0700 Subject: Add bounds check to l2cble_process_sig_cmd L2CAP_CMD_DISC_REQ Bug: 74121659 Test: Compiles Change-Id: Idf58e7b25b41ae1bd43cdd51de424b18e03cc7e8 (cherry picked from commit ca4f8a18bce9331360144f1dbc51db1e2525bcc3) --- stack/l2cap/l2c_ble.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/stack/l2cap/l2c_ble.cc b/stack/l2cap/l2c_ble.cc index 6c7820f66..17ce2d305 100644 --- a/stack/l2cap/l2c_ble.cc +++ b/stack/l2cap/l2c_ble.cc @@ -33,6 +33,7 @@ #include "hcimsgs.h" #include "l2c_int.h" #include "l2cdefs.h" +#include "log/log.h" #include "osi/include/osi.h" #include "stack_config.h" @@ -788,6 +789,10 @@ void l2cble_process_sig_cmd(tL2C_LCB* p_lcb, uint8_t* p, uint16_t pkt_len) { break; case L2CAP_CMD_DISC_REQ: + if (p + 4 > p_pkt_end) { + android_errorWriteLog(0x534e4554, "74121659"); + return; + } STREAM_TO_UINT16(lcid, p); STREAM_TO_UINT16(rcid, p); -- cgit v1.2.3 From bba59ce8b36de32f2666284e9d9af5e8f93df8e1 Mon Sep 17 00:00:00 2001 From: Jakub Pawlowski Date: Thu, 24 May 2018 08:59:34 -0700 Subject: Add PDU size checks in process_service_search_attr_rsp Bug: 79884292 Change-Id: Icc02a6188f806f766aa8676804d74995afa08d25 Merged-In: Icc02a6188f806f766aa8676804d74995afa08d25 (cherry picked from commit 980f6427b183e013958acd6b70e91f58177408a6) --- stack/sdp/sdp_discovery.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/stack/sdp/sdp_discovery.cc b/stack/sdp/sdp_discovery.cc index 7eea5fda3..b25915772 100644 --- a/stack/sdp/sdp_discovery.cc +++ b/stack/sdp/sdp_discovery.cc @@ -531,6 +531,13 @@ static void process_service_search_attr_rsp(tCONN_CB* p_ccb, uint8_t* p_reply, #endif /* If p_reply is NULL, we were called for the initial read */ if (p_reply) { + if (p_reply + 4 /* transaction ID and length */ + sizeof(lists_byte_count) > + p_reply_end) { + android_errorWriteLog(0x534e4554, "79884292"); + sdp_disconnect(p_ccb, SDP_INVALID_PDU_SIZE); + return; + } + #if (SDP_DEBUG_RAW == TRUE) SDP_TRACE_WARNING("ID & len: 0x%02x-%02x-%02x-%02x", p_reply[0], p_reply[1], p_reply[2], p_reply[3]); @@ -554,6 +561,13 @@ static void process_service_search_attr_rsp(tCONN_CB* p_ccb, uint8_t* p_reply, SDP_TRACE_WARNING("list_len: %d, list_byte_count: %d", p_ccb->list_len, lists_byte_count); #endif + + if (p_reply + lists_byte_count + 1 /* continuation */ > p_reply_end) { + android_errorWriteLog(0x534e4554, "79884292"); + sdp_disconnect(p_ccb, SDP_INVALID_PDU_SIZE); + return; + } + if (p_ccb->rsp_list == NULL) p_ccb->rsp_list = (uint8_t*)osi_malloc(SDP_MAX_LIST_BYTE_COUNT); memcpy(&p_ccb->rsp_list[p_ccb->list_len], p_reply, lists_byte_count); -- cgit v1.2.3 From 3efa01ccdd7e1cc707c0fa3fd98723c0975ba8f4 Mon Sep 17 00:00:00 2001 From: Jakub Pawlowski Date: Wed, 23 May 2018 10:19:53 -0700 Subject: GATT: Handle too short Error Response PDU Since the spec is not clear what to do in this case, use one of reserved error codes as a failure reason, and pass it to upper layers. Bug: 79591688 Change-Id: Ie6a53e9c8e4ceb8f1e5a75aee44baa5f4a798c4f Merged-In: Ie6a53e9c8e4ceb8f1e5a75aee44baa5f4a798c4f (cherry picked from commit f63c4b652b3231c2b4907bffd13410c6eb2aa760) --- stack/gatt/gatt_cl.cc | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/stack/gatt/gatt_cl.cc b/stack/gatt/gatt_cl.cc index 2843ac938..9e77e1587 100644 --- a/stack/gatt/gatt_cl.cc +++ b/stack/gatt/gatt_cl.cc @@ -29,6 +29,7 @@ #include "bt_utils.h" #include "gatt_int.h" #include "l2c_int.h" +#include "log/log.h" #include "osi/include/osi.h" #define GATT_WRITE_LONG_HDR_SIZE 5 /* 1 opcode + 2 handle + 2 offset */ @@ -507,9 +508,24 @@ void gatt_process_error_rsp(tGATT_TCB& tcb, tGATT_CLCB* p_clcb, tGATT_VALUE* p_attr = (tGATT_VALUE*)p_clcb->p_attr_buf; VLOG(1) << __func__; - STREAM_TO_UINT8(opcode, p); - STREAM_TO_UINT16(handle, p); - STREAM_TO_UINT8(reason, p); + + if (len < 4) { + android_errorWriteLog(0x534e4554, "79591688"); + LOG(ERROR) << "Error response too short"; + // Specification does not clearly define what should happen if error + // response is too short. General rule in BT Spec 5.0 Vol 3, Part F 3.4.1.1 + // is: "If an error code is received in the Error Response that is not + // understood by the client, for example an error code that was reserved for + // future use that is now being used in a future version of this + // specification, then the Error Response shall still be considered to state + // that the given request cannot be performed for an unknown reason." + opcode = handle = 0; + reason = 0x7F; + } else { + STREAM_TO_UINT8(opcode, p); + STREAM_TO_UINT16(handle, p); + STREAM_TO_UINT8(reason, p); + } if (p_clcb->operation == GATTC_OPTYPE_DISCOVERY) { gatt_proc_disc_error_rsp(tcb, p_clcb, opcode, handle, reason); -- cgit v1.2.3 From 2f300925edff9666b562b54c7174d4f6306005b5 Mon Sep 17 00:00:00 2001 From: Hansong Zhang Date: Tue, 29 May 2018 17:38:39 -0700 Subject: DO NOT MERGE SMP: Check p_cb->role in smp_br_state_machine_event Bug: 80145946 Test: manual Change-Id: Ic83eaa4be868d5a345d80cd50a6915c0af719a53 (cherry picked from commit 519b61392a96fbd45bdcc0bfddc881167c20cc23) --- stack/smp/smp_br_main.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/stack/smp/smp_br_main.cc b/stack/smp/smp_br_main.cc index 260b9c4fd..55405ccfd 100644 --- a/stack/smp/smp_br_main.cc +++ b/stack/smp/smp_br_main.cc @@ -19,6 +19,7 @@ #include "bt_target.h" #include +#include "log/log.h" #include "smp_int.h" const char* const smp_br_state_name[SMP_BR_STATE_MAX + 1] = { @@ -308,6 +309,12 @@ void smp_br_state_machine_event(tSMP_CB* p_cb, tSMP_BR_EVENT event, return; } + if (p_cb->role > HCI_ROLE_SLAVE) { + SMP_TRACE_ERROR("%s: invalid role %d", __func__, p_cb->role); + android_errorWriteLog(0x534e4554, "80145946"); + return; + } + SMP_TRACE_DEBUG("SMP Role: %s State: [%s (%d)], Event: [%s (%d)]", (p_cb->role == HCI_ROLE_SLAVE) ? "Slave" : "Master", smp_get_br_state_name(p_cb->br_state), p_cb->br_state, -- cgit v1.2.3 From c9ce1d4cbca05ea1ed82e836cff6fc46f2be27c6 Mon Sep 17 00:00:00 2001 From: Jakub Pawlowski Date: Tue, 29 May 2018 16:17:32 -0700 Subject: Decrease length after reading from array in process_service_attr_req Test: compilation Bug: 78136677 Change-Id: I4807a350e2b4764a93f104ce88f23a957a7e85c0 (cherry picked from commit 6cd2e8bf6e5707e8e77e7aca6519c58200ee58db) --- stack/sdp/sdp_server.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/stack/sdp/sdp_server.cc b/stack/sdp/sdp_server.cc index e4ed60a92..121c248ad 100644 --- a/stack/sdp/sdp_server.cc +++ b/stack/sdp/sdp_server.cc @@ -333,9 +333,11 @@ static void process_service_attr_req(tCONN_CB* p_ccb, uint16_t trans_num, /* Extract the record handle */ BE_STREAM_TO_UINT32(rec_handle, p_req); + param_len -= sizeof(rec_handle); /* Get the max list length we can send. Cap it at MTU size minus overhead */ BE_STREAM_TO_UINT16(max_list_len, p_req); + param_len -= sizeof(max_list_len); if (max_list_len > (p_ccb->rem_mtu_size - SDP_MAX_ATTR_RSPHDR_LEN)) max_list_len = p_ccb->rem_mtu_size - SDP_MAX_ATTR_RSPHDR_LEN; -- cgit v1.2.3 From 279341be6c430bd671a164273bbb33703472a46a Mon Sep 17 00:00:00 2001 From: Myles Watson Date: Tue, 29 May 2018 16:55:58 -0700 Subject: DO NOT MERGE: SDP: Recalculate param_len after max_list_len Bug: 78136869 Test: manual connection to an A2DP device Change-Id: I71392cf1a70567fec957feb36768069ac5258aa1 (cherry picked from commit 9cc9eea21c7868034242b7ab8be750c565e46bfd) --- stack/sdp/sdp_server.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/stack/sdp/sdp_server.cc b/stack/sdp/sdp_server.cc index 121c248ad..d2bbd6c4d 100644 --- a/stack/sdp/sdp_server.cc +++ b/stack/sdp/sdp_server.cc @@ -569,6 +569,7 @@ static void process_service_search_attr_req(tCONN_CB* p_ccb, uint16_t trans_num, if (max_list_len > (p_ccb->rem_mtu_size - SDP_MAX_SERVATTR_RSPHDR_LEN)) max_list_len = p_ccb->rem_mtu_size - SDP_MAX_SERVATTR_RSPHDR_LEN; + param_len = static_cast(p_req_end - p_req); p_req = sdpu_extract_attr_seq(p_req, param_len, &attr_seq); if ((!p_req) || (!attr_seq.num_attr) || -- cgit v1.2.3 From ea7f83d9b05772b1b07a762a4bffa79414487ae9 Mon Sep 17 00:00:00 2001 From: akirilov Date: Mon, 21 May 2018 12:56:17 -0700 Subject: RESTRICT AUTOMERGE: Fixes two bluetooth bugs causing remote overreads (1/2) Bug: 74075873 Test: manual test (poc in bug) Change-Id: I56e87cfdf8731acca00cefac98abb2ba06f6e7ed (cherry picked from commit 3575ba8ca36dccf7dcdb2dbf16ed170d549911d3) --- 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 b25915772..f5dab6e1a 100644 --- a/stack/sdp/sdp_discovery.cc +++ b/stack/sdp/sdp_discovery.cc @@ -357,7 +357,7 @@ static void sdp_copy_raw_data(tCONN_CB* p_ccb, bool offset) { type = *p++; p = sdpu_get_len_from_type(p, type, &list_len); } - if (list_len && list_len < cpy_len) { + if (list_len < cpy_len) { cpy_len = list_len; } SDP_TRACE_WARNING( -- cgit v1.2.3 From 927155a6fe8fb52b4375b483307e7c93857d444b Mon Sep 17 00:00:00 2001 From: akirilov Date: Mon, 21 May 2018 11:45:55 -0700 Subject: RESTRICT AUTOMERGE: Fixes two bluetooth causing remote overreads (2/2) Bug: 74075873 Test: manual Change-Id: I9a7035a74aca3256c5712ea67a7435627b139c37 (cherry picked from commit 9d647b201b64949e04eade9b594af76c764dbb96) --- stack/sdp/sdp_discovery.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/stack/sdp/sdp_discovery.cc b/stack/sdp/sdp_discovery.cc index f5dab6e1a..bf6f43be7 100644 --- a/stack/sdp/sdp_discovery.cc +++ b/stack/sdp/sdp_discovery.cc @@ -332,7 +332,7 @@ static void process_service_search_rsp(tCONN_CB* p_ccb, uint8_t* p_reply, ******************************************************************************/ #if (SDP_RAW_DATA_INCLUDED == TRUE) static void sdp_copy_raw_data(tCONN_CB* p_ccb, bool offset) { - unsigned int cpy_len; + unsigned int cpy_len, rem_len; uint32_t list_len; uint8_t* p; uint8_t type; @@ -360,6 +360,11 @@ static void sdp_copy_raw_data(tCONN_CB* p_ccb, bool offset) { if (list_len < cpy_len) { cpy_len = list_len; } + rem_len = SDP_MAX_LIST_BYTE_COUNT - (unsigned int)(p - &p_ccb->rsp_list[0]); + if (cpy_len > rem_len) { + SDP_TRACE_WARNING("rem_len :%d less than cpy_len:%d", rem_len, cpy_len); + cpy_len = rem_len; + } SDP_TRACE_WARNING( "%s: list_len:%d cpy_len:%d p:%p p_ccb:%p p_db:%p raw_size:%d " "raw_used:%d raw_data:%p", -- cgit v1.2.3 From 036ec33c8b20319df67b66735d4b67151d970fc9 Mon Sep 17 00:00:00 2001 From: Jack He Date: Fri, 1 Jun 2018 14:00:42 -0700 Subject: BNEP: Fix OOB access in bnep_data_ind * Stop reading the L2CAP packet if packet length is 0 * Process the buffer for BNEP_EXTENSION_CONTROL packet before advancing the buffer pointer by length of payload * Reject BNEP_EXTENSION_CONTROL packet when the payload size is zero * Move error logging to more appropriate locations at where the OOB access is most likely triggered Bug: 78286118 Bug: 79164722 Test: Send zero length L2CAP packet to BNEP, send invalid BNEP_EXTENSION_CONTROL packet Merged-In: I7e18632b8faab1b6aaca1bff1b7f55d69962729e Change-Id: I7e18632b8faab1b6aaca1bff1b7f55d69962729e (cherry picked from commit 3c799a6e25abdf6bacb660ff7a06338836cc7356) (cherry picked from commit 0416340ffa61337dbaa2f6602ef85a1c32563ec2) --- stack/bnep/bnep_main.cc | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/stack/bnep/bnep_main.cc b/stack/bnep/bnep_main.cc index 463fca33b..d60fb55fa 100644 --- a/stack/bnep/bnep_main.cc +++ b/stack/bnep/bnep_main.cc @@ -431,6 +431,11 @@ static void bnep_data_ind(uint16_t l2cap_cid, BT_HDR* p_buf) { tBNEP_CONN* p_bcb; uint8_t* p = (uint8_t*)(p_buf + 1) + p_buf->offset; uint16_t rem_len = p_buf->len; + if (rem_len == 0) { + android_errorWriteLog(0x534e4554, "78286118"); + osi_free(p_buf); + return; + } uint8_t type, ctrl_type, ext_type = 0; bool extension_present, fw_ext_present; uint16_t protocol = 0; @@ -479,24 +484,35 @@ static void bnep_data_ind(uint16_t l2cap_cid, BT_HDR* p_buf) { uint16_t org_len, new_len; /* parse the extension headers and process unknown control headers */ org_len = rem_len; - new_len = 0; do { - if (org_len < 2) break; + if (org_len < 2) { + android_errorWriteLog(0x534e4554, "67863755"); + break; + } ext = *p++; length = *p++; - p += length; new_len = (length + 2); - if (new_len > org_len) break; + if (new_len > org_len) { + android_errorWriteLog(0x534e4554, "67863755"); + break; + } - if ((!(ext & 0x7F)) && (*p > BNEP_FILTER_MULTI_ADDR_RESPONSE_MSG)) - bnep_send_command_not_understood(p_bcb, *p); + if ((ext & 0x7F) == BNEP_EXTENSION_FILTER_CONTROL) { + if (length == 0) { + android_errorWriteLog(0x534e4554, "79164722"); + break; + } + if (*p > BNEP_FILTER_MULTI_ADDR_RESPONSE_MSG) { + bnep_send_command_not_understood(p_bcb, *p); + } + } + + p += length; org_len -= new_len; } while (ext & 0x80); - android_errorWriteLog(0x534e4554, "67863755"); } - osi_free(p_buf); return; } @@ -540,13 +556,13 @@ static void bnep_data_ind(uint16_t l2cap_cid, BT_HDR* p_buf) { while (extension_present && p && rem_len) { ext_type = *p++; rem_len--; - android_errorWriteLog(0x534e4554, "69271284"); extension_present = ext_type >> 7; ext_type &= 0x7F; /* if unknown extension present stop processing */ - if (ext_type) break; + if (ext_type != BNEP_EXTENSION_FILTER_CONTROL) break; + android_errorWriteLog(0x534e4554, "69271284"); p = bnep_process_control_packet(p_bcb, p, &rem_len, true); } } -- cgit v1.2.3 From 83326685d0de32bfa9f9b492fbb87c6b8f1b8395 Mon Sep 17 00:00:00 2001 From: Pavlin Radoslavov Date: Wed, 30 May 2018 17:56:14 -0700 Subject: Add checks whether the AVDTP element data length is valid Bug: 78288378 Test: Manual: Python script and extra logging Change-Id: I715b5977c833d33ff798f008fbf244effa13ea1f Merged-In: I715b5977c833d33ff798f008fbf244effa13ea1f (cherry picked from commit 9b3f96f50287d8789aff6d6895d7ae02ca6ac619) (cherry picked from commit ee30c88a8d49b30860d35b34a57c3037a4045678) --- stack/avdt/avdt_msg.cc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/stack/avdt/avdt_msg.cc b/stack/avdt/avdt_msg.cc index 7ab7edc00..52ce2e8a6 100644 --- a/stack/avdt/avdt_msg.cc +++ b/stack/avdt/avdt_msg.cc @@ -26,6 +26,7 @@ * ******************************************************************************/ +#include #include #include "avdt_api.h" #include "avdt_int.h" @@ -602,6 +603,11 @@ static uint8_t avdt_msg_prs_cfg(tAVDT_CFG* p_cfg, uint8_t* p, uint16_t len, case AVDT_CAT_PROTECT: p_cfg->psc_mask &= ~AVDT_PSC_PROTECT; + if (p + elem_len > p_end) { + err = AVDT_ERR_LENGTH; + android_errorWriteLog(0x534e4554, "78288378"); + break; + } if ((elem_len + protect_offset) < AVDT_PROTECT_SIZE) { p_cfg->num_protect++; p_cfg->protect_info[protect_offset] = elem_len; @@ -622,6 +628,11 @@ static uint8_t avdt_msg_prs_cfg(tAVDT_CFG* p_cfg, uint8_t* p, uint16_t len, if (elem_len >= AVDT_CODEC_SIZE) { tmp = AVDT_CODEC_SIZE - 1; } + if (p + tmp > p_end) { + err = AVDT_ERR_LENGTH; + android_errorWriteLog(0x534e4554, "78288378"); + break; + } p_cfg->num_codec++; p_cfg->codec_info[0] = elem_len; memcpy(&p_cfg->codec_info[1], p, tmp); -- cgit v1.2.3 From 863938504f4f687c8ee4c334b0a10dbc149ed739 Mon Sep 17 00:00:00 2001 From: Pavlin Radoslavov Date: Wed, 30 May 2018 19:26:16 -0700 Subject: Add packet length check for received AVCTP packets Bug: 79944113 Test: Manual: Custom test program and extra logging Change-Id: Icde465fed723bf876ce3885d11099fddcb92de81 Merged-In: Icde465fed723bf876ce3885d11099fddcb92de81 (cherry picked from commit 2a934acf498a6b715cc7c634123aa403a70fe9e6) (cherry picked from commit d6fb21d8d8ae20addfc51246d840151fc86d8572) --- stack/avct/avct_bcb_act.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/stack/avct/avct_bcb_act.cc b/stack/avct/avct_bcb_act.cc index bd99562ca..70d8ce743 100644 --- a/stack/avct/avct_bcb_act.cc +++ b/stack/avct/avct_bcb_act.cc @@ -25,6 +25,7 @@ * *****************************************************************************/ +#include #include #include "avct_api.h" #include "avct_int.h" @@ -520,6 +521,14 @@ void avct_bcb_msg_ind(tAVCT_BCB* p_bcb, tAVCT_LCB_EVT* p_data) { return; } + if (p_data->p_buf->len < AVCT_HDR_LEN_SINGLE) { + AVCT_TRACE_WARNING("Invalid AVCTP packet length %d: must be at least %d", + p_data->p_buf->len, AVCT_HDR_LEN_SINGLE); + osi_free_and_reset((void**)&p_data->p_buf); + android_errorWriteLog(0x534e4554, "79944113"); + return; + } + p = (uint8_t*)(p_data->p_buf + 1) + p_data->p_buf->offset; /* parse header byte */ -- cgit v1.2.3 From ca25313d9ee4ba9a96f5244de1626911a7d63d32 Mon Sep 17 00:00:00 2001 From: Pavlin Radoslavov Date: Thu, 31 May 2018 11:04:54 -0700 Subject: Add BT_HDR length check for received AVCTP packets Bug: 79944113 Test: Code compilation Change-Id: I02c76ab8fad61669394062bf34656ea32f465b6a Merged-In: I02c76ab8fad61669394062bf34656ea32f465b6a (cherry picked from commit 4262b932e487b19d578d79e0120cf03291f44efc) (cherry picked from commit fa538540a7f147b8440ac49735a8dc596ce8dfc7) --- stack/avct/avct_bcb_act.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/stack/avct/avct_bcb_act.cc b/stack/avct/avct_bcb_act.cc index 70d8ce743..011a52db7 100644 --- a/stack/avct/avct_bcb_act.cc +++ b/stack/avct/avct_bcb_act.cc @@ -69,6 +69,12 @@ static BT_HDR* avct_bcb_msg_asmbl(UNUSED_ATTR tAVCT_BCB* p_bcb, BT_HDR* p_buf) { uint8_t* p; uint8_t pkt_type; + if (p_buf->len == 0) { + osi_free_and_reset((void**)&p_buf); + android_errorWriteLog(0x534e4554, "79944113"); + return nullptr; + } + /* parse the message header */ p = (uint8_t*)(p_buf + 1) + p_buf->offset; pkt_type = AVCT_PKT_TYPE(p); -- cgit v1.2.3 From 62a2cdf7b9f9d0a3635333bb6e2d65f102bdc482 Mon Sep 17 00:00:00 2001 From: Ajay Panicker Date: Tue, 5 Jun 2018 16:08:06 -0700 Subject: DO NOT MERGE: Don't reuse buffer when building response Bug: 79541338 Test: Compile and connect to remote headset Change-Id: I2d808f941d3c71fcb6306c733717624be10478e0 (cherry picked from commit 9bbce8603846159dec0d506ba867b7616557a303) --- stack/avrc/avrc_api.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/stack/avrc/avrc_api.cc b/stack/avrc/avrc_api.cc index b67a6b764..bdf78b736 100644 --- a/stack/avrc/avrc_api.cc +++ b/stack/avrc/avrc_api.cc @@ -425,15 +425,15 @@ static BT_HDR* avrc_proc_vendor_command(uint8_t handle, uint8_t label, } if (status != AVRC_STS_NO_ERROR) { - /* use the current GKI buffer to build/send the reject message */ - p_data = (uint8_t*)(p_pkt + 1) + p_pkt->offset; + p_rsp = (BT_HDR*)osi_malloc(BT_DEFAULT_BUFFER_SIZE); + p_rsp->offset = p_pkt->offset; + p_data = (uint8_t*)(p_rsp + 1) + p_pkt->offset; *p_data++ = AVRC_RSP_REJ; p_data += AVRC_VENDOR_HDR_SIZE; /* pdu */ *p_data++ = 0; /* pkt_type */ UINT16_TO_BE_STREAM(p_data, 1); /* len */ *p_data++ = status; /* error code */ - p_pkt->len = AVRC_VENDOR_HDR_SIZE + 5; - p_rsp = p_pkt; + p_rsp->len = AVRC_VENDOR_HDR_SIZE + 5; } return p_rsp; @@ -574,6 +574,7 @@ static uint8_t avrc_proc_far_msg(uint8_t handle, uint8_t label, uint8_t cr, p_rsp = avrc_proc_vendor_command(handle, label, *pp_pkt, p_msg); if (p_rsp) { AVCT_MsgReq(handle, label, AVCT_RSP, p_rsp); + osi_free_and_reset((void**)pp_pkt); drop_code = 3; } else if (p_msg->hdr.opcode == AVRC_OP_DROP) { drop_code = 1; -- cgit v1.2.3