From 6c6dd65deba7e66bb2c40ef995074dc4ad42e843 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Mon, 16 Jul 2018 17:01:40 +0100 Subject: [PATCH] Refactor hid command, mitigate overflows Currently there are a few overflows possible in the hid command by sending malformed json requests. Refactor the code to avoid these overflows. These are mostly memcpy usage, where the size comes (indirectly) from the size of the json array. The json array must still be valid, because invalid json will produce an earlier exception in the java layer. Test: hid malformed_commands.json The file "malformed_commands.json" can be found in the bug. Bug: 111363077 Merged-In: I2f9dd31e0bfa2badc58779f40f4a80e025754cd2 (cherry picked from commit d54b70c8bfe393ef57445bd59962e6730b0b13c0) Change-Id: I2f9dd31e0bfa2badc58779f40f4a80e025754cd2 --- .../jni/com_android_commands_hid_Device.cpp | 51 +++++++++++-------- .../hid/jni/com_android_commands_hid_Device.h | 6 +-- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/cmds/hid/jni/com_android_commands_hid_Device.cpp b/cmds/hid/jni/com_android_commands_hid_Device.cpp index 5cc4fc4c16b22..b3e287bae76a8 100644 --- a/cmds/hid/jni/com_android_commands_hid_Device.cpp +++ b/cmds/hid/jni/com_android_commands_hid_Device.cpp @@ -42,7 +42,6 @@ namespace android { namespace uhid { static const char* UHID_PATH = "/dev/uhid"; -static const size_t UHID_MAX_NAME_LENGTH = 128; static struct { jmethodID onDeviceOpen; @@ -90,8 +89,13 @@ JNIEnv* DeviceCallback::getJNIEnv() { } Device* Device::open(int32_t id, const char* name, int32_t vid, int32_t pid, - std::unique_ptr descriptor, size_t descriptorSize, - std::unique_ptr callback) { + std::vector descriptor, std::unique_ptr callback) { + + size_t size = descriptor.size(); + if (size > HID_MAX_DESCRIPTOR_SIZE) { + LOGE("Received invalid hid report with descriptor size %zu, skipping", size); + return nullptr; + } int fd = ::open(UHID_PATH, O_RDWR | O_CLOEXEC); if (fd < 0) { @@ -102,10 +106,10 @@ Device* Device::open(int32_t id, const char* name, int32_t vid, int32_t pid, struct uhid_event ev; memset(&ev, 0, sizeof(ev)); ev.type = UHID_CREATE2; - strncpy((char*)ev.u.create2.name, name, UHID_MAX_NAME_LENGTH); - memcpy(&ev.u.create2.rd_data, descriptor.get(), - descriptorSize * sizeof(ev.u.create2.rd_data[0])); - ev.u.create2.rd_size = descriptorSize; + strlcpy(reinterpret_cast(ev.u.create2.name), name, sizeof(ev.u.create2.name)); + memcpy(&ev.u.create2.rd_data, descriptor.data(), + size * sizeof(ev.u.create2.rd_data[0])); + ev.u.create2.rd_size = size; ev.u.create2.bus = BUS_BLUETOOTH; ev.u.create2.vendor = vid; ev.u.create2.product = pid; @@ -156,12 +160,17 @@ Device::~Device() { mFd = -1; } -void Device::sendReport(uint8_t* report, size_t reportSize) { +void Device::sendReport(const std::vector& report) const { + if (report.size() > UHID_DATA_MAX) { + LOGE("Received invalid report of size %zu, skipping", report.size()); + return; + } + struct uhid_event ev; memset(&ev, 0, sizeof(ev)); ev.type = UHID_INPUT2; - ev.u.input2.size = reportSize; - memcpy(&ev.u.input2.data, report, reportSize); + ev.u.input2.size = report.size(); + memcpy(&ev.u.input2.data, report.data(), report.size() * sizeof(ev.u.input2.data[0])); ssize_t ret = TEMP_FAILURE_RETRY(::write(mFd, &ev, sizeof(ev))); if (ret < 0 || ret != sizeof(ev)) { LOGE("Failed to send hid event: %s", strerror(errno)); @@ -191,12 +200,13 @@ int Device::handleEvents(int events) { } // namespace uhid -std::unique_ptr getData(JNIEnv* env, jbyteArray javaArray, size_t& outSize) { +std::vector getData(JNIEnv* env, jbyteArray javaArray) { ScopedByteArrayRO scopedArray(env, javaArray); - outSize = scopedArray.size(); - std::unique_ptr data(new uint8_t[outSize]); - for (size_t i = 0; i < outSize; i++) { - data[i] = static_cast(scopedArray[i]); + size_t size = scopedArray.size(); + std::vector data; + data.reserve(size); + for (size_t i = 0; i < size; i++) { + data.push_back(static_cast(scopedArray[i])); } return data; } @@ -208,23 +218,20 @@ static jlong openDevice(JNIEnv* env, jclass /* clazz */, jstring rawName, jint i return 0; } - size_t size; - std::unique_ptr desc = getData(env, rawDescriptor, size); + std::vector desc = getData(env, rawDescriptor); std::unique_ptr cb(new uhid::DeviceCallback(env, callback)); uhid::Device* d = uhid::Device::open( - id, reinterpret_cast(name.c_str()), vid, pid, - std::move(desc), size, std::move(cb)); + id, reinterpret_cast(name.c_str()), vid, pid, desc, std::move(cb)); return reinterpret_cast(d); } static void sendReport(JNIEnv* env, jclass /* clazz */, jlong ptr, jbyteArray rawReport) { - size_t size; - std::unique_ptr report = getData(env, rawReport, size); + std::vector report = getData(env, rawReport); uhid::Device* d = reinterpret_cast(ptr); if (d) { - d->sendReport(report.get(), size); + d->sendReport(report); } else { LOGE("Could not send report, Device* is null!"); } diff --git a/cmds/hid/jni/com_android_commands_hid_Device.h b/cmds/hid/jni/com_android_commands_hid_Device.h index 149456d8c10d2..61a1f760697fb 100644 --- a/cmds/hid/jni/com_android_commands_hid_Device.h +++ b/cmds/hid/jni/com_android_commands_hid_Device.h @@ -15,6 +15,7 @@ */ #include +#include #include @@ -38,13 +39,12 @@ private: class Device { public: static Device* open(int32_t id, const char* name, int32_t vid, int32_t pid, - std::unique_ptr descriptor, size_t descriptorSize, - std::unique_ptr callback); + std::vector descriptor, std::unique_ptr callback); Device(int32_t id, int fd, std::unique_ptr callback); ~Device(); - void sendReport(uint8_t* report, size_t reportSize); + void sendReport(const std::vector& report) const; void close(); int handleEvents(int events);