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

@@ -26,85 +26,65 @@
 #include <stdio.h>
 #include <string.h>
 #include <support/logging/CHIPLogging.h>
+#include <core/CHIPEncoding.h>
 
-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<int N>
+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<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);
+
+#under TRY_READ
+
+    return read_ptr - buffer;
 }
 
 void printApsFrame(EmberApsFrame * frame)

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