[spinel] Improve string field parse checks (#2200)

This change addresses an input validation error when parsing malformed
Spinel string fields. This change may have security implications.

This change also proactively adds an additional sanity check on the
size of the data buffers being passed to the pack/unpack functions,
limiting the maximum input size to 32767 bytes.

With the Spinel data packing format, fields with the type `U` are
zero-terminated strings. Failure to zero-terminate a string is a
syntax error that usually causes `spinel_datatype_vunpack_` to
explicitly fail. However, if there were no bytes left in the unparsed
data buffer when the parser gets to parsing a string field, then this
condition is triggered and undefined behavior results.

This bug could allow an attacker that is already in control of either
the NCP or the host to bypass the zero-termination check on a Spinel
string field, leading to undefined behavior on the other device and,
most likely, Denial Of Service. Note that if an attacker has already
compromised either the NCP or the host, the ability of the attacker to
deny service is a foregone conclusion.

This bug is not a buffer-overflow: this bug results in a garbage
string with no explicit zero termination being returned to the caller.
Such strings usually lead to crashes, not code execution. However, it
is not immediately clear that this bug couldn't be cleverly exploited
in such a way as to enable remote code execution. If such a method was
found this bug would be a vector for a compromised NCP to compromise
the host, and vise versa.

src/ncp/spinel.c

@@ -128,6 +128,8 @@ typedef struct {
     va_list obj;
 } va_list_obj;
 
+#define SPINEL_MAX_PACK_LENGTH         32767
+
 // ----------------------------------------------------------------------------
 // MARK: -
 
@@ -247,6 +249,9 @@ spinel_datatype_vunpack_(bool in_place, const uint8_t *data_ptr, spinel_size_t d
 {
     spinel_ssize_t ret = 0;
 
+    // Buffer length sanity check
+    require_action(data_len <= SPINEL_MAX_PACK_LENGTH, bail, (ret = -1, errno = EINVAL));
+
     for (; *pack_format != 0; pack_format = spinel_next_packed_datatype(pack_format))
     {
         if (*pack_format == ')')
@@ -425,9 +430,18 @@ spinel_datatype_vunpack_(bool in_place, const uint8_t *data_ptr, spinel_size_t d
 
         case SPINEL_DATATYPE_UTF8_C:
         {
-            size_t len = strnlen((const char *)data_ptr, data_len) + 1;
+            size_t len;
 
-            require_action((len <= data_len) || (data_ptr[data_len - 1] != 0), bail, (ret = -1, errno = EOVERFLOW));
+            // Make sure we have at least one byte.
+            require_action(data_len > 0, bail, (ret = -1, errno = EOVERFLOW));
+
+            // Add 1 for zero termination. If not zero terminated,
+            // len will then be data_len+1, which we will detect
+            // in the next check.
+            len = strnlen((const char *)data_ptr, data_len) + 1;
+
+            // Verify that the string is zero terminated.
+            require_action(len <= data_len, bail, (ret = -1, errno = EOVERFLOW));
 
             if (in_place)
             {
@@ -630,6 +644,9 @@ spinel_datatype_vpack_(uint8_t *data_ptr, spinel_size_t data_len_max, const char
 {
     spinel_ssize_t ret = 0;
 
+    // Buffer length sanity check
+    require_action(data_len_max <= SPINEL_MAX_PACK_LENGTH, bail, (ret = -1, errno = EINVAL));
+
     for (; *pack_format != 0; pack_format = spinel_next_packed_datatype(pack_format))
     {
         if (*pack_format == ')')
@@ -2029,6 +2046,27 @@ main(void)
         goto bail;
     }
 
+    {
+        const char *str = NULL;
+
+        // Length ends right before the string.
+        len = spinel_datatype_unpack(buffer, 8, "CiiLU", NULL, NULL, NULL, NULL, &str);
+
+        if (len != -1)
+        {
+            printf("error:%d: len != -1; (%d)\n", __LINE__, (int)len);
+            goto bail;
+        }
+
+        if (str != NULL)
+        {
+            printf("error:%d: str != NULL\n", __LINE__);
+            goto bail;
+        }
+    }
+
+    len = 30;
+
     {
         uint8_t c = 0;
         unsigned int i1 = 0;