Important spiffy decode fix to correctly handle some bad input cases (#150)
When fetching map items by label or using QCBORDecode_VGetNextConsume some bad input would result in an infinite loop. The bad input includes labels that are not strings or integers, non-well-formed maps and arrays and invalid tag content for tags like string and epoch dates. This can result in a denial-of-service attack, but not an overrun security issue.
These errors are now considered unrecoverable:
QCBOR_ERR_INDEF_LEN_ARRAYS_DISABLED
QCBOR_ERR_INDEF_LEN_STRINGS_DISABLED
QCBOR_ERR_MAP_LABEL_TYPE
QCBOR_ERR_UNRECOVERABLE_TAG_CONTENT
The error codes have been renumbered.
Spiffy decode traversal of maps and arrays with these errors will now correctly and directly fail. Without the fix they might fail incorrectly or get stuck in an infinite loop.
Tests to cover these cases are added.
Thanks to Jan Brandstetter and a fuzzing project for finding this.
* Fix error classification to make Consume work correctly
* Bring tests cases up to date with new behavior
* Documentation fixes
* error code renumbering
* Minor clean up
* fix one more test case
Co-authored-by: Laurence Lundblade <lgl@securitytheory.com>
diff --git a/src/qcbor_decode.c b/src/qcbor_decode.c
index 8217073..faa0218 100644
--- a/src/qcbor_decode.c
+++ b/src/qcbor_decode.c
@@ -1,6 +1,6 @@
/*==============================================================================
Copyright (c) 2016-2018, The Linux Foundation.
- Copyright (c) 2018-2021, Laurence Lundblade.
+ Copyright (c) 2018-2022, Laurence Lundblade.
Copyright (c) 2021, Arm Limited.
All rights reserved.
@@ -1525,7 +1525,7 @@
* combines pairs of items into one data item with a label
* and value.
*
- * This is passthrough if the current nesting leve is
+ * This is passthrough if the current nesting level is
* not a map.
*
* This also implements maps-as-array mode where a map
@@ -1913,7 +1913,7 @@
* @retval QCBOR_ERR_DATE_OVERFLOW
* @retval QCBOR_ERR_FLOAT_DATE_DISABLED
* @retval QCBOR_ERR_ALL_FLOAT_DISABLED
- * @retval QCBOR_ERR_BAD_TAG_CONTENT
+ * @retval QCBOR_ERR_UNRECOVERABLE_TAG_CONTENT
*
* The epoch date tag defined in QCBOR allows for floating-point
* dates. It even allows a protocol to flop between date formats when
@@ -1998,7 +1998,11 @@
break;
default:
- uReturn = QCBOR_ERR_BAD_TAG_CONTENT;
+ /* It's the arrays and maps that are unrecoverable because
+ * they are not consumed here. Since this is just an error
+ * condition, no extra code is added here to make the error
+ * recoverable for non-arrays and maps like strings. */
+ uReturn = QCBOR_ERR_UNRECOVERABLE_TAG_CONTENT;
goto Done;
}
@@ -2017,7 +2021,7 @@
* @retval QCBOR_ERR_DATE_OVERFLOW
* @retval QCBOR_ERR_FLOAT_DATE_DISABLED
* @retval QCBOR_ERR_ALL_FLOAT_DISABLED
- * @retval QCBOR_ERR_BAD_TAG_CONTENT
+ * @retval QCBOR_ERR_UNRECOVERABLE_TAG_CONTENT
*
* This is much simpler than the other epoch date format because
* floating-porint is not allowed. This is mostly a simple type check.
@@ -2041,7 +2045,11 @@
break;
default:
- uReturn = QCBOR_ERR_BAD_TAG_CONTENT;
+ /* It's the arrays and maps that are unrecoverable because
+ * they are not consumed here. Since this is just an error
+ * condition, no extra code is added here to make the error
+ * recoverable for non-arrays and maps like strings. */
+ uReturn = QCBOR_ERR_UNRECOVERABLE_TAG_CONTENT;
goto Done;
break;
}
@@ -2184,7 +2192,11 @@
} else if(pDecodedItem->uDataType == QCBOR_TYPE_BYTE_STRING) {
pDecodedItem->uDataType = QCBOR_TYPE_BINARY_MIME;
} else {
- return QCBOR_ERR_BAD_OPT_TAG;
+ /* It's the arrays and maps that are unrecoverable because
+ * they are not consumed here. Since this is just an error
+ * condition, no extra code is added here to make the error
+ * recoverable for non-arrays and maps like strings. */
+ return QCBOR_ERR_UNRECOVERABLE_TAG_CONTENT;
}
return QCBOR_SUCCESS;
@@ -2232,7 +2244,7 @@
*
* @returns This returns QCBOR_SUCCESS if the tag was procssed,
* \ref QCBOR_ERR_UNSUPPORTED if the tag was not processed and
- * \ref QCBOR_ERR_BAD_OPT_TAG if the content type was wrong for the tag.
+ * \ref QCBOR_ERR_UNRECOVERABLE_TAG_CONTENT if the content type was wrong for the tag.
*
* Process the CBOR tags that whose content is a byte string or a text
* string and for which the string is just passed on to the caller.
@@ -2269,7 +2281,11 @@
}
if(pDecodedItem->uDataType != uExpectedType) {
- return QCBOR_ERR_BAD_OPT_TAG;
+ /* It's the arrays and maps that are unrecoverable because
+ * they are not consumed here. Since this is just an error
+ * condition, no extra code is added here to make the error
+ * recoverable for non-arrays and maps like strings. */
+ return QCBOR_ERR_UNRECOVERABLE_TAG_CONTENT;
}
pDecodedItem->uDataType = (uint8_t)(uQCBORType & QCBOR_TYPE_MASK);
@@ -2755,9 +2771,18 @@
}
-/*
- Consume an entire map or array (and do next to
- nothing for non-aggregate types).
+/**
+ * @brief Consume an entire map or array including its contents.
+ *
+ * @param[in] pMe The decoder context.
+ * @param[in] pItemToConsume The array/map whose contents are to be
+ * consumed.
+ * @param[out] puNextNestLevel The next nesting level after the item was
+ * fully consumed.
+ *
+ * This may be called when @c pItemToConsume is not an array or
+ * map. In that case, this is just a pass through for @c puNextNestLevel
+ * since there is nothing to do.
*/
static inline QCBORError
ConsumeItem(QCBORDecodeContext *pMe,
@@ -2767,18 +2792,19 @@
QCBORError uReturn;
QCBORItem Item;
- // If it is a map or array, this will tell if it is empty.
+ /* If it is a map or array, this will tell if it is empty. */
const bool bIsEmpty = (pItemToConsume->uNextNestLevel <= pItemToConsume->uNestingLevel);
if(QCBORItem_IsMapOrArray(pItemToConsume) && !bIsEmpty) {
/* There is only real work to do for non-empty maps and arrays */
- /* This works for definite- and indefinite- length
- * maps and arrays by using the nesting level
+ /* This works for definite- and indefinite-length maps and
+ * arrays by using the nesting level
*/
do {
uReturn = QCBORDecode_GetNext(pMe, &Item);
- if(QCBORDecode_IsUnrecoverableError(uReturn)) {
+ if(QCBORDecode_IsUnrecoverableError(uReturn) ||
+ uReturn == QCBOR_ERR_NO_MORE_ITEMS) {
goto Done;
}
} while(Item.uNextNestLevel >= pItemToConsume->uNextNestLevel);
@@ -2788,8 +2814,8 @@
uReturn = QCBOR_SUCCESS;
} else {
- /* item_to_consume is not a map or array */
- /* Just pass the nesting level through */
+ /* pItemToConsume is not a map or array. Just pass the nesting
+ * level through. */
*puNextNestLevel = pItemToConsume->uNextNestLevel;
uReturn = QCBOR_SUCCESS;