From 103b2c44d3db6172a2dc9319e215eeece504316e Mon Sep 17 00:00:00 2001 From: Jakub Pawlowski Date: Fri, 28 Apr 2017 14:59:46 -0700 Subject: [PATCH] LE advertise data parsing refactor * merge BTM_CheckAdvData and BTM_CheckEirData into GetFieldByType * check wether AD data is properly formatted when the packet is received. Some controllers were returning malformed data that resulted in stack crashes for BT5 packets. * add tests to verify the helpers work as expected Test: added net_test_stack_ad_parser Bug: 37671082 Change-Id: I49e43d7cf7d0a8ace1ee45d9b14b2b8440096b05 --- btif/src/btif_ble_scanner.cc | 8 ++-- btif/src/btif_dm.cc | 10 ++-- stack/Android.bp | 17 +++++++ stack/btm/btm_ble_gap.cc | 56 ++++++---------------- stack/btm/btm_inq.cc | 68 ++++++++------------------- stack/include/advertise_data_parser.h | 88 +++++++++++++++++++++++++++++++++++ stack/include/btm_api.h | 7 --- stack/include/btm_ble_api.h | 7 --- stack/test/ad_parser_unittest.cc | 81 ++++++++++++++++++++++++++++++++ test/run_unit_tests.sh | 1 + 10 files changed, 231 insertions(+), 112 deletions(-) create mode 100644 stack/include/advertise_data_parser.h create mode 100644 stack/test/ad_parser_unittest.cc diff --git a/btif/src/btif_ble_scanner.cc b/btif/src/btif_ble_scanner.cc index 4d87c7be3..2e24ecc6d 100644 --- a/btif/src/btif_ble_scanner.cc +++ b/btif/src/btif_ble_scanner.cc @@ -34,6 +34,7 @@ #include +#include "advertise_data_parser.h" #include "bta_api.h" #include "bta_closure_api.h" #include "bta_gatt_api.h" @@ -147,11 +148,11 @@ void bta_scan_results_cb_impl(bt_bdaddr_t bd_addr, tBT_DEVICE_TYPE device_type, bt_device_type_t dev_type; bt_property_t properties; - const uint8_t* p_eir_remote_name = BTM_CheckAdvData( + const uint8_t* p_eir_remote_name = AdvertiseDataParser::GetFieldByType( value, BTM_EIR_COMPLETE_LOCAL_NAME_TYPE, &remote_name_len); if (p_eir_remote_name == NULL) { - p_eir_remote_name = BTM_CheckAdvData( + p_eir_remote_name = AdvertiseDataParser::GetFieldByType( value, BT_EIR_SHORTENED_LOCAL_NAME_TYPE, &remote_name_len); } @@ -212,7 +213,8 @@ void bta_scan_results_cb(tBTA_DM_SEARCH_EVT event, tBTA_DM_SEARCH* p_data) { value.insert(value.begin(), p_data->inq_res.p_eir, p_data->inq_res.p_eir + p_data->inq_res.eir_len); - if (BTM_CheckAdvData(value, BTM_EIR_COMPLETE_LOCAL_NAME_TYPE, &len)) { + if (AdvertiseDataParser::GetFieldByType( + value, BTM_EIR_COMPLETE_LOCAL_NAME_TYPE, &len)) { p_data->inq_res.remt_name_not_required = true; } } diff --git a/btif/src/btif_dm.cc b/btif/src/btif_dm.cc index e428bd155..511d30722 100644 --- a/btif/src/btif_dm.cc +++ b/btif/src/btif_dm.cc @@ -43,6 +43,7 @@ #include +#include "advertise_data_parser.h" #include "bdaddr.h" #include "bt_common.h" #include "bta_closure_api.h" @@ -52,11 +53,8 @@ #include "btif_dm.h" #include "btif_hd.h" #include "btif_hh.h" -#include "btif_hh.h" #include "btif_sdp.h" #include "btif_storage.h" -#include "btif_storage.h" -#include "btif_util.h" #include "btif_util.h" #include "btu.h" #include "device/include/controller.h" @@ -344,16 +342,16 @@ bt_status_t btif_in_execute_service_request(tBTA_SERVICE_ID service_id, static bool check_eir_remote_name(tBTA_DM_SEARCH* p_search_data, uint8_t* p_remote_name, uint8_t* p_remote_name_len) { - uint8_t* p_eir_remote_name = NULL; + const uint8_t* p_eir_remote_name = NULL; uint8_t remote_name_len = 0; /* Check EIR for remote name and services */ if (p_search_data->inq_res.p_eir) { - p_eir_remote_name = BTM_CheckEirData( + p_eir_remote_name = AdvertiseDataParser::GetFieldByType( p_search_data->inq_res.p_eir, p_search_data->inq_res.eir_len, BTM_EIR_COMPLETE_LOCAL_NAME_TYPE, &remote_name_len); if (!p_eir_remote_name) { - p_eir_remote_name = BTM_CheckEirData( + p_eir_remote_name = AdvertiseDataParser::GetFieldByType( p_search_data->inq_res.p_eir, p_search_data->inq_res.eir_len, BTM_EIR_SHORTENED_LOCAL_NAME_TYPE, &remote_name_len); } diff --git a/stack/Android.bp b/stack/Android.bp index 43a554678..58c8fe957 100644 --- a/stack/Android.bp +++ b/stack/Android.bp @@ -272,3 +272,20 @@ cc_test { "libgmock", ], } + +// Bluetooth stack advertise data parsing unit tests for target +// ============================================================= +cc_test { + name: "net_test_stack_ad_parser", + defaults: ["fluoride_defaults"], + local_include_dirs: [ + "include", + ], + srcs: [ + "test/ad_parser_unittest.cc", + ], + static_libs: [ + "liblog", + "libgmock", + ], +} diff --git a/stack/btm/btm_ble_gap.cc b/stack/btm/btm_ble_gap.cc index 4ce05842d..caf610f3e 100644 --- a/stack/btm/btm_ble_gap.cc +++ b/stack/btm/btm_ble_gap.cc @@ -43,6 +43,7 @@ #include "hcimsgs.h" #include "osi/include/osi.h" +#include "advertise_data_parser.h" #include "btm_ble_int.h" #include "gatt_int.h" #include "gattdefs.h" @@ -1093,41 +1094,6 @@ void BTM_BleWriteScanRsp(uint8_t* data, uint8_t length, p_adv_data_cback(BTM_SUCCESS); } -/** - * This function returns a pointer inside the |adv| where a field of |type| is - * located, together with it' length in |p_length| - **/ -const uint8_t* BTM_CheckAdvData(std::vector const& adv, uint8_t type, - uint8_t* p_length) { - BTM_TRACE_API("%s: type=0x%02x", __func__, type); - - if (adv.empty()) { - *p_length = 0; - return NULL; - } - - size_t position = 0; - uint8_t length = adv[position]; - - while (length > 0 && (position < adv.size())) { - uint8_t adv_type = adv[position + 1]; - - if (adv_type == type) { - /* length doesn't include itself */ - *p_length = length - 1; /* minus the length of type */ - return adv.data() + position + 2; - } - - position += length + 1; /* skip the length of data */ - if (position >= adv.size()) break; - - length = adv[position]; - } - - *p_length = 0; - return NULL; -} - /******************************************************************************* * * Function BTM__BLEReadDiscoverability @@ -1707,8 +1673,8 @@ uint8_t btm_ble_is_discoverable(BD_ADDR bda, } if (!adv_data.empty()) { - const uint8_t* p_flag = - BTM_CheckAdvData(adv_data, BTM_BLE_AD_TYPE_FLAG, &data_len); + const uint8_t* p_flag = AdvertiseDataParser::GetFieldByType( + adv_data, BTM_BLE_AD_TYPE_FLAG, &data_len); if (p_flag != NULL) { flag = *p_flag; @@ -1893,7 +1859,8 @@ void btm_ble_update_inq_result(tINQ_DB_ENT* p_i, uint8_t addr_type, BD_ADDR bda, p_i->inq_count = p_inq->inq_counter; /* Mark entry for current inquiry */ if (!data.empty()) { - const uint8_t* p_flag = BTM_CheckAdvData(data, BTM_BLE_AD_TYPE_FLAG, &len); + const uint8_t* p_flag = + AdvertiseDataParser::GetFieldByType(data, BTM_BLE_AD_TYPE_FLAG, &len); if (p_flag != NULL) p_cur->flag = *p_flag; } @@ -1905,13 +1872,14 @@ void btm_ble_update_inq_result(tINQ_DB_ENT* p_i, uint8_t addr_type, BD_ADDR bda, * Otherwise fall back to trying to infer if it is a HID device based on the * service class. */ - const uint8_t* p_uuid16 = - BTM_CheckAdvData(data, BTM_BLE_AD_TYPE_APPEARANCE, &len); + const uint8_t* p_uuid16 = AdvertiseDataParser::GetFieldByType( + data, BTM_BLE_AD_TYPE_APPEARANCE, &len); if (p_uuid16 && len == 2) { btm_ble_appearance_to_cod((uint16_t)p_uuid16[0] | (p_uuid16[1] << 8), p_cur->dev_class); } else { - p_uuid16 = BTM_CheckAdvData(data, BTM_BLE_AD_TYPE_16SRV_CMPL, &len); + p_uuid16 = AdvertiseDataParser::GetFieldByType( + data, BTM_BLE_AD_TYPE_16SRV_CMPL, &len); if (p_uuid16 != NULL) { uint8_t i; for (i = 0; i + 2 <= len; i = i + 2) { @@ -2162,6 +2130,12 @@ static void btm_ble_process_adv_pkt_cont( return; } + if (!AdvertiseDataParser::IsValid(adv_data)) { + DVLOG(1) << __func__ << "Dropping bad advertisement packet: " + << base::HexEncode(adv_data.data(), adv_data.size()); + return; + } + tINQ_DB_ENT* p_i = btm_inq_db_find(bda); /* Check if this address has already been processed for this inquiry */ diff --git a/stack/btm/btm_inq.cc b/stack/btm/btm_inq.cc index a4137d595..687e89da7 100644 --- a/stack/btm/btm_inq.cc +++ b/stack/btm/btm_inq.cc @@ -34,6 +34,7 @@ #include "osi/include/osi.h" #include "osi/include/time.h" +#include "advertise_data_parser.h" #include "bt_common.h" #include "bt_types.h" #include "btm_api.h" @@ -118,10 +119,12 @@ static void btm_clr_inq_result_flt(void); static uint8_t btm_convert_uuid_to_eir_service(uint16_t uuid16); static void btm_set_eir_uuid(uint8_t* p_eir, tBTM_INQ_RESULTS* p_results); -static uint8_t* btm_eir_get_uuid_list(uint8_t* p_eir, size_t eir_len, - uint8_t uuid_size, uint8_t* p_num_uuid, - uint8_t* p_uuid_list_type); -static uint16_t btm_convert_uuid_to_uuid16(uint8_t* p_uuid, uint8_t uuid_size); +static const uint8_t* btm_eir_get_uuid_list(uint8_t* p_eir, size_t eir_len, + uint8_t uuid_size, + uint8_t* p_num_uuid, + uint8_t* p_uuid_list_type); +static uint16_t btm_convert_uuid_to_uuid16(const uint8_t* p_uuid, + uint8_t uuid_size); /******************************************************************************* * @@ -2288,41 +2291,6 @@ tBTM_STATUS BTM_WriteEIR(BT_HDR* p_buff) { } } -/** - * This function returns a pointer inside the |p_eir| array of length |eir_len| - * where a field of |type| is located, together with its length in |p_length| - */ -uint8_t* BTM_CheckEirData(uint8_t* p_eir, size_t eir_len, uint8_t type, - uint8_t* p_length) { - BTM_TRACE_API("%s: type=0x%02x", __func__, type); - - if (eir_len == 0) { - *p_length = 0; - return NULL; - } - - size_t position = 0; - uint8_t length = p_eir[position]; - - while (length > 0 && (position < eir_len)) { - uint8_t adv_type = p_eir[position + 1]; - - if (adv_type == type) { - /* length doesn't include itself */ - *p_length = length - 1; /* minus the length of type */ - return p_eir + position + 2; - } - - position += length + 1; /* skip the length of data */ - if (position >= eir_len) break; - - length = p_eir[position]; - } - - *p_length = 0; - return NULL; -} - /******************************************************************************* * * Function btm_convert_uuid_to_eir_service @@ -2500,7 +2468,7 @@ uint8_t BTM_GetEirSupportedServices(uint32_t* p_eir_uuid, uint8_t** p, uint8_t BTM_GetEirUuidList(uint8_t* p_eir, size_t eir_len, uint8_t uuid_size, uint8_t* p_num_uuid, uint8_t* p_uuid_list, uint8_t max_num_uuid) { - uint8_t* p_uuid_data; + const uint8_t* p_uuid_data; uint8_t type; uint8_t yy, xx; uint16_t* p_uuid16 = (uint16_t*)p_uuid_list; @@ -2561,10 +2529,11 @@ uint8_t BTM_GetEirUuidList(uint8_t* p_eir, size_t eir_len, uint8_t uuid_size, * beginning of UUID list in EIR - otherwise * ******************************************************************************/ -static uint8_t* btm_eir_get_uuid_list(uint8_t* p_eir, size_t eir_len, - uint8_t uuid_size, uint8_t* p_num_uuid, - uint8_t* p_uuid_list_type) { - uint8_t* p_uuid_data; +static const uint8_t* btm_eir_get_uuid_list(uint8_t* p_eir, size_t eir_len, + uint8_t uuid_size, + uint8_t* p_num_uuid, + uint8_t* p_uuid_list_type) { + const uint8_t* p_uuid_data; uint8_t complete_type, more_type; uint8_t uuid_len; @@ -2587,9 +2556,11 @@ static uint8_t* btm_eir_get_uuid_list(uint8_t* p_eir, size_t eir_len, break; } - p_uuid_data = BTM_CheckEirData(p_eir, eir_len, complete_type, &uuid_len); + p_uuid_data = AdvertiseDataParser::GetFieldByType(p_eir, eir_len, + complete_type, &uuid_len); if (p_uuid_data == NULL) { - p_uuid_data = BTM_CheckEirData(p_eir, eir_len, more_type, &uuid_len); + p_uuid_data = AdvertiseDataParser::GetFieldByType(p_eir, eir_len, more_type, + &uuid_len); *p_uuid_list_type = more_type; } else { *p_uuid_list_type = complete_type; @@ -2612,7 +2583,8 @@ static uint8_t* btm_eir_get_uuid_list(uint8_t* p_eir, size_t eir_len, * UUID 16-bit - otherwise * ******************************************************************************/ -static uint16_t btm_convert_uuid_to_uuid16(uint8_t* p_uuid, uint8_t uuid_size) { +static uint16_t btm_convert_uuid_to_uuid16(const uint8_t* p_uuid, + uint8_t uuid_size) { static const uint8_t base_uuid[LEN_UUID_128] = { 0xFB, 0x34, 0x9B, 0x5F, 0x80, 0x00, 0x00, 0x80, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; @@ -2668,7 +2640,7 @@ static uint16_t btm_convert_uuid_to_uuid16(uint8_t* p_uuid, uint8_t uuid_size) { * ******************************************************************************/ void btm_set_eir_uuid(uint8_t* p_eir, tBTM_INQ_RESULTS* p_results) { - uint8_t* p_uuid_data; + const uint8_t* p_uuid_data; uint8_t num_uuid; uint16_t uuid16; uint8_t yy; diff --git a/stack/include/advertise_data_parser.h b/stack/include/advertise_data_parser.h new file mode 100644 index 000000000..24ee2b378 --- /dev/null +++ b/stack/include/advertise_data_parser.h @@ -0,0 +1,88 @@ +/****************************************************************************** + * + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + ******************************************************************************/ + +#pragma once + +#include + +class AdvertiseDataParser { + public: + /** + * Return true if this |ad| represent properly formatted advertising data. + */ + static bool IsValid(const std::vector& ad) { + size_t position = 0; + + size_t ad_len = ad.size(); + while (position != ad_len) { + uint8_t len = ad[position]; + + // A field length of 0 would be invalid as it should at least contain the + // EIR field type. + if (len == 0) return false; + + // If the length of the current field would exceed the total data length, + // then the data is badly formatted. + if (position + len >= ad_len) { + return false; + } + + position += len + 1; + } + + return true; + } + + /** + * This function returns a pointer inside the |ad| array of length |ad_len| + * where a field of |type| is located, together with its length in |p_length| + */ + static const uint8_t* GetFieldByType(const uint8_t* ad, size_t ad_len, + uint8_t type, uint8_t* p_length) { + size_t position = 0; + + while (position != ad_len) { + uint8_t len = ad[position]; + + if (len == 0) break; + if (position + len >= ad_len) break; + + uint8_t adv_type = ad[position + 1]; + + if (adv_type == type) { + /* length doesn't include itself */ + *p_length = len - 1; /* minus the length of type */ + return ad + position + 2; + } + + position += len + 1; /* skip the length of data */ + } + + *p_length = 0; + return NULL; + } + + /** + * This function returns a pointer inside the |adv| where a field of |type| is + * located, together with it' length in |p_length| + */ + static const uint8_t* GetFieldByType(std::vector const& ad, + uint8_t type, uint8_t* p_length) { + return GetFieldByType(ad.data(), ad.size(), type, p_length); + } +}; diff --git a/stack/include/btm_api.h b/stack/include/btm_api.h index 7359453cc..812ecf52a 100644 --- a/stack/include/btm_api.h +++ b/stack/include/btm_api.h @@ -1874,13 +1874,6 @@ extern tBTM_STATUS BTM_DeleteStoredLinkKey(BD_ADDR bd_addr, tBTM_CMPL_CB* p_cb); ******************************************************************************/ extern tBTM_STATUS BTM_WriteEIR(BT_HDR* p_buff); -/** - * This function returns a pointer inside the |p_eir| array of length |eir_len| - * where a field of |type| is located, together with its length in |p_length| - */ -extern uint8_t* BTM_CheckEirData(uint8_t* p_eir, size_t eir_len, uint8_t type, - uint8_t* p_length); - /******************************************************************************* * * Function BTM_HasEirService diff --git a/stack/include/btm_ble_api.h b/stack/include/btm_ble_api.h index bb8c6ff8d..ee1ede637 100644 --- a/stack/include/btm_ble_api.h +++ b/stack/include/btm_ble_api.h @@ -480,13 +480,6 @@ extern void BTM_BleSetConnScanParams(uint32_t scan_interval, extern void BTM_BleReadControllerFeatures( tBTM_BLE_CTRL_FEATURES_CBACK* p_vsc_cback); -/** - * This function returns a pointer inside the |adv| where a field of |type| is - * located, together with it' length in |p_length| - **/ -extern const uint8_t* BTM_CheckAdvData(std::vector const& adv, - uint8_t type, uint8_t* p_length); - /******************************************************************************* * * Function BTM__BLEReadDiscoverability diff --git a/stack/test/ad_parser_unittest.cc b/stack/test/ad_parser_unittest.cc new file mode 100644 index 000000000..46ff3ec48 --- /dev/null +++ b/stack/test/ad_parser_unittest.cc @@ -0,0 +1,81 @@ +/****************************************************************************** + * + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + ******************************************************************************/ + +#include +#include "advertise_data_parser.h" + +TEST(AdvertiseDataParserTest, IsValidEmpty) { + const std::vector data0; + EXPECT_TRUE(AdvertiseDataParser::IsValid(data0)); + + // Single empty field not allowed. + const std::vector data1{0x00}; + EXPECT_FALSE(AdvertiseDataParser::IsValid(data1)); +} + +TEST(AdvertiseDataParserTest, IsValidBad) { + // Single field, field empty. + const std::vector data0{0x01}; + EXPECT_FALSE(AdvertiseDataParser::IsValid(data0)); + + // Single field, first field length too long. + const std::vector data1{0x05, 0x02, 0x00, 0x00, 0x00}; + EXPECT_FALSE(AdvertiseDataParser::IsValid(data1)); + + // Two fields, second field length too long. + const std::vector data2{0x02, 0x02, 0x00, 0x02, 0x00}; + EXPECT_FALSE(AdvertiseDataParser::IsValid(data2)); + + // Two fields, second field empty. + const std::vector data3{0x02, 0x02, 0x00, 0x01}; + EXPECT_FALSE(AdvertiseDataParser::IsValid(data3)); +} + +TEST(AdvertiseDataParserTest, IsValidGood) { + // Single field. + const std::vector data0{0x03, 0x02, 0x01, 0x02}; + EXPECT_TRUE(AdvertiseDataParser::IsValid(data0)); + + // Two fields. + const std::vector data1{0x03, 0x02, 0x01, 0x02, 0x02, 0x03, 0x01}; + EXPECT_TRUE(AdvertiseDataParser::IsValid(data1)); +} + +TEST(AdvertiseDataParserTest, GetFieldByType) { + // Single field. + const std::vector data0{0x03, 0x02, 0x01, 0x02}; + + uint8_t p_length; + const uint8_t* data = + AdvertiseDataParser::GetFieldByType(data0, 0x02, &p_length); + EXPECT_EQ(data0.data() + 2, data); + EXPECT_EQ(2, p_length); + + // Two fields, second field length too long. + const std::vector data1{0x02, 0x02, 0x00, 0x03, 0x00}; + + // First field is ok. + data = AdvertiseDataParser::GetFieldByType(data1, 0x02, &p_length); + EXPECT_EQ(data1.data() + 2, data); + EXPECT_EQ(0x01, p_length); + + // Second field have bad length. + data = AdvertiseDataParser::GetFieldByType(data1, 0x03, &p_length); + EXPECT_EQ(nullptr, data); + EXPECT_EQ(0, p_length); +} \ No newline at end of file diff --git a/test/run_unit_tests.sh b/test/run_unit_tests.sh index ecc6c36d9..f8488d73e 100755 --- a/test/run_unit_tests.sh +++ b/test/run_unit_tests.sh @@ -11,6 +11,7 @@ known_tests=( net_test_hci net_test_stack net_test_stack_multi_adv + net_test_stack_ad_parser net_test_stack_smp net_test_osi ) -- 2.11.0