From decc859fad7e92548ef0e69174c359e4e31809c8 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/Makefile.am | 1 + src/app/decoder.cpp | 115 ++++++++++++++++++------------------------ src/lib/core/BUILD.gn | 2 + 4 files changed, 53 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/Makefile.am b/src/app/Makefile.am index f713439d5c07e3..d09c0ec0f677b9 100644 --- a/src/app/Makefile.am +++ b/src/app/Makefile.am @@ -42,6 +42,7 @@ lib_LIBRARIES = libCHIPDataModel.a libCHIPDataModel_a_CPPFLAGS = \ -I$(top_srcdir)/src \ -I$(top_srcdir)/src/lib \ + $(NLIO_CPPFLAGS) \ $(NULL) libCHIPDataModel_a_SOURCES = \ diff --git a/src/app/decoder.cpp b/src/app/decoder.cpp index e59e7dcd45e996..86c48e97757083 100644 --- a/src/app/decoder.cpp +++ b/src/app/decoder.cpp @@ -23,88 +23,69 @@ // InterPanHeader *interPanHeader) #include +#include #include #include #include +template +struct Reader +{ +}; + +template <> +struct Reader<1> +{ + static uint8_t read(const uint8_t *& p) { return chip::Encoding::Read8(p); } +}; + +template <> +struct Reader<2> +{ + static uint16_t read(const uint8_t *& p) { return chip::Encoding::LittleEndian::Read16(p); } +}; + 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); - - if (nextByteToRead >= buf_length) - { - ChipLogError(Zcl, "Error extracting APS frame after reading profileId"); - return 0; - } - 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; - } - 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); - - if (nextByteToRead >= buf_length) - { - ChipLogError(Zcl, "Error extracting APS frame after reading groupId"); - return 0; - } - memcpy(&outApsFrame->sequence, buffer + nextByteToRead, sizeof(outApsFrame->sequence)); - nextByteToRead += sizeof(outApsFrame->sequence); + const uint8_t * read_ptr = buffer; - 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); - - 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); + +#undef 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..7fab14ace16dde 100644 --- a/src/lib/core/BUILD.gn +++ b/src/lib/core/BUILD.gn @@ -111,6 +111,7 @@ static_library("core") { public_deps = [ ":chip_config_header", + "${chip_root}/src/app", "${chip_root}/src/ble", "${chip_root}/src/inet", "${chip_root}/src/lib/support", @@ -125,5 +126,6 @@ static_library("core") { "${chip_root}/src/inet", "${chip_root}/src/platform", "${chip_root}/src/system", + "${chip_root}/src/app", ] }