From 457d2c4ab906e1166e8b2e19bf4b57dcbfd4b3c8 Mon Sep 17 00:00:00 2001 From: Chienyuan Date: Thu, 11 Oct 2018 09:47:46 +0800 Subject: [PATCH] DO NOT MERGE HFP: Check AT command buffer boundary during parsing * add p_end parameter to tBTA_AG_AT_CMD_CBACK, bta_ag_at_hsp_cback and bta_ag_at_hfp_cback to indicate effective data range of p_arg * add checks for buffer copy overflow in bta_ag_at_hsp_cback and bta_ag_at_hfp_cback * add packet legnth checks with p_end in bta_ag_parse_cmer * add packet length checks with p_end in bta_ag_parse_bac Bug: 112860487 Test: manual Change-Id: I6bbbc2ba29ad025c7d3ba023d8191af6a11c4aa9 (cherry picked from commit 749063afebb8324276a47bdfbf320aa70f94a8ba) (cherry picked from commit 9cb959d00d33737b399377cfc0f4070081d48f5e) --- bta/ag/bta_ag_act.cc | 2 +- bta/ag/bta_ag_at.cc | 15 +++++++++++---- bta/ag/bta_ag_at.h | 2 +- bta/ag/bta_ag_cmd.cc | 47 ++++++++++++++++++++++++++++++++++------------- bta/ag/bta_ag_int.h | 6 ++++-- 5 files changed, 51 insertions(+), 21 deletions(-) diff --git a/bta/ag/bta_ag_act.cc b/bta/ag/bta_ag_act.cc index ad40c5dd8..c58305417 100644 --- a/bta/ag/bta_ag_act.cc +++ b/bta/ag/bta_ag_act.cc @@ -58,7 +58,7 @@ const tBTA_SERVICE_MASK bta_ag_svc_mask[BTA_AG_NUM_IDX] = { BTA_HSP_SERVICE_MASK, BTA_HFP_SERVICE_MASK}; typedef void (*tBTA_AG_ATCMD_CBACK)(tBTA_AG_SCB* p_scb, uint16_t cmd, - uint8_t arg_type, char* p_arg, + uint8_t arg_type, char* p_arg, char* p_end, int16_t int_arg); const tBTA_AG_ATCMD_CBACK bta_ag_at_cback_tbl[BTA_AG_NUM_IDX] = { diff --git a/bta/ag/bta_ag_at.cc b/bta/ag/bta_ag_at.cc index 3e4868bff..7fde8e0a7 100644 --- a/bta/ag/bta_ag_at.cc +++ b/bta/ag/bta_ag_at.cc @@ -26,6 +26,7 @@ #include "bt_common.h" #include "bta_ag_at.h" +#include "log/log.h" #include "utl.h" /***************************************************************************** @@ -76,7 +77,7 @@ void bta_ag_at_reinit(tBTA_AG_AT_CB* p_cb) { * Returns void * *****************************************************************************/ -void bta_ag_process_at(tBTA_AG_AT_CB* p_cb) { +void bta_ag_process_at(tBTA_AG_AT_CB* p_cb, char* p_end) { uint16_t idx; uint8_t arg_type; char* p_arg; @@ -92,6 +93,11 @@ void bta_ag_process_at(tBTA_AG_AT_CB* p_cb) { if (p_cb->p_at_tbl[idx].p_cmd[0] != 0) { /* start of argument is p + strlen matching command */ p_arg = p_cb->p_cmd_buf + strlen(p_cb->p_at_tbl[idx].p_cmd); + if (p_arg > p_end) { + (*p_cb->p_err_cback)(p_cb->p_user, false, NULL); + android_errorWriteLog(0x534e4554, "112860487"); + return; + } /* if no argument */ if (p_arg[0] == 0) { @@ -132,11 +138,11 @@ void bta_ag_process_at(tBTA_AG_AT_CB* p_cb) { (*p_cb->p_err_cback)(p_cb->p_user, false, NULL); } else { (*p_cb->p_cmd_cback)(p_cb->p_user, p_cb->p_at_tbl[idx].command_id, - arg_type, p_arg, int_arg); + arg_type, p_arg, p_end, int_arg); } } else { (*p_cb->p_cmd_cback)(p_cb->p_user, p_cb->p_at_tbl[idx].command_id, - arg_type, p_arg, int_arg); + arg_type, p_arg, p_end, int_arg); } } /* else error */ @@ -187,8 +193,9 @@ void bta_ag_at_parse(tBTA_AG_AT_CB* p_cb, char* p_buf, uint16_t len) { (p_cb->p_cmd_buf[0] == 'A' || p_cb->p_cmd_buf[0] == 'a') && (p_cb->p_cmd_buf[1] == 'T' || p_cb->p_cmd_buf[1] == 't')) { p_save = p_cb->p_cmd_buf; + char* p_end = p_cb->p_cmd_buf + p_cb->cmd_pos; p_cb->p_cmd_buf += 2; - bta_ag_process_at(p_cb); + bta_ag_process_at(p_cb, p_end); p_cb->p_cmd_buf = p_save; } diff --git a/bta/ag/bta_ag_at.h b/bta/ag/bta_ag_at.h index 144bc3831..71ac442f0 100644 --- a/bta/ag/bta_ag_at.h +++ b/bta/ag/bta_ag_at.h @@ -55,7 +55,7 @@ typedef struct { /* callback function executed when command is parsed */ typedef void(tBTA_AG_AT_CMD_CBACK)(void* p_user, uint16_t command_id, - uint8_t arg_type, char* p_arg, + uint8_t arg_type, char* p_arg, char* p_end, int16_t int_arg); /* callback function executed to send "ERROR" result code */ diff --git a/bta/ag/bta_ag_cmd.cc b/bta/ag/bta_ag_cmd.cc index 79028db12..dbd38ed69 100644 --- a/bta/ag/bta_ag_cmd.cc +++ b/bta/ag/bta_ag_cmd.cc @@ -30,6 +30,7 @@ #include "bta_ag_int.h" #include "bta_api.h" #include "bta_sys.h" +#include "log/log.h" #include "osi/include/log.h" #include "osi/include/osi.h" #include "port_api.h" @@ -378,23 +379,23 @@ static void bta_ag_send_ind(tBTA_AG_SCB* p_scb, uint16_t id, uint16_t value, * Returns true if parsed ok, false otherwise. * ******************************************************************************/ -static bool bta_ag_parse_cmer(char* p_s, bool* p_enabled) { +static bool bta_ag_parse_cmer(char* p_s, char* p_end, bool* p_enabled) { int16_t n[4] = {-1, -1, -1, -1}; int i; char* p; - for (i = 0; i < 4; i++) { + for (i = 0; i < 4; i++, p_s = p + 1) { /* skip to comma delimiter */ - for (p = p_s; *p != ',' && *p != 0; p++) + for (p = p_s; p < p_end && *p != ',' && *p != 0; p++) ; /* get integer value */ + if (p > p_end) { + android_errorWriteLog(0x534e4554, "112860487"); + return false; + } *p = 0; n[i] = utl_str2int(p_s); - p_s = p + 1; - if (p_s == 0) { - break; - } } /* process values */ @@ -452,7 +453,8 @@ static uint8_t bta_ag_parse_chld(UNUSED_ATTR tBTA_AG_SCB* p_scb, char* p_s) { * Returns Returns bitmap of supported codecs. * ******************************************************************************/ -static tBTA_AG_PEER_CODEC bta_ag_parse_bac(tBTA_AG_SCB* p_scb, char* p_s) { +static tBTA_AG_PEER_CODEC bta_ag_parse_bac(tBTA_AG_SCB* p_scb, char* p_s, + char* p_end) { tBTA_AG_PEER_CODEC retval = BTA_AG_CODEC_NONE; uint16_t uuid_codec; bool cont = false; /* Continue processing */ @@ -460,10 +462,14 @@ static tBTA_AG_PEER_CODEC bta_ag_parse_bac(tBTA_AG_SCB* p_scb, char* p_s) { while (p_s) { /* skip to comma delimiter */ - for (p = p_s; *p != ',' && *p != 0; p++) + for (p = p_s; p < p_end && *p != ',' && *p != 0; p++) ; /* get integre value */ + if (p > p_end) { + android_errorWriteLog(0x534e4554, "112860487"); + break; + } if (*p != 0) { *p = 0; cont = true; @@ -597,7 +603,8 @@ void bta_ag_send_call_inds(tBTA_AG_SCB* p_scb, tBTA_AG_RES result) { * ******************************************************************************/ void bta_ag_at_hsp_cback(tBTA_AG_SCB* p_scb, uint16_t command_id, - uint8_t arg_type, char* p_arg, int16_t int_arg) { + uint8_t arg_type, char* p_arg, char* p_end, + int16_t int_arg) { APPL_TRACE_DEBUG("AT cmd:%d arg_type:%d arg:%d arg:%s", command_id, arg_type, int_arg, p_arg); @@ -607,6 +614,13 @@ void bta_ag_at_hsp_cback(tBTA_AG_SCB* p_scb, uint16_t command_id, val.hdr.handle = bta_ag_scb_to_idx(p_scb); val.hdr.app_id = p_scb->app_id; val.num = (uint16_t)int_arg; + + if ((p_end - p_arg + 1) >= (long)sizeof(val.str)) { + APPL_TRACE_ERROR("%s: p_arg is too long, send error and return", __func__); + bta_ag_send_error(p_scb, BTA_AG_ERR_TEXT_TOO_LONG); + android_errorWriteLog(0x534e4554, "112860487"); + return; + } strlcpy(val.str, p_arg, sizeof(val.str)); /* call callback with event */ @@ -836,7 +850,7 @@ static bool bta_ag_parse_biev_response(tBTA_AG_SCB* p_scb, tBTA_AG_VAL* val) { * ******************************************************************************/ void bta_ag_at_hfp_cback(tBTA_AG_SCB* p_scb, uint16_t cmd, uint8_t arg_type, - char* p_arg, int16_t int_arg) { + char* p_arg, char* p_end, int16_t int_arg) { tBTA_AG_VAL val; tBTA_AG_SCB* ag_scb; uint32_t i, ind_id; @@ -856,6 +870,13 @@ void bta_ag_at_hfp_cback(tBTA_AG_SCB* p_scb, uint16_t cmd, uint8_t arg_type, val.hdr.status = BTA_AG_SUCCESS; val.num = int_arg; val.bd_addr = p_scb->peer_addr; + + if ((p_end - p_arg + 1) >= (long)sizeof(val.str)) { + APPL_TRACE_ERROR("%s: p_arg is too long, send error and return", __func__); + bta_ag_send_error(p_scb, BTA_AG_ERR_TEXT_TOO_LONG); + android_errorWriteLog(0x534e4554, "112860487"); + return; + } strlcpy(val.str, p_arg, sizeof(val.str)); /** @@ -1034,7 +1055,7 @@ void bta_ag_at_hfp_cback(tBTA_AG_SCB* p_scb, uint16_t cmd, uint8_t arg_type, case BTA_AG_LOCAL_EVT_CMER: /* if parsed ok store setting, send OK */ - if (bta_ag_parse_cmer(p_arg, &p_scb->cmer_enabled)) { + if (bta_ag_parse_cmer(p_arg, p_end, &p_scb->cmer_enabled)) { bta_ag_send_ok(p_scb); /* if service level conn. not already open and our features and @@ -1195,7 +1216,7 @@ void bta_ag_at_hfp_cback(tBTA_AG_SCB* p_scb, uint16_t cmd, uint8_t arg_type, /* store available codecs from the peer */ if ((p_scb->peer_features & BTA_AG_PEER_FEAT_CODEC) && (p_scb->features & BTA_AG_FEAT_CODEC)) { - p_scb->peer_codecs = bta_ag_parse_bac(p_scb, p_arg); + p_scb->peer_codecs = bta_ag_parse_bac(p_scb, p_arg, p_end); p_scb->codec_updated = true; if (p_scb->peer_codecs & BTA_AG_CODEC_MSBC) { diff --git a/bta/ag/bta_ag_int.h b/bta/ag/bta_ag_int.h index 3ebf3690c..bb40f2209 100644 --- a/bta/ag/bta_ag_int.h +++ b/bta/ag/bta_ag_int.h @@ -361,9 +361,11 @@ extern void bta_ag_sco_conn_rsp(tBTA_AG_SCB* p_scb, /* AT command functions */ extern void bta_ag_at_hsp_cback(tBTA_AG_SCB* p_scb, uint16_t cmd, - uint8_t arg_type, char* p_arg, int16_t int_arg); + uint8_t arg_type, char* p_arg, char* p_end, + int16_t int_arg); extern void bta_ag_at_hfp_cback(tBTA_AG_SCB* p_scb, uint16_t cmd, - uint8_t arg_type, char* p_arg, int16_t int_arg); + uint8_t arg_type, char* p_arg, char* p_end, + int16_t int_arg); extern void bta_ag_at_err_cback(tBTA_AG_SCB* p_scb, bool unknown, char* p_arg); extern bool bta_ag_inband_enabled(tBTA_AG_SCB* p_scb); extern void bta_ag_send_call_inds(tBTA_AG_SCB* p_scb, tBTA_AG_RES result); -- 2.11.0