Fix buffer overflow in BRSF
bta_hf_client_at does not properly check bounds on its inputs,
allowing a buffer overflow when fed a buffer that is more than
twice the expected maximum size. Add a new bounds check to
enforce, and a new security test to validate.
Bug: 231156521
Test: atest BtaHfClientSecurityTest
Tag: #security
Ignore-AOSP-First: Security
(cherry picked from commit f8adec665aa1e9b969e7efb1ee8790698a2e5b11)
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:691f010f3d2b4b4d1414f38a5dbf0cae7dae68a3)
Merged-In: I2cf89a786ba7cd0423eaccd8082bd824ac2f0d43
Change-Id: I2cf89a786ba7cd0423eaccd8082bd824ac2f0d43
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/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