Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit f9ac0f68 authored by Brian Delwiche's avatar Brian Delwiche Committed by Android Build Coastguard Worker
Browse files

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

Change-Id: I2cf89a786ba7cd0423eaccd8082bd824ac2f0d43
(cherry picked from commit f8adec66)
Merged-In: I2cf89a786ba7cd0423eaccd8082bd824ac2f0d43
parent 329fe84b

system/bta/Android.bp

+33 −0
Original line number Diff line number Diff line
@@ -197,6 +197,39 @@ cc_test { 
    ],

}



// 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: [

system/bta/hf_client/bta_hf_client_at.cc

+7 −0
Original line number Diff line number Diff line
@@ -1721,6 +1721,13 @@ void bta_hf_client_at_parse(tBTA_HF_CLIENT_CB* client_cb, char* buf, 
    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;

system/bta/test/bta_hf_client_security_test.cc

0 → 100644
+79 −0
Original line number Diff line number Diff line 
/******************************************************************************

 *

 *  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);

}

system/test/mock/mock_device_esco_parameters.cc

+5 −0
Original line number Diff line number Diff line
@@ -49,5 +49,10 @@ enh_esco_params_t esco_parameters_for_codec(esco_codec_t codec) { 
  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

system/test/run_unit_tests.sh

+4 −0
Original line number Diff line number Diff line
@@ -10,6 +10,7 @@ known_tests=( 
  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 @@ do 
  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