commit 9123e5b11f859249f97770572e3e02d23036ab08
author David Navarro <dnav@users.noreply.github.com> Mon Mar 28 16:04:03 2022 +0200
committer GitHub <noreply@github.com> Mon Mar 28 07:04:03 2022 -0700
tree 336aa943ef0b8b6de70dfeb02183290092139a59
parent 7278700c32ec763b56efa1f5995c120c8af0140e

Handle zero-length chunks in indefinite-length strings

Previously this would generate a QCBOR_ERR_STRING_ALLOCATE error.  There is no security issue or attack vector here. QCBOR just errored out on a zero-length string chunk when it should not have.  Zero-length string chunks are explicitly allowed in RFC 8949

Thanks David for the catch and the fix!


* Fix decoding of an indefinite-length string with a zero-length first chunk. (#134)

Signed-off-by: David Navarro <david.navarro@ioterop.com>

* Add an unit test for #134.

Signed-off-by: David Navarro <david.navarro@ioterop.com>

Co-authored-by: David Navarro <david.navarro@ioterop.com>

src/qcbor_decode.c

diff --git a/src/qcbor_decode.c b/src/qcbor_decode.c
index 7ca2b81..8217073 100644
--- a/src/qcbor_decode.c
+++ b/src/qcbor_decode.c
@@ -1303,21 +1303,23 @@
          break;
       }
 
-      /* The first time throurgh FullString.ptr is NULL and this is
-       * equivalent to StringAllocator_Allocate(). Subsequently it is
-       * not NULL and a reallocation happens.
-       */
-      UsefulBuf NewMem = StringAllocator_Reallocate(pAllocator,
-                                                    FullString.ptr,
-                                                    FullString.len + StringChunkItem.val.string.len);
+      if (StringChunkItem.val.string.len > 0) {
+         /* The first time throurgh FullString.ptr is NULL and this is
+          * equivalent to StringAllocator_Allocate(). Subsequently it is
+          * not NULL and a reallocation happens.
+          */
+         UsefulBuf NewMem = StringAllocator_Reallocate(pAllocator,
+                                                       FullString.ptr,
+                                                       FullString.len + StringChunkItem.val.string.len);
 
-      if(UsefulBuf_IsNULL(NewMem)) {
-         uReturn = QCBOR_ERR_STRING_ALLOCATE;
-         break;
+         if(UsefulBuf_IsNULL(NewMem)) {
+            uReturn = QCBOR_ERR_STRING_ALLOCATE;
+            break;
+         }
+ 
+         /* Copy new string chunk to the end of accumulated string */
+         FullString = UsefulBuf_CopyOffset(NewMem, FullString.len, StringChunkItem.val.string);
       }
-
-      /* Copy new string chunk to the end of accumulated string */
-      FullString = UsefulBuf_CopyOffset(NewMem, FullString.len, StringChunkItem.val.string);
    }
 
    if(uReturn != QCBOR_SUCCESS && !UsefulBuf_IsNULLC(FullString)) {

test/qcbor_decode_tests.c

diff --git a/test/qcbor_decode_tests.c b/test/qcbor_decode_tests.c
index c440fb0..70fc014 100644
--- a/test/qcbor_decode_tests.c
+++ b/test/qcbor_decode_tests.c
@@ -6050,8 +6050,28 @@
    return 0;
 }
 
+int32_t CBORTestIssue134()
+{
+   QCBORDecodeContext DCtx;
+   QCBORItem          Item;
+   QCBORError         uCBORError;
+   const uint8_t      spTestIssue134[] = { 0x5F, 0x40, 0xFF };
 
+   QCBORDecode_Init(&DCtx,
+                    UsefulBuf_FROM_BYTE_ARRAY_LITERAL(spTestIssue134),
+                    QCBOR_DECODE_MODE_NORMAL);
 
+   UsefulBuf_MAKE_STACK_UB(StringBuf, 200);
+   QCBORDecode_SetMemPool(&DCtx, StringBuf, false);
+   
+   do {
+      uCBORError = QCBORDecode_GetNext(&DCtx, &Item);
+   } while (QCBOR_SUCCESS == uCBORError);
+
+   uCBORError = QCBORDecode_Finish(&DCtx);
+
+   return uCBORError;
+}
 
 int32_t CBORSequenceDecodeTests(void)
 {

test/qcbor_decode_tests.h

diff --git a/test/qcbor_decode_tests.h b/test/qcbor_decode_tests.h
index 54e125a..11fdc94 100644
--- a/test/qcbor_decode_tests.h
+++ b/test/qcbor_decode_tests.h
@@ -313,5 +313,9 @@
 */
 int32_t BoolTest(void);
 
+/*
+Test GitHub issue #134: decode an indefinite-length string with a zero-length first chunk.
+*/
+int32_t CBORTestIssue134(void);
 
 #endif /* defined(__QCBOR__qcbort_decode_tests__) */

test/run_tests.c

diff --git a/test/run_tests.c b/test/run_tests.c
index be2582f..a5c6635 100644
--- a/test/run_tests.c
+++ b/test/run_tests.c
@@ -111,6 +111,7 @@
     TEST_ENTRY(IndefiniteLengthStringTest),
     TEST_ENTRY(SpiffyIndefiniteLengthStringsTests),
     TEST_ENTRY(SetUpAllocatorTest),
+    TEST_ENTRY(CBORTestIssue134),
 #endif /* #ifndef QCBOR_DISABLE_INDEFINITE_LENGTH_STRINGS */
 #ifndef QCBOR_DISABLE_PREFERRED_FLOAT
     TEST_ENTRY(HalfPrecisionDecodeBasicTests),