binder: fix possible stack corruption

The stack could be corrupted by crafting a IPC call in interesting ways
when a character buffer was passed.

This patch also removes code duplication where these would occur.

Bug: 26917241
Change-Id: Ib6c149a293abf01f31c69a94c8f6dd91d8a2fff2

diff --git a/service/common/bluetooth/binder/IBluetoothGattServer.cpp b/service/common/bluetooth/binder/IBluetoothGattServer.cpp
index c9274f9..6013dc4 100644 (file)
--- a/service/common/bluetooth/binder/IBluetoothGattServer.cpp
+++ b/service/common/bluetooth/binder/IBluetoothGattServer.cpp
@@ -130,16 +130,11 @@ status_t BnBluetoothGattServer::onTransact(
     int status = data.readInt32();
     int offset = data.readInt32();
 
-    std::vector<uint8_t> value;
-    int value_len = data.readInt32();
-    if (value_len != -1) {
-      uint8_t bytes[value_len];
-      data.read(bytes, value_len);
-      value.insert(value.begin(), bytes, bytes + value_len);
-    }
+    auto value = ReadByteVectorFromParcel(data);
+    CHECK(value.get());
 
     bool result = SendResponse(
-        server_if, device_address, request_id, status, offset, value);
+        server_if, device_address, request_id, status, offset, *value);
 
     reply->writeInt32(result);
 
@@ -152,16 +147,11 @@ status_t BnBluetoothGattServer::onTransact(
     CHECK(char_id);
     bool confirm = data.readInt32();
 
-    std::vector<uint8_t> value;
-    int value_len = data.readInt32();
-    if (value_len != -1) {
-      uint8_t bytes[value_len];
-      data.read(bytes, value_len);
-      value.insert(value.begin(), bytes, bytes + value_len);
-    }
+    auto value = ReadByteVectorFromParcel(data);
+    CHECK(value.get());
 
     bool result = SendNotification(server_if, device_address, *char_id, confirm,
-                                   value);
+                                   *value);
 
     reply->writeInt32(result);
 

