From aaf4ab293c250bed51c33c6ec4b7e3c3dbbc44a9 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 27 Aug 2020 13:10:03 -0400 Subject: [PATCH] Fix buffer overruns and endianness issues in decoder code. There are two issues being fixed here: 1) The code was checking that it had at least one more byte available to read, but then reading multiple bytes in some cases. 2) The code was reading the data (which is little-endian) via memcpy, which would not do the right thing on big-endian systems. The former problem is fixed by checking that we can read the number of bytes we plan to read; the latter problem is fixed by using ChipEncoding to read the data explicitly as little-endian data. --- src/app/BUILD.gn | 2 + src/app/decoder.cpp | 114 +++++++++++++++++------------------------- src/lib/core/BUILD.gn | 2 + 3 files changed, 51 insertions(+), 67 deletions(-) diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 3c12d1c4f118a6..66d732557ef94b 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -13,6 +13,7 @@ # limitations under the License. import("//build_overrides/chip.gni") +import("//build_overrides/nlio.gni") config("app_config") { include_dirs = [ @@ -35,6 +36,7 @@ static_library("app") { ":codec_headers", "${chip_root}/src/lib/support", "${chip_root}/src/system", + "${nlio_root}:nlio", ] public_configs = [ diff --git a/src/app/decoder.cpp b/src/app/decoder.cpp index e59e7dcd45e996..65d068e372e8ed 100644 --- a/src/app/decoder.cpp +++ b/src/app/decoder.cpp @@ -26,85 +26,65 @@ #include #include #include +#include -extern "C" { - -uint16_t extractApsFrame(uint8_t * buffer, uint32_t buf_length, EmberApsFrame * outApsFrame) -{ - - if (buffer == NULL || buf_length == 0 || outApsFrame == NULL) - { - ChipLogError(Zcl, "Error extracting APS frame. invalid inputs"); - return 0; - } - // Skip first byte, because that's the always-0 frame control. - uint8_t nextByteToRead = 1; - - if (nextByteToRead >= buf_length) - { - ChipLogError(Zcl, "Error extracting APS frame after reading first byte"); - return 0; - } - memcpy(&outApsFrame->profileId, buffer + nextByteToRead, sizeof(outApsFrame->profileId)); - nextByteToRead += sizeof(outApsFrame->profileId); +template +struct Reader {}; - if (nextByteToRead >= buf_length) - { - ChipLogError(Zcl, "Error extracting APS frame after reading profileId"); - return 0; +template<> struct Reader<1> { + static uint8_t read(const uint8_t*& p) { + return chip::Encoding::Read8(p); } - memcpy(&outApsFrame->clusterId, buffer + nextByteToRead, sizeof(outApsFrame->clusterId)); - nextByteToRead += sizeof(outApsFrame->clusterId); +}; - if (nextByteToRead >= buf_length) - { - ChipLogError(Zcl, "Error extracting APS frame after reading clusterId"); - return 0; +template<> struct Reader<2> { + static uint16_t read(const uint8_t*& p) { + return chip::Encoding::LittleEndian::Read16(p); } - memcpy(&outApsFrame->sourceEndpoint, buffer + nextByteToRead, sizeof(outApsFrame->sourceEndpoint)); - nextByteToRead += sizeof(outApsFrame->sourceEndpoint); +}; - if (nextByteToRead >= buf_length) - { - ChipLogError(Zcl, "Error extracting APS frame after reading sourceEndpoint"); - return 0; - } - memcpy(&outApsFrame->destinationEndpoint, buffer + nextByteToRead, sizeof(outApsFrame->destinationEndpoint)); - nextByteToRead += sizeof(outApsFrame->destinationEndpoint); - - if (nextByteToRead >= buf_length) - { - ChipLogError(Zcl, "Error extracting APS frame after reading destinationEndpoint"); - return 0; - } - memcpy(&outApsFrame->options, buffer + nextByteToRead, sizeof(outApsFrame->options)); - nextByteToRead += sizeof(outApsFrame->options); - - if (nextByteToRead >= buf_length) - { - ChipLogError(Zcl, "Error extracting APS frame after reading options"); - return 0; - } - memcpy(&outApsFrame->groupId, buffer + nextByteToRead, sizeof(outApsFrame->groupId)); - nextByteToRead += sizeof(outApsFrame->groupId); +extern "C" { - if (nextByteToRead >= buf_length) +uint16_t extractApsFrame(uint8_t * buffer, uint32_t buf_length, EmberApsFrame * outApsFrame) +{ + if (buffer == NULL || buf_length == 0 || outApsFrame == NULL) { - ChipLogError(Zcl, "Error extracting APS frame after reading groupId"); + ChipLogError(Zcl, "Error extracting APS frame. invalid inputs"); return 0; } - memcpy(&outApsFrame->sequence, buffer + nextByteToRead, sizeof(outApsFrame->sequence)); - nextByteToRead += sizeof(outApsFrame->sequence); - if (nextByteToRead >= buf_length) - { - ChipLogError(Zcl, "Error extracting APS frame after reading sequence"); - return 0; - } - memcpy(&outApsFrame->radius, buffer + nextByteToRead, sizeof(outApsFrame->radius)); - nextByteToRead += sizeof(outApsFrame->radius); + const uint8_t * read_ptr = buffer; - return nextByteToRead; + // Skip first byte, because that's the always-0 frame control. + ++read_ptr; + --buf_length; + +#define TRY_READ(fieldName, fieldSize) \ + do { \ + static_assert(sizeof(outApsFrame->fieldName) == fieldSize, \ + "incorrect size for " #fieldName); \ + if (buf_length < fieldSize) \ + { \ + ChipLogError(Zcl, "Missing " #fieldName \ + " when extracting APS frame"); \ + return 0; \ + } \ + outApsFrame->fieldName = Reader::read(read_ptr); \ + buf_length -= fieldSize; \ + } while (0) + + TRY_READ(profileId, 2); + TRY_READ(clusterId, 2); + TRY_READ(sourceEndpoint, 1); + TRY_READ(destinationEndpoint, 1); + TRY_READ(options, 2); + TRY_READ(groupId, 2); + TRY_READ(sequence, 1); + TRY_READ(radius, 1); + +#under TRY_READ + + return read_ptr - buffer; } void printApsFrame(EmberApsFrame * frame) diff --git a/src/lib/core/BUILD.gn b/src/lib/core/BUILD.gn index 224b4b9faaefa0..02e2e62cc60978 100644 --- a/src/lib/core/BUILD.gn +++ b/src/lib/core/BUILD.gn @@ -116,6 +116,7 @@ static_library("core") { "${chip_root}/src/lib/support", "${chip_root}/src/platform", "${chip_root}/src/system", + "${chip_root}/src/app", "${nlio_root}:nlio", ] @@ -125,5 +126,6 @@ static_library("core") { "${chip_root}/src/inet", "${chip_root}/src/platform", "${chip_root}/src/system", + "${chip_root}/src/app", ] }