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

@@ -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 = [

src/app/decoder.cpp

@@ -23,88 +23,69 @@
 //                            InterPanHeader *interPanHeader)
 
 #include <app/chip-zcl-zpro-codec.h>
+#include <core/CHIPEncoding.h>
 #include <stdio.h>
 #include <string.h>
 #include <support/logging/CHIPLogging.h>
 
+template <int N>
+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<fieldSize>::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)

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",
   ]
 }