diff --git a/service/common/bluetooth/binder/IBluetoothGattServerCallback.cpp b/service/common/bluetooth/binder/IBluetoothGattServerCallback.cpp
index 8c79ab2..986dc9b 100644 (file)
--- a/service/common/bluetooth/binder/IBluetoothGattServerCallback.cpp
+++ b/service/common/bluetooth/binder/IBluetoothGattServerCallback.cpp
@@ -88,19 +88,14 @@ status_t BnBluetoothGattServerCallback::onTransact(
     bool is_prep = data.readInt32();
     bool need_rsp = data.readInt32();
 
-    std::vector<uint8_t> value;
-    int value_len = data.readInt32();
-    if (value_len != -1) {
-      uint8_t bytes[value_len];
-      data.read(bytes, value_len);
-      value.insert(value.begin(), bytes, bytes + value_len);
-    }
+    auto value = ReadByteVectorFromParcel(data);
+    CHECK(value.get());
 
     auto char_id = CreateGattIdentifierFromParcel(data);
     CHECK(char_id);
 
-    OnCharacteristicWriteRequest(
-        device_address, request_id, offset, is_prep, need_rsp, value, *char_id);
+    OnCharacteristicWriteRequest(device_address, request_id, offset, is_prep,
+                                 need_rsp, *value, *char_id);
     return android::NO_ERROR;
   }
   case ON_DESCRIPTOR_WRITE_REQUEST_TRANSACTION: {
@@ -110,19 +105,14 @@ status_t BnBluetoothGattServerCallback::onTransact(
     bool is_prep = data.readInt32();
     bool need_rsp = data.readInt32();
 
-    std::vector<uint8_t> value;
-    int value_len = data.readInt32();
-    if (value_len != -1) {
-      uint8_t bytes[value_len];
-      data.read(bytes, value_len);
-      value.insert(value.begin(), bytes, bytes + value_len);
-    }
+    auto value = ReadByteVectorFromParcel(data);
+    CHECK(value.get());
 
     auto desc_id = CreateGattIdentifierFromParcel(data);
     CHECK(desc_id);
 
-    OnDescriptorWriteRequest(
-        device_address, request_id, offset, is_prep, need_rsp, value, *desc_id);
+    OnDescriptorWriteRequest(device_address, request_id, offset, is_prep,
+                             need_rsp, *value, *desc_id);
     return android::NO_ERROR;
   }
   case ON_EXECUTE_WRITE_REQUEST_TRANSACTION: {

diff --git a/service/common/bluetooth/binder/parcel_helpers.cpp b/service/common/bluetooth/binder/parcel_helpers.cpp
index 141f940..b698b71 100644 (file)
--- a/service/common/bluetooth/binder/parcel_helpers.cpp
+++ b/service/common/bluetooth/binder/parcel_helpers.cpp
@@ -44,25 +44,13 @@ void WriteAdvertiseDataToParcel(const AdvertiseData& data, Parcel* parcel) {
 
 std::unique_ptr<AdvertiseData> CreateAdvertiseDataFromParcel(
     const Parcel& parcel) {
-  std::vector<uint8_t> data;
-
-  // For len=0 Parcel::writeByteArray writes "-1" for the length value. So, any
-  // other value means that there is data to read.
-  // TODO(pavlin): We shouldn't need to worry about his here. Instead, Parcel
-  // should have an API for deserializing an array of bytes (e.g.
-  // Parcel::readByteArray()).
-  int data_len = parcel.readInt32();
-  if (data_len != -1) {
-    uint8_t bytes[data_len];
-    parcel.read(bytes, data_len);
-
-    data = std::vector<uint8_t>(bytes, bytes + data_len);
-  }
+  auto data = ReadByteVectorFromParcel(parcel);
+  CHECK(data.get());
 
   bool include_device_name = parcel.readInt32();
   bool include_tx_power = parcel.readInt32();
 
-  std::unique_ptr<AdvertiseData> adv(new AdvertiseData(data));
+  std::unique_ptr<AdvertiseData> adv(new AdvertiseData(*data));
   adv->set_include_device_name(include_device_name);
   adv->set_include_tx_power_level(include_tx_power);
 
@@ -310,21 +298,27 @@ std::unique_ptr<bluetooth::ScanResult> CreateScanResultFromParcel(
   if (parcel.readInt32())
     device_address = parcel.readCString();
 
-  std::vector<uint8_t> scan_record;
-  if (parcel.readInt32()) {
-    int record_len = parcel.readInt32();
-    if (record_len != -1) {
-      uint8_t bytes[record_len];
-      parcel.read(bytes, record_len);
-
-      scan_record = std::vector<uint8_t>(bytes, bytes + record_len);
-    }
-  }
+  auto scan_record = ReadByteVectorFromParcel(parcel);
+  CHECK(scan_record.get());
 
   int rssi = parcel.readInt32();
 
   return std::unique_ptr<ScanResult>(new ScanResult(
-      device_address, scan_record, rssi));
+      device_address, *scan_record, rssi));
+}
+
+std::unique_ptr<std::vector<uint8_t>> ReadByteVectorFromParcel(
+    const android::Parcel& parcel) {
+  int32_t value_len = parcel.readInt32();
+  value_len = std::min(0, value_len);
+
+  std::unique_ptr<std::vector<uint8_t>> p(new std::vector<uint8_t>(value_len));
+
+  android::status_t result = parcel.read(p->data(), value_len);
+  if (result != android::NO_ERROR)
+    return nullptr;
+
+  return p;
 }
 
 }  // namespace binder

diff --git a/service/common/bluetooth/binder/parcel_helpers.h b/service/common/bluetooth/binder/parcel_helpers.h
index a4c1073..0052caa 100644 (file)
--- a/service/common/bluetooth/binder/parcel_helpers.h
+++ b/service/common/bluetooth/binder/parcel_helpers.h
@@ -95,5 +95,12 @@ void WriteScanResultToParcel(
 std::unique_ptr<bluetooth::ScanResult> CreateScanResultFromParcel(
     const android::Parcel& parcel);
 
+// Reads a byte vector from |parcel| which is packed as a Int32 value
+// followed by the indicated number of bytes.
+// Returns the read vector, or nullptr if there is an error reading the
+// vector.
+std::unique_ptr<std::vector<uint8_t>> ReadByteVectorFromParcel(
+    const android::Parcel& parcel);
+
 }  // namespace binder
 }  // namespace ipc

diff --git a/service/test/parcel_helpers_unittest.cpp b/service/test/parcel_helpers_unittest.cpp
index 138f9b5..2cc39be 100644 (file)
--- a/service/test/parcel_helpers_unittest.cpp
+++ b/service/test/parcel_helpers_unittest.cpp
@@ -97,6 +97,7 @@ bool TestScanResult(const ScanResult& result_in) {
   WriteScanResultToParcel(result_in, &parcel);
   parcel.setDataPosition(0);
   auto result_out = CreateScanResultFromParcel(parcel);
+  assert(result_out.get() != nullptr);
 
   return result_in == *result_out;
 }