From aaf4ab293c250bed51c33c6ec4b7e3c3dbbc44a9 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 27 Aug 2020 13:10:03 -0400 Subject: [PATCH 1/4] 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", ] } From c0e52ae2d8b8bbee709c70d192d1cf05bd6c8760 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Thu, 27 Aug 2020 17:31:01 +0000 Subject: [PATCH 2/4] Restyled by whitespace --- src/app/decoder.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/decoder.cpp b/src/app/decoder.cpp index 65d068e372e8ed..58e365f4b8af14 100644 --- a/src/app/decoder.cpp +++ b/src/app/decoder.cpp @@ -83,7 +83,7 @@ uint16_t extractApsFrame(uint8_t * buffer, uint32_t buf_length, EmberApsFrame * TRY_READ(radius, 1); #under TRY_READ - + return read_ptr - buffer; } From 44e3e6d76629e78579c1ae920af673c44c3ed3c1 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Thu, 27 Aug 2020 17:31:03 +0000 Subject: [PATCH 3/4] Restyled by clang-format --- src/app/decoder.cpp | 47 +++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/app/decoder.cpp b/src/app/decoder.cpp index 58e365f4b8af14..fac12473f83d4a 100644 --- a/src/app/decoder.cpp +++ b/src/app/decoder.cpp @@ -23,24 +23,26 @@ // InterPanHeader *interPanHeader) #include +#include #include #include #include -#include -template -struct Reader {}; +template +struct Reader +{ +}; -template<> struct Reader<1> { - static uint8_t read(const uint8_t*& p) { - return chip::Encoding::Read8(p); - } +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); - } +template <> +struct Reader<2> +{ + static uint16_t read(const uint8_t *& p) { return chip::Encoding::LittleEndian::Read16(p); } }; extern "C" { @@ -59,18 +61,17 @@ uint16_t extractApsFrame(uint8_t * buffer, uint32_t buf_length, EmberApsFrame * ++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; \ +#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); From 9e64148b9ee2531b325b6a5dcafcc4b87bc01189 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Thu, 27 Aug 2020 17:31:04 +0000 Subject: [PATCH 4/4] Restyled by gn --- src/lib/core/BUILD.gn | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/core/BUILD.gn b/src/lib/core/BUILD.gn index 02e2e62cc60978..7fab14ace16dde 100644 --- a/src/lib/core/BUILD.gn +++ b/src/lib/core/BUILD.gn @@ -111,12 +111,12 @@ 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", "${chip_root}/src/platform", "${chip_root}/src/system", - "${chip_root}/src/app", "${nlio_root}:nlio", ]