diff options
author | Andrew de los Reyes <adlr@google.com> | 2015-09-04 14:40:06 -0700 |
---|---|---|
committer | Andrew Duggan <aduggan@synaptics.com> | 2015-09-10 11:16:24 -0700 |
commit | 242ea83b394b44a8eec4cc4307cd98460ea114da (patch) | |
tree | 1a5d5fa8d3254b873604dd700cbee901df421e2e | |
parent | 074c44877931621f32459e80e105e10a9119bcc8 (diff) | |
download | rmi4utils-242ea83b394b44a8eec4cc4307cd98460ea114da.tar.gz |
validate m_*Report lengths
Addresses Security concerns:
HIDDevice::Open does not validate minimum sizes for m_*ReportSize, which
could lead to past-end-of-buffer writes when using m_*Report arrays.
HIDDevice::GetAttentionReport does not correctly validate the size of
the m_attnData buffer vs the buf len. This is a past-end-of-buffer read
condition. I don't understand the point of reading bytes-many bytes but
returning *len set to the valid size of bytes in the buffer.
-rw-r--r-- | rmidevice/hiddevice.cpp | 33 | ||||
-rw-r--r-- | rmidevice/hiddevice.h | 6 |
2 files changed, 32 insertions, 7 deletions
diff --git a/rmidevice/hiddevice.cpp b/rmidevice/hiddevice.cpp index 07e51cb..6e2a890 100644 --- a/rmidevice/hiddevice.cpp +++ b/rmidevice/hiddevice.cpp @@ -240,6 +240,9 @@ int HIDDevice::Read(unsigned short addr, unsigned char *buf, unsigned short len) else bytesToRequest = bytesPerRequest; + if (m_outputReportSize < HID_RMI4_READ_OUTPUT_COUNT + 2) { + return -1; + } m_outputReport[HID_RMI4_REPORT_ID] = RMI_READ_ADDR_REPORT_ID; m_outputReport[1] = 0; /* old 1 byte read count */ m_outputReport[HID_RMI4_READ_OUTPUT_ADDR] = addr & 0xFF; @@ -266,6 +269,10 @@ int HIDDevice::Read(unsigned short addr, unsigned char *buf, unsigned short len) while (bytesReadPerRequest < bytesToRequest) { rc = GetReport(&reportId); if (rc > 0 && reportId == RMI_READ_DATA_REPORT_ID) { + if (static_cast<ssize_t>(m_inputReportSize) < + std::max(HID_RMI4_READ_INPUT_COUNT, + HID_RMI4_READ_INPUT_DATA)) + return -1; bytesInDataReport = m_readData[HID_RMI4_READ_INPUT_COUNT]; memcpy(buf + bytesReadPerRequest, &m_readData[HID_RMI4_READ_INPUT_DATA], bytesInDataReport); @@ -286,6 +293,9 @@ int HIDDevice::Write(unsigned short addr, const unsigned char *buf, unsigned sho if (!m_deviceOpen) return -1; + if (static_cast<ssize_t>(m_outputReportSize) < + HID_RMI4_WRITE_OUTPUT_DATA + len) + return -1; m_outputReport[HID_RMI4_REPORT_ID] = RMI_WRITE_REPORT_ID; m_outputReport[HID_RMI4_WRITE_OUTPUT_COUNT] = len; m_outputReport[HID_RMI4_WRITE_OUTPUT_ADDR] = addr & 0xFF; @@ -353,7 +363,7 @@ int HIDDevice::GetAttentionReport(struct timeval * timeout, unsigned int source_ unsigned char *buf, unsigned int *len) { int rc = 0; - int bytes = m_inputReportSize; + unsigned int bytes = m_inputReportSize; int reportId; if (len && m_inputReportSize < *len) { @@ -367,8 +377,12 @@ int HIDDevice::GetAttentionReport(struct timeval * timeout, unsigned int source_ rc = GetReport(&reportId, timeout); if (rc > 0) { if (reportId == RMI_ATTN_REPORT_ID) { - if (buf) + if (buf) { + if (bytes > m_inputReportSize || + m_inputReportSize < HID_RMI4_ATTN_INTERUPT_SOURCES + 1) + return -1; memcpy(buf, m_attnData, bytes); + } if (source_mask & m_attnData[HID_RMI4_ATTN_INTERUPT_SOURCES]) return rc; } @@ -389,6 +403,9 @@ int HIDDevice::GetReport(int *reportId, struct timeval * timeout) if (!m_deviceOpen) return -1; + if (m_inputReportSize < HID_RMI4_REPORT_ID + 1) + return -1; + for (;;) { FD_ZERO(&fds); FD_SET(m_fd, &fds); @@ -424,10 +441,14 @@ int HIDDevice::GetReport(int *reportId, struct timeval * timeout) *reportId = m_inputReport[HID_RMI4_REPORT_ID]; if (m_inputReport[HID_RMI4_REPORT_ID] == RMI_ATTN_REPORT_ID) { - memcpy(m_attnData, m_inputReport, count); + if (static_cast<ssize_t>(m_inputReportSize) < count) + return -1; + memcpy(m_attnData, m_inputReport, count /*offset?*/); } else if (m_inputReport[HID_RMI4_REPORT_ID] == RMI_READ_DATA_REPORT_ID) { - memcpy(m_readData, m_inputReport, count); - m_dataBytesRead = count; + if (static_cast<ssize_t>(m_inputReportSize) < count) + return -1; + memcpy(m_readData, m_inputReport, count /*offset?*/); + m_dataBytesRead = count /*offset?*/; } return 1; } @@ -684,4 +705,4 @@ bool HIDDevice::FindHidRawFile(std::string & deviceName, std::string & hidrawFil closedir(devDir); return ret; -}
\ No newline at end of file +} diff --git a/rmidevice/hiddevice.h b/rmidevice/hiddevice.h index 97be0e3..05a11fa 100644 --- a/rmidevice/hiddevice.h +++ b/rmidevice/hiddevice.h @@ -26,7 +26,11 @@ class HIDDevice : public RMIDevice { public: HIDDevice() : RMIDevice(), m_inputReport(NULL), m_outputReport(NULL), m_attnData(NULL), - m_readData(NULL), m_deviceOpen(false) + m_readData(NULL), + m_inputReportSize(0), + m_outputReportSize(0), + m_featureReportSize(0), + m_deviceOpen(false) {} virtual int Open(const char * filename); virtual int Read(unsigned short addr, unsigned char *buf, |