Merge cherrypicks of ['googleplex-android-review.googlesource.com/29465523', 'googleplex-android-review.googlesource.com/29467145', 'googleplex-android-review.googlesource.com/29465857'] into security-aosp-tm-release.
Change-Id: I7ec56f788f831e26b29934dbb4983a60fe0165c5
diff --git a/system/bta/Android.bp b/system/bta/Android.bp
index ff1d304..939f0be 100644
--- a/system/bta/Android.bp
+++ b/system/bta/Android.bp
@@ -194,6 +194,39 @@
],
}
+// bta unit tests for target
+cc_test {
+ name: "net_test_bta_security",
+ defaults: [
+ "fluoride_bta_defaults",
+ "mts_defaults"
+ ],
+ test_suites: ["device-tests"],
+ srcs: [
+ ":TestCommonMockFunctions",
+ ":TestMockDevice",
+ ":TestMockStack",
+ ":TestMockBtif",
+ "test/bta_hf_client_security_test.cc",
+ ],
+ shared_libs: [
+ "android.hardware.bluetooth.audio@2.0",
+ "android.hardware.bluetooth.audio@2.1",
+ "libcrypto",
+ "liblog",
+ "libprotobuf-cpp-lite",
+ ],
+ static_libs: [
+ "crypto_toolbox_for_tests",
+ "libbtcore",
+ "libbt-bta",
+ "libbt-audio-hal-interface",
+ "libbluetooth-types",
+ "libbt-protos-lite",
+ "libosi",
+ "libbt-common",
+ ],
+}
cc_test {
name: "bt_host_test_bta",
defaults: [
diff --git a/system/bta/hf_client/bta_hf_client_at.cc b/system/bta/hf_client/bta_hf_client_at.cc
index 5d3569b..5c08331 100644
--- a/system/bta/hf_client/bta_hf_client_at.cc
+++ b/system/bta/hf_client/bta_hf_client_at.cc
@@ -1721,6 +1721,13 @@
client_cb->at_cb.offset += tmp;
}
+ /* prevent buffer overflow in cases where LEN exceeds available buffer space
+ */
+ if (len > BTA_HF_CLIENT_AT_PARSER_MAX_LEN - client_cb->at_cb.offset) {
+ android_errorWriteWithInfoLog(0x534e4554, "231156521", -1, NULL, 0);
+ return;
+ }
+
memcpy(client_cb->at_cb.buf + client_cb->at_cb.offset, buf, len);
client_cb->at_cb.offset += len;
diff --git a/system/bta/test/bta_hf_client_security_test.cc b/system/bta/test/bta_hf_client_security_test.cc
new file mode 100644
index 0000000..a33c52e
--- /dev/null
+++ b/system/bta/test/bta_hf_client_security_test.cc
@@ -0,0 +1,79 @@
+/******************************************************************************
+ *
+ * Copyright 2022 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 <gtest/gtest.h>
+
+#include "bta/hf_client/bta_hf_client_int.h"
+#include "bta/include/bta_hf_client_api.h"
+#include "common/message_loop_thread.h"
+#include "device/include/esco_parameters.h"
+#include "test/mock/mock_device_controller.h"
+#include "types/raw_address.h"
+
+namespace base {
+class MessageLoop;
+} // namespace base
+
+bluetooth::common::MessageLoopThread* get_main_thread() { return nullptr; }
+void do_in_main_thread(base::Location const&, base::OnceCallback<void()>) {
+ return;
+}
+
+namespace {
+const RawAddress bdaddr1({0x11, 0x22, 0x33, 0x44, 0x55, 0x66});
+} // namespace
+
+// TODO(jpawlowski): there is some weird dependency issue in tests, and the
+// tests here fail to compile without this definition.
+void LogMsg(uint32_t trace_set_mask, const char* fmt_str, ...) {}
+
+class BtaHfClientSecurityTest : public testing::Test {
+ protected:
+ void SetUp() override {
+ // Reset the memory block, this is the state on which the allocate handle
+ // would start operating
+ bta_hf_client_cb_arr_init();
+ }
+};
+
+// Attempt to parse a buffer which exceeds available buffer space.
+// This should fail but not crash
+TEST_F(BtaHfClientSecurityTest, test_parse_overflow_buffer) {
+ uint16_t p_handle;
+ bool status = bta_hf_client_allocate_handle(bdaddr1, &p_handle);
+
+ tBTA_HF_CLIENT_CB* cb;
+
+ // Allocation should succeed
+ ASSERT_EQ(true, status);
+ ASSERT_GT(p_handle, 0);
+
+ cb = bta_hf_client_find_cb_by_bda(bdaddr1);
+
+ ASSERT_TRUE(cb != NULL);
+
+ uint16_t len = BTA_HF_CLIENT_AT_PARSER_MAX_LEN * 2 + 3;
+ char buf[BTA_HF_CLIENT_AT_PARSER_MAX_LEN * 2 + 3] = {'\n'};
+
+ bta_hf_client_at_parse(cb, (char*)(&buf[0]), len);
+
+ ASSERT_TRUE(len);
+ ASSERT_TRUE(buf != NULL);
+
+ ASSERT_TRUE(1);
+}
diff --git a/system/btif/src/btif_rc.cc b/system/btif/src/btif_rc.cc
index ad05be2..dae4b5c 100644
--- a/system/btif/src/btif_rc.cc
+++ b/system/btif/src/btif_rc.cc
@@ -3656,29 +3656,31 @@
* for standard attributes.
*/
p_app_settings->num_ext_attrs = 0;
- for (xx = 0; xx < p_app_settings->ext_attr_index; xx++) {
+ for (xx = 0;
+ xx < p_app_settings->ext_attr_index && xx < AVRC_MAX_APP_ATTR_SIZE;
+ xx++) {
osi_free_and_reset((void**)&p_app_settings->ext_attrs[xx].p_str);
}
p_app_settings->ext_attr_index = 0;
- if (p_dev) {
- for (xx = 0; xx < p_app_settings->num_attrs; xx++) {
- attrs[xx] = p_app_settings->attrs[xx].attr_id;
- }
-
- do_in_jni_thread(
- FROM_HERE,
- base::Bind(bt_rc_ctrl_callbacks->playerapplicationsetting_cb,
- p_dev->rc_addr, p_app_settings->num_attrs,
- p_app_settings->attrs, 0, nullptr));
- get_player_app_setting_cmd(xx, attrs, p_dev);
+ for (xx = 0; xx < p_app_settings->num_attrs && xx < AVRC_MAX_APP_ATTR_SIZE;
+ xx++) {
+ attrs[xx] = p_app_settings->attrs[xx].attr_id;
}
+
+ do_in_jni_thread(
+ FROM_HERE, base::Bind(bt_rc_ctrl_callbacks->playerapplicationsetting_cb,
+ p_dev->rc_addr, p_app_settings->num_attrs,
+ p_app_settings->attrs, 0, nullptr));
+ get_player_app_setting_cmd(xx, attrs, p_dev);
+
return;
}
for (xx = 0; xx < p_rsp->num_attr; xx++) {
uint8_t x;
- for (x = 0; x < p_app_settings->num_ext_attrs; x++) {
+ for (x = 0; x < p_app_settings->num_ext_attrs && x < AVRC_MAX_APP_ATTR_SIZE;
+ x++) {
if (p_app_settings->ext_attrs[x].attr_id == p_rsp->p_attrs[xx].attr_id) {
p_app_settings->ext_attrs[x].charset_id = p_rsp->p_attrs[xx].charset_id;
p_app_settings->ext_attrs[x].str_len = p_rsp->p_attrs[xx].str_len;
@@ -3688,7 +3690,9 @@
}
}
- for (xx = 0; xx < p_app_settings->ext_attrs[0].num_val; xx++) {
+ for (xx = 0;
+ xx < p_app_settings->ext_attrs[0].num_val && xx < BTRC_MAX_APP_ATTR_SIZE;
+ xx++) {
vals[xx] = p_app_settings->ext_attrs[0].ext_attr_val[xx].val;
}
get_player_app_setting_value_text_cmd(vals, xx, p_dev);
@@ -3732,11 +3736,13 @@
* for standard attributes.
*/
p_app_settings->num_ext_attrs = 0;
- for (xx = 0; xx < p_app_settings->ext_attr_index; xx++) {
+ for (xx = 0;
+ xx < p_app_settings->ext_attr_index && xx < AVRC_MAX_APP_ATTR_SIZE;
+ xx++) {
int x;
btrc_player_app_ext_attr_t* p_ext_attr = &p_app_settings->ext_attrs[xx];
- for (x = 0; x < p_ext_attr->num_val; x++)
+ for (x = 0; x < p_ext_attr->num_val && x < BTRC_MAX_APP_ATTR_SIZE; x++)
osi_free_and_reset((void**)&p_ext_attr->ext_attr_val[x].p_str);
p_ext_attr->num_val = 0;
osi_free_and_reset((void**)&p_app_settings->ext_attrs[xx].p_str);
@@ -3755,11 +3761,17 @@
return;
}
+ if (p_app_settings->ext_val_index >= AVRC_MAX_APP_ATTR_SIZE) {
+ BTIF_TRACE_ERROR("ext_val_index is 0x%02x, overflow!",
+ p_app_settings->ext_val_index);
+ return;
+ }
+
for (xx = 0; xx < p_rsp->num_attr; xx++) {
uint8_t x;
btrc_player_app_ext_attr_t* p_ext_attr;
p_ext_attr = &p_app_settings->ext_attrs[p_app_settings->ext_val_index];
- for (x = 0; x < p_rsp->num_attr; x++) {
+ for (x = 0; x < p_rsp->num_attr && x < BTRC_MAX_APP_ATTR_SIZE; x++) {
if (p_ext_attr->ext_attr_val[x].val == p_rsp->p_attrs[xx].attr_id) {
p_ext_attr->ext_attr_val[x].charset_id = p_rsp->p_attrs[xx].charset_id;
p_ext_attr->ext_attr_val[x].str_len = p_rsp->p_attrs[xx].str_len;
@@ -3812,10 +3824,12 @@
**************************************************************************/
static void cleanup_app_attr_val_txt_response(
btif_rc_player_app_settings_t* p_app_settings) {
- for (uint8_t xx = 0; xx < p_app_settings->ext_attr_index; xx++) {
+ for (uint8_t xx = 0;
+ xx < p_app_settings->ext_attr_index && xx < AVRC_MAX_APP_ATTR_SIZE;
+ xx++) {
int x;
btrc_player_app_ext_attr_t* p_ext_attr = &p_app_settings->ext_attrs[xx];
- for (x = 0; x < p_ext_attr->num_val; x++) {
+ for (x = 0; x < p_ext_attr->num_val && x < BTRC_MAX_APP_ATTR_SIZE; x++) {
osi_free_and_reset((void**)&p_ext_attr->ext_attr_val[x].p_str);
}
p_ext_attr->num_val = 0;
diff --git a/system/stack/l2cap/l2c_ble.cc b/system/stack/l2cap/l2c_ble.cc
index 5ab208c..19b74f7 100755
--- a/system/stack/l2cap/l2c_ble.cc
+++ b/system/stack/l2cap/l2c_ble.cc
@@ -638,7 +638,7 @@
break;
}
case L2CAP_CMD_CREDIT_BASED_CONN_RES:
- if (p + 2 > p_pkt_end) {
+ if (p + 8 > p_pkt_end) {
LOG(ERROR) << "invalid L2CAP_CMD_CREDIT_BASED_CONN_RES len";
return;
}
diff --git a/system/test/mock/mock_device_esco_parameters.cc b/system/test/mock/mock_device_esco_parameters.cc
index 86410fe..a772568 100644
--- a/system/test/mock/mock_device_esco_parameters.cc
+++ b/system/test/mock/mock_device_esco_parameters.cc
@@ -49,5 +49,10 @@
mock_function_count_map[__func__]++;
return test::mock::device_esco_parameters::esco_parameters_for_codec(codec);
}
+
+enh_esco_params_t esco_parameters_for_codec(esco_codec_t codec, bool b) {
+ mock_function_count_map[__func__]++;
+ return test::mock::device_esco_parameters::esco_parameters_for_codec(codec);
+}
// Mocked functions complete
// END mockcify generation
diff --git a/system/test/run_unit_tests.sh b/system/test/run_unit_tests.sh
index 3b17dd1..ace51b4 100755
--- a/system/test/run_unit_tests.sh
+++ b/system/test/run_unit_tests.sh
@@ -10,6 +10,7 @@
net_test_bluetooth
net_test_btcore
net_test_bta
+ net_test_bta_security
net_test_btif
net_test_btif_profile_queue
net_test_btif_config_cache
@@ -118,6 +119,9 @@
name="${spec%%.*}"
if [[ $target_arch == *"64"* ]]; then
binary="/data/nativetest64/${name}/${name}"
+ if [ -e "${ANDROID_PRODUCT_OUT}${binary}64" ]; then
+ binary="/data/nativetest64/${name}/${name}64"
+ fi
else
binary="/data/nativetest/${name}/${name}"
fi