Parser: validate that maps have both key and value items

If a map end (Break byte) occurs before we've read the concrete item for
the value, then the map is invalid.

Fixes #167

Signed-off-by: Thiago Macieira <[email protected]>

Author: thiagomacieira

src/cbor.h

@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2017 Intel Corporation
+** Copyright (C) 2019 Intel Corporation
 **
 ** Permission is hereby granted, free of charge, to any person obtaining a copy
 ** of this software and associated documentation files (the "Software"), to deal
@@ -270,7 +270,8 @@ enum CborParserIteratorFlags
     CborIteratorFlag_NegativeInteger        = 0x02,
     CborIteratorFlag_IteratingStringChunks  = 0x02,
     CborIteratorFlag_UnknownLength          = 0x04,
-    CborIteratorFlag_ContainerIsMap         = 0x20
+    CborIteratorFlag_ContainerIsMap         = 0x20,
+    CborIteratorFlag_NextIsMapKey           = 0x40
 };
 
 struct CborParser

src/cborparser.c

@@ -214,6 +214,10 @@ static bool is_fixed_type(uint8_t type)
 
 static CborError preparse_value(CborValue *it)
 {
+    enum {
+        /* flags to keep */
+        FlagsToKeep = CborIteratorFlag_ContainerIsMap | CborIteratorFlag_NextIsMapKey
+    };
     const CborParser *parser = it->parser;
     it->type = CborInvalidType;
 
@@ -224,7 +228,7 @@ static CborError preparse_value(CborValue *it)
     uint8_t descriptor = *it->ptr;
     uint8_t type = descriptor & MajorTypeMask;
     it->type = type;
-    it->flags = 0;
+    it->flags &= FlagsToKeep;
     it->extra = (descriptor &= SmallValueMask);
 
     if (descriptor > Value64Bit) {
@@ -302,6 +306,11 @@ static CborError preparse_next_value_nodecrement(CborValue *it)
 {
     if (it->remaining == UINT32_MAX && it->ptr != it->parser->end && *it->ptr == (uint8_t)BreakByte) {
         /* end of map or array */
+        if ((it->flags & CborIteratorFlag_ContainerIsMap && it->flags & CborIteratorFlag_NextIsMapKey)
+                || it->type == CborTagType) {
+            /* but we weren't expecting it! */
+            return CborErrorUnexpectedBreak;
+        }
         ++it->ptr;
         it->type = CborInvalidType;
         it->remaining = 0;
@@ -313,13 +322,20 @@ static CborError preparse_next_value_nodecrement(CborValue *it)
 
 static CborError preparse_next_value(CborValue *it)
 {
+    /* tags don't count towards item totals or whether we've successfully
+     * read a map's key or value */
+    bool itemCounts = it->type != CborTagType;
+
     if (it->remaining != UINT32_MAX) {
-        /* don't decrement the item count if the current item is tag: they don't count */
-        if (it->type != CborTagType && --it->remaining == 0) {
+        if (itemCounts && --it->remaining == 0) {
             it->type = CborInvalidType;
             return CborNoError;
         }
     }
+    if (itemCounts) {
+        /* toggle the flag indicating whether this was a map key */
+        it->flags ^= CborIteratorFlag_NextIsMapKey;
+    }
     return preparse_next_value_nodecrement(it);
 }
 
@@ -381,6 +397,7 @@ CborError cbor_parser_init(const uint8_t *buffer, size_t size, uint32_t flags, C
     it->parser = parser;
     it->ptr = buffer;
     it->remaining = 1;      /* there's one type altogether, usually an array or map */
+    it->flags = 0;
     return preparse_value(it);
 }
 
@@ -586,6 +603,7 @@ CborError cbor_value_skip_tag(CborValue *it)
  */
 CborError cbor_value_enter_container(const CborValue *it, CborValue *recursed)
 {
+    cbor_static_assert(CborIteratorFlag_ContainerIsMap == (CborMapType & ~CborArrayType));
     cbor_assert(cbor_value_is_container(it));
     *recursed = *it;
 
@@ -618,6 +636,7 @@ CborError cbor_value_enter_container(const CborValue *it, CborValue *recursed)
             return CborNoError;
         }
     }
+    recursed->flags = (recursed->type & CborIteratorFlag_ContainerIsMap);
     return preparse_next_value_nodecrement(recursed);
 }
 

tests/parser/tst_parser.cpp

@@ -1563,6 +1563,10 @@ static void addValidationData()
     QTest::newRow("array-no-break2") << raw("\x81\x9f\0") << 0 << CborErrorUnexpectedEOF;
     QTest::newRow("map-no-break1") << raw("\x81\xbf") << 0 << CborErrorUnexpectedEOF;
     QTest::newRow("map-no-break2") << raw("\x81\xbf\0\0") << 0 << CborErrorUnexpectedEOF;
+    QTest::newRow("map-break-after-key") << raw("\x81\xbf\0\xff") << 0 << CborErrorUnexpectedBreak;
+    QTest::newRow("map-break-after-second-key") << raw("\x81\xbf\x64xyzw\x04\x00\xff") << 0 << CborErrorUnexpectedBreak;
+    QTest::newRow("map-break-after-value-tag") << raw("\x81\xbf\0\xc0\xff") << 0 << CborErrorUnexpectedBreak;
+    QTest::newRow("map-break-after-value-tag2") << raw("\x81\xbf\0\xd8\x20\xff") << 0 << CborErrorUnexpectedBreak;
 
     // check for pointer additions wrapping over the limit of the address space
     CborError tooLargeOn32bit = (sizeof(void *) == 4) ? CborErrorDataTooLarge : CborErrorUnexpectedEOF;
@@ -1674,6 +1678,24 @@ void tst_Parser::validation()
         QCOMPARE(err2, expectedError);
         QCOMPARE(err3, expectedError);
     }
+
+    // see if we've got a map
+    if (QByteArray(QTest::currentDataTag()).startsWith("map")) {
+        w.init(data, uint32_t(flags));      // reinit
+        QVERIFY(cbor_value_is_array(&w.first));
+
+        CborValue map;
+        CborError err = cbor_value_enter_container(&w.first, &map);
+        if (err == CborNoError) {
+            QVERIFY(cbor_value_is_map(&map));
+            CborValue element;
+            err = cbor_value_map_find_value(&map, "foobar", &element);
+            if (err == CborNoError)
+                QVERIFY(!cbor_value_is_valid(&element));
+        }
+
+        QCOMPARE(err, expectedError);
+    }
 }
 
 void tst_Parser::strictValidation_data()