From 643292f158ea1a729c01d5e5c328c6dc6da174cc Mon Sep 17 00:00:00 2001 From: Ugo Yu Date: Tue, 13 Nov 2018 20:03:28 +0800 Subject: [PATCH] Add OOB check in avrc_pars_browse_rsp Bug: 111451066 Test: Manully Change-Id: I068d218b8957bb8f053148d252a9119a8def28cc (cherry picked from commit f44cbb20e7658116472981bac0ffb0305f4a2c04) --- stack/avrc/avrc_pars_ct.cc | 100 +++++++++++++++++++++++++++++---------------- 1 file changed, 65 insertions(+), 35 deletions(-) diff --git a/stack/avrc/avrc_pars_ct.cc b/stack/avrc/avrc_pars_ct.cc index de7c2caa0..e6e6da81c 100644 --- a/stack/avrc/avrc_pars_ct.cc +++ b/stack/avrc/avrc_pars_ct.cc @@ -222,69 +222,88 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, uint8_t* p = p_msg->p_browse_data; /* read the pdu */ + if (p_msg->browse_len < 3) { + android_errorWriteLog(0x534e4554, "111451066"); + AVRC_TRACE_WARNING("%s: message length %d too short: must be at least 3", + __func__, p_msg->browse_len); + return AVRC_STS_BAD_PARAM; + } BE_STREAM_TO_UINT8(pdu, p); uint16_t pkt_len; + int min_len = 0; /* read the entire packet len */ BE_STREAM_TO_UINT16(pkt_len, p); - AVRC_TRACE_DEBUG("%s pdu %d", __func__, pdu); + AVRC_TRACE_DEBUG("%s pdu:%d, pkt_len:%d", __func__, pdu, pkt_len); - /* used to track how much we have read, if we cannot read anymore but the - * packet says so then we have a malformed packet. Also vice versa. */ - uint16_t pkt_len_read = 0; + if (p_msg->browse_len < (pkt_len + 3)) { + android_errorWriteLog(0x534e4554, "111451066"); + AVRC_TRACE_WARNING("%s: message length %d too short: must be at least %d", + __func__, p_msg->browse_len, pkt_len + 3); + return AVRC_STS_INTERNAL_ERR; + } switch (pdu) { case AVRC_PDU_GET_FOLDER_ITEMS: { tAVRC_GET_ITEMS_RSP* get_item_rsp = &(p_rsp->get_items); /* Copy back the PDU */ get_item_rsp->pdu = pdu; + + min_len += 1; + if (pkt_len < min_len) goto browse_length_error; /* read the status */ BE_STREAM_TO_UINT8(get_item_rsp->status, p); + if (get_item_rsp->status != AVRC_STS_NO_ERROR) { + AVRC_TRACE_WARNING("%s returning error %d", __func__, + get_item_rsp->status); + return get_item_rsp->status; + } + + min_len += 4; + if (pkt_len < min_len) goto browse_length_error; /* read the UID counter */ BE_STREAM_TO_UINT16(get_item_rsp->uid_counter, p); /* read the number of items */ BE_STREAM_TO_UINT16(get_item_rsp->item_count, p); - pkt_len_read += 5; AVRC_TRACE_DEBUG( "%s pdu %d status %d pkt_len %d uid counter %d item count %d", __func__, get_item_rsp->pdu, get_item_rsp->status, pkt_len, get_item_rsp->uid_counter, get_item_rsp->item_count); - if (get_item_rsp->status != AVRC_STS_NO_ERROR) { - AVRC_TRACE_WARNING("%s returning error %d", __func__, - get_item_rsp->status); - return get_item_rsp->status; - } - /* get each of the items */ get_item_rsp->p_item_list = (tAVRC_ITEM*)osi_malloc( 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++) { + min_len += 1; + if (pkt_len < min_len) goto browse_length_error; BE_STREAM_TO_UINT8(curr_item->item_type, p); - pkt_len_read += 1; AVRC_TRACE_DEBUG("%s item type %d", __func__, curr_item->item_type); switch (curr_item->item_type) { case AVRC_ITEM_PLAYER: { /* Handle player */ tAVRC_ITEM_PLAYER* player = &(curr_item->u.player); uint8_t player_len; + min_len += 10 + AVRC_FEATURE_MASK_SIZE; + if (pkt_len < min_len) goto browse_length_error; BE_STREAM_TO_UINT16(player_len, p); BE_STREAM_TO_UINT16(player->player_id, p); BE_STREAM_TO_UINT8(player->major_type, p); BE_STREAM_TO_UINT32(player->sub_type, p); BE_STREAM_TO_UINT8(player->play_status, p); BE_STREAM_TO_ARRAY(p, player->features, AVRC_FEATURE_MASK_SIZE); - pkt_len_read += (10 + AVRC_FEATURE_MASK_SIZE); /* read str */ + min_len += 4; + if (pkt_len < min_len) goto browse_length_error; BE_STREAM_TO_UINT16(player->name.charset_id, p); BE_STREAM_TO_UINT16(player->name.str_len, p); + min_len += player->name.str_len; + if (pkt_len < min_len) goto browse_length_error; player->name.p_str = (uint8_t*)osi_malloc( (player->name.str_len + 1) * sizeof(uint8_t)); BE_STREAM_TO_ARRAY(p, player->name.p_str, player->name.str_len); - pkt_len_read += (4 + player->name.str_len); AVRC_TRACE_DEBUG( "%s type %d id %d mtype %d stype %d ps %d cs %d name len %d", __func__, curr_item->item_type, player->player_id, @@ -295,20 +314,24 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, case AVRC_ITEM_FOLDER: { tAVRC_ITEM_FOLDER* folder = &(curr_item->u.folder); uint16_t folder_len; + min_len += 4 + AVRC_UID_SIZE; + if (pkt_len < min_len) goto browse_length_error; BE_STREAM_TO_UINT16(folder_len, p); BE_STREAM_TO_ARRAY(p, folder->uid, AVRC_UID_SIZE); BE_STREAM_TO_UINT8(folder->type, p); BE_STREAM_TO_UINT8(folder->playable, p); - pkt_len_read += (4 + AVRC_UID_SIZE); /* read str, encoding to be handled by upper layers */ + min_len += 4; + if (pkt_len < min_len) goto browse_length_error; BE_STREAM_TO_UINT16(folder->name.charset_id, p); BE_STREAM_TO_UINT16(folder->name.str_len, p); + min_len += folder->name.str_len; + if (pkt_len < min_len) goto browse_length_error; folder->name.p_str = (uint8_t*)osi_malloc( (folder->name.str_len + 1) * sizeof(uint8_t)); BE_STREAM_TO_ARRAY(p, folder->name.p_str, folder->name.str_len); - pkt_len_read += (4 + folder->name.str_len); AVRC_TRACE_DEBUG("%s type %d playable %d cs %d name len %d", __func__, folder->type, folder->playable, folder->name.charset_id, folder->name.str_len); @@ -317,14 +340,19 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, case AVRC_ITEM_MEDIA: { tAVRC_ITEM_MEDIA* media = &(curr_item->u.media); uint8_t media_len; + min_len += 3 + AVRC_UID_SIZE; + if (pkt_len < min_len) goto browse_length_error; BE_STREAM_TO_UINT16(media_len, p); BE_STREAM_TO_ARRAY(p, media->uid, AVRC_UID_SIZE); BE_STREAM_TO_UINT8(media->type, p); - pkt_len_read += (3 + AVRC_UID_SIZE); /* read str, encoding to be handled by upper layers */ + min_len += 4; + if (pkt_len < min_len) goto browse_length_error; BE_STREAM_TO_UINT16(media->name.charset_id, p); BE_STREAM_TO_UINT16(media->name.str_len, p); + min_len += 1 + media->name.str_len; + if (pkt_len < min_len) goto browse_length_error; media->name.p_str = (uint8_t*)osi_malloc((media->name.str_len) * sizeof(uint8_t)); BE_STREAM_TO_ARRAY(p, media->name.p_str, media->name.str_len); @@ -333,22 +361,24 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, AVRC_TRACE_DEBUG("%s media type %d charset id %d len %d attr ct %d", __func__, media->type, media->name.charset_id, media->name.str_len, media->attr_count); - pkt_len_read += (5 + media->name.str_len); media->p_attr_list = (tAVRC_ATTR_ENTRY*)osi_malloc( 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]); + min_len += 8; + if (pkt_len < min_len) goto browse_length_error; BE_STREAM_TO_UINT32(attr_entry->attr_id, p); /* Parse the name now */ BE_STREAM_TO_UINT16(attr_entry->name.charset_id, p); BE_STREAM_TO_UINT16(attr_entry->name.str_len, p); + 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); - pkt_len_read += (8 + attr_entry->name.str_len); AVRC_TRACE_DEBUG("%s media attr id %d cs %d name len %d", __func__, attr_entry->attr_id, attr_entry->name.charset_id, @@ -362,14 +392,8 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, return AVRC_STS_INTERNAL_ERR; } - /* we check if we have overrun */ - if (pkt_len_read > pkt_len) { - AVRC_TRACE_ERROR("%s overflow in read pkt_len %d pkt_len_read %d", - __func__, pkt_len, pkt_len_read); - return AVRC_STS_BAD_CMD; - } - AVRC_TRACE_DEBUG("%s pkt_len %d pkt_len_read %d", __func__, pkt_len, - pkt_len_read); + AVRC_TRACE_DEBUG("%s pkt_len %d min_len %d", __func__, pkt_len, + min_len); /* advance to populate the next item */ curr_item++; @@ -379,13 +403,14 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, case AVRC_PDU_CHANGE_PATH: { tAVRC_CHG_PATH_RSP* change_path_rsp = &(p_rsp->chg_path); + min_len += 5; + if (pkt_len < min_len) goto browse_length_error; /* Copyback the PDU */ change_path_rsp->pdu = pdu; /* Read the status */ BE_STREAM_TO_UINT8(change_path_rsp->status, p); /* Read the number of items in folder */ BE_STREAM_TO_UINT32(change_path_rsp->num_items, p); - pkt_len_read += 5; AVRC_TRACE_DEBUG("%s pdu %d status %d item count %d", __func__, change_path_rsp->pdu, change_path_rsp->status, @@ -399,6 +424,8 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, set_br_pl_rsp->pdu = pdu; /* Read the status */ + min_len += 10; + if (pkt_len < min_len) goto browse_length_error; BE_STREAM_TO_UINT8(set_br_pl_rsp->status, p); if (set_br_pl_rsp->status != AVRC_STS_NO_ERROR) { @@ -415,7 +442,6 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, "%s AVRC_PDU_SET_BROWSED_PLAYER status %d items %d cs %d depth %d", __func__, set_br_pl_rsp->status, set_br_pl_rsp->num_items, set_br_pl_rsp->charset_id, set_br_pl_rsp->folder_depth); - pkt_len_read += 10; set_br_pl_rsp->p_folders = (tAVRC_NAME*)osi_malloc( set_br_pl_rsp->num_items * sizeof(tAVRC_NAME)); @@ -423,13 +449,16 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, /* Read each of the folder in the depth */ for (uint32_t i = 0; i < set_br_pl_rsp->folder_depth; i++) { tAVRC_NAME* folder_name = &(set_br_pl_rsp->p_folders[i]); + min_len += 2; + if (pkt_len < min_len) goto browse_length_error; BE_STREAM_TO_UINT16(folder_name->str_len, p); + min_len += folder_name->str_len; + if (pkt_len < min_len) goto browse_length_error; AVRC_TRACE_DEBUG("%s AVRC_PDU_SET_BROWSED_PLAYER item: %d len: %d", __func__, i, folder_name->str_len); folder_name->p_str = (uint8_t*)osi_malloc((folder_name->str_len + 1) * sizeof(uint8_t)); BE_STREAM_TO_ARRAY(p, folder_name->p_str, folder_name->str_len); - pkt_len_read += (2 + folder_name->str_len); } break; } @@ -438,12 +467,13 @@ static tAVRC_STS avrc_pars_browse_rsp(tAVRC_MSG_BROWSE* p_msg, AVRC_TRACE_ERROR("%s pdu %d not handled", __func__, pdu); } - if (pkt_len != pkt_len_read) { - AVRC_TRACE_ERROR("%s finished pkt_len %d pkt_len_read %d", __func__, - pkt_len, pkt_len_read); - return AVRC_STS_BAD_CMD; - } return status; + +browse_length_error: + android_errorWriteLog(0x534e4554, "111451066"); + AVRC_TRACE_WARNING("%s: invalid parameter length %d: must be at least %d", + __func__, pkt_len, min_len); + return AVRC_STS_BAD_CMD; } /******************************************************************************* -- 2.11.0