Fix incorrect behavior when closing unopened map, array or bstr
Undefined behavior occurred if QCBOREncode_CloseArray(), QCBOREncode_CloseMap(), or QCBOREncode_CloseBstrWrap() were called more than their corresponding open was called.
This manifested as a test case failing on some platforms (but not others).
There is no issue with correct use of QCBOREncode_CloseArray(), QCBOREncode_CloseMap(), or QCBOREncode_CloseBstrWrap()
This bug was introduced with the change to remove encode guards some years ago.
Test coverage has been improved, particularly around the encode guards both when they are on and when they are off.
* Fix test failures related to extraneously map/array/bstr closes
* run encode-guard-related tests both with encode gaurds on and off
* some code refactoring to be more clear
Co-authored-by: Laurence Lundblade <lgl@securitytheory.com>
diff --git a/src/qcbor_encode.c b/src/qcbor_encode.c
index af0e38b..3a0a968 100644
--- a/src/qcbor_encode.c
+++ b/src/qcbor_encode.c
@@ -72,7 +72,8 @@
* with the type CBOR_MAJOR_TYPE_BYTE_STRING and is tracked here. Byte
* string wrapped CBOR is used by COSE for data that is to be hashed.
*/
-inline static void Nesting_Init(QCBORTrackNesting *pNesting)
+static inline void
+Nesting_Init(QCBORTrackNesting *pNesting)
{
/* Assumes pNesting has been zeroed. */
pNesting->pCurrentNesting = &pNesting->pArrays[0];
@@ -82,9 +83,10 @@
pNesting->pCurrentNesting->uMajorType = CBOR_MAJOR_TYPE_ARRAY;
}
-inline static uint8_t Nesting_Increase(QCBORTrackNesting *pNesting,
- uint8_t uMajorType,
- uint32_t uPos)
+static inline uint8_t
+Nesting_Increase(QCBORTrackNesting *pNesting,
+ uint8_t uMajorType,
+ uint32_t uPos)
{
if(pNesting->pCurrentNesting == &pNesting->pArrays[QCBOR_MAX_ARRAY_NESTING]) {
return QCBOR_ERR_ARRAY_NESTING_TOO_DEEP;
@@ -97,12 +99,16 @@
}
}
-inline static void Nesting_Decrease(QCBORTrackNesting *pNesting)
+static inline void
+Nesting_Decrease(QCBORTrackNesting *pNesting)
{
- pNesting->pCurrentNesting--;
+ if(pNesting->pCurrentNesting > &pNesting->pArrays[0]) {
+ pNesting->pCurrentNesting--;
+ }
}
-inline static uint8_t Nesting_Increment(QCBORTrackNesting *pNesting)
+static inline uint8_t
+Nesting_Increment(QCBORTrackNesting *pNesting)
{
#ifndef QCBOR_DISABLE_ENCODE_USAGE_GUARDS
if(1 >= QCBOR_MAX_ITEMS_IN_ARRAY - pNesting->pCurrentNesting->uCount) {
@@ -115,7 +121,8 @@
return QCBOR_SUCCESS;
}
-inline static void Nesting_Decrement(QCBORTrackNesting *pNesting)
+static inline void
+Nesting_Decrement(QCBORTrackNesting *pNesting)
{
/* No error check for going below 0 here needed because this
* is only used by QCBOREncode_CancelBstrWrap() and it checks
@@ -123,7 +130,8 @@
pNesting->pCurrentNesting->uCount--;
}
-inline static uint16_t Nesting_GetCount(QCBORTrackNesting *pNesting)
+static inline uint16_t
+Nesting_GetCount(QCBORTrackNesting *pNesting)
{
/* The nesting count recorded is always the actual number of
* individual data items in the array or map. For arrays CBOR uses
@@ -140,18 +148,21 @@
}
}
-inline static uint32_t Nesting_GetStartPos(QCBORTrackNesting *pNesting)
+static inline uint32_t
+Nesting_GetStartPos(QCBORTrackNesting *pNesting)
{
return pNesting->pCurrentNesting->uStart;
}
#ifndef QCBOR_DISABLE_ENCODE_USAGE_GUARDS
-inline static uint8_t Nesting_GetMajorType(QCBORTrackNesting *pNesting)
+static inline uint8_t
+Nesting_GetMajorType(QCBORTrackNesting *pNesting)
{
return pNesting->pCurrentNesting->uMajorType;
}
-inline static bool Nesting_IsInNest(QCBORTrackNesting *pNesting)
+static inline bool
+Nesting_IsInNest(QCBORTrackNesting *pNesting)
{
return pNesting->pCurrentNesting == &pNesting->pArrays[0] ? false : true;
}
@@ -518,6 +529,53 @@
/**
+ * @brief Check for errors when decreasing nesting.
+ *
+ * @param pMe QCBOR encoding context.
+ * @param uMajorType The major type of the nesting.
+ *
+ * Check that there is no previous error, that there is actually some
+ * nesting and that the major type of the opening of the nesting
+ * matches the major type of the nesting being closed.
+ *
+ * This is called when closing maps, arrays, byte string wrapping and
+ * open/close of byte strings.
+ */
+bool
+CheckDecreaseNesting(QCBOREncodeContext *pMe, uint8_t uMajorType)
+{
+#ifndef QCBOR_DISABLE_ENCODE_USAGE_GUARDS
+ if(pMe->uError != QCBOR_SUCCESS) {
+ return true;
+ }
+
+ if(!Nesting_IsInNest(&(pMe->nesting))) {
+ pMe->uError = QCBOR_ERR_TOO_MANY_CLOSES;
+ return true;
+ }
+
+ if(Nesting_GetMajorType(&(pMe->nesting)) != uMajorType) {
+ pMe->uError = QCBOR_ERR_CLOSE_MISMATCH;
+ return true;
+ }
+
+#else
+ /* None of these checks are performed if the encode guards are
+ * turned off as they all relate to correct calling.
+ *
+ * Turning off all these checks does not turn off any checking for
+ * buffer overflows or pointer issues.
+ */
+
+ (void)uMajorType;
+ (void)pMe;
+#endif
+
+ return false;
+}
+
+
+/**
* @brief Insert the CBOR head for a map, array or wrapped bstr
*
* @param me QCBOR encoding context.
@@ -530,22 +588,15 @@
*/
static void InsertCBORHead(QCBOREncodeContext *me, uint8_t uMajorType, size_t uLen)
{
-#ifndef QCBOR_DISABLE_ENCODE_USAGE_GUARDS
- if(me->uError == QCBOR_SUCCESS) {
- if(!Nesting_IsInNest(&(me->nesting))) {
- me->uError = QCBOR_ERR_TOO_MANY_CLOSES;
- return;
- } else if(Nesting_GetMajorType(&(me->nesting)) != uMajorType) {
- me->uError = QCBOR_ERR_CLOSE_MISMATCH;
- return;
- }
+ if(CheckDecreaseNesting(me, uMajorType)) {
+ return;
}
-#endif /* QCBOR_DISABLE_ENCODE_USAGE_GUARDS */
+
if(uMajorType == CBOR_MAJOR_NONE_TYPE_OPEN_BSTR) {
uMajorType = CBOR_MAJOR_TYPE_BYTE_STRING;
}
- /* A stack buffer large enough for a CBOR head */
+ /* A stack buffer large enough for a CBOR head (9 bytes) */
UsefulBuf_MAKE_STACK_UB(pBufferForEncodedHead, QCBOR_HEAD_BUFFER_SIZE);
UsefulBufC EncodedHead = QCBOREncode_EncodeHead(pBufferForEncodedHead,
@@ -798,7 +849,7 @@
* this.
*
* See qcbor/qcbor_encode.h
-*/
+ */
void QCBOREncode_OpenMapOrArray(QCBOREncodeContext *me, uint8_t uMajorType)
{
/* Add one item to the nesting level we are in for the new map or array */
@@ -904,20 +955,15 @@
*/
void QCBOREncode_CancelBstrWrap(QCBOREncodeContext *pMe)
{
+ if(CheckDecreaseNesting(pMe, CBOR_MAJOR_TYPE_BYTE_STRING)) {
+ return;
+ }
+
#ifndef QCBOR_DISABLE_ENCODE_USAGE_GUARDS
- if(pMe->uError == QCBOR_SUCCESS) {
- if(!Nesting_IsInNest(&(pMe->nesting))) {
- pMe->uError = QCBOR_ERR_TOO_MANY_CLOSES;
- return;
- } else if(Nesting_GetMajorType(&(pMe->nesting)) != CBOR_MAJOR_TYPE_BYTE_STRING) {
- pMe->uError = QCBOR_ERR_CLOSE_MISMATCH;
- return;
- }
- const size_t uCurrent = UsefulOutBuf_GetEndPosition(&(pMe->OutBuf));
- if(pMe->nesting.pCurrentNesting->uStart != uCurrent) {
- pMe->uError = QCBOR_ERR_CANNOT_CANCEL;
- return;
- }
+ const size_t uCurrent = UsefulOutBuf_GetEndPosition(&(pMe->OutBuf));
+ if(pMe->nesting.pCurrentNesting->uStart != uCurrent) {
+ pMe->uError = QCBOR_ERR_CANNOT_CANCEL;
+ return;
}
/* QCBOREncode_CancelBstrWrap() can't correctly undo
* QCBOREncode_BstrWrapInMap() or QCBOREncode_BstrWrapInMapN(). It
@@ -940,8 +986,9 @@
void QCBOREncode_OpenBytes(QCBOREncodeContext *pMe, UsefulBuf *pPlace)
{
*pPlace = UsefulOutBuf_GetOutPlace(&(pMe->OutBuf));
- if(!UsefulBuf_IsNULL(*pPlace)){
+ if(!UsefulBuf_IsNULL(*pPlace)) {
#ifndef QCBOR_DISABLE_ENCODE_USAGE_GUARDS
+ // TODO: is this right?
uint8_t uMajorType = Nesting_GetMajorType(&(pMe->nesting));
if(uMajorType == CBOR_MAJOR_NONE_TYPE_OPEN_BSTR) {
pMe->uError = QCBOR_ERR_OPEN_BYTE_STRING;
@@ -949,7 +996,7 @@
}
#endif /* QCBOR_DISABLE_ENCODE_USAGE_GUARDS */
- QCBOREncode_OpenMapOrArray(pMe, CBOR_MAJOR_NONE_TYPE_OPEN_BSTR);
+ QCBOREncode_OpenMapOrArray(pMe, CBOR_MAJOR_NONE_TYPE_OPEN_BSTR);
}
}
@@ -972,25 +1019,15 @@
/*
* Public function for closing arrays and maps. See qcbor/qcbor_encode.h
*/
-void QCBOREncode_CloseMapOrArrayIndefiniteLength(QCBOREncodeContext *me, uint8_t uMajorType)
+void QCBOREncode_CloseMapOrArrayIndefiniteLength(QCBOREncodeContext *pMe, uint8_t uMajorType)
{
-#ifndef QCBOR_DISABLE_ENCODE_USAGE_GUARDS
- if(me->uError == QCBOR_SUCCESS) {
- if(!Nesting_IsInNest(&(me->nesting))) {
- me->uError = QCBOR_ERR_TOO_MANY_CLOSES;
- return;
- } else if(Nesting_GetMajorType(&(me->nesting)) != uMajorType) {
- me->uError = QCBOR_ERR_CLOSE_MISMATCH;
- return;
- }
+ if(CheckDecreaseNesting(pMe, uMajorType)) {
+ return;
}
-#else /* QCBOR_DISABLE_ENCODE_USAGE_GUARDS */
- (void) uMajorType;
-#endif /* QCBOR_DISABLE_ENCODE_USAGE_GUARDS */
/* Append the break marker (0xff for both arrays and maps) */
- AppendCBORHead(me, CBOR_MAJOR_NONE_TYPE_SIMPLE_BREAK, CBOR_SIMPLE_BREAK, 0);
- Nesting_Decrease(&(me->nesting));
+ AppendCBORHead(pMe, CBOR_MAJOR_NONE_TYPE_SIMPLE_BREAK, CBOR_SIMPLE_BREAK, 0);
+ Nesting_Decrease(&(pMe->nesting));
}