Encode/UB test coverage 100%; fix bugs in UsefulOutBuf_Compare and sorting
UsefulBuf test coverage back up to 100% (filled out coverage for new v2 features)
UsefulOutBuf_Compare() bugs for unequal length comparisons and comparisons off the end of the buffer
Improve UsefulOutBuf magic number check
UsefulOutBuf_OutSubString renamed to UsefulOutBuf_SubString for consistency. Old name still supported.
* Useful test coverage; UsefulOutBuf_Compare fixes
* Improve sort error handling; encode test coverage 100%
* Fix ifdef test fan out
* revert error codes
* Add diverse labels test
---------
Co-authored-by: Laurence Lundblade <lgl@securitytheory.com>
diff --git a/src/UsefulBuf.c b/src/UsefulBuf.c
index 46b66c6..6e99c43 100644
--- a/src/UsefulBuf.c
+++ b/src/UsefulBuf.c
@@ -1,6 +1,6 @@
/*==============================================================================
* Copyright (c) 2016-2018, The Linux Foundation.
- * Copyright (c) 2018-2024, Laurence Lundblade.
+ * Copyright (c) 2018-2025, Laurence Lundblade.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -44,6 +44,9 @@
when who what, where, why
-------- ---- ---------------------------------------------------
+ 02/21/2025 llundblade Improve magic number to detect lack of initialization.
+ 02/21/2025 llundblade Bug fixes to UsefulOutBuf_Compare().
+ 02/21/2025 llundblade Rename to UsefulOutBuf_OutSubString().
08/08/2024 llundblade Add UsefulOutBuf_SubString().
21/05/2024 llundblade Comment formatting and some code tidiness.
1/7/2024 llundblade Add UsefulInputBuf_Compare().
@@ -246,11 +249,6 @@
*/
void UsefulOutBuf_InsertUsefulBuf(UsefulOutBuf *pMe, UsefulBufC NewData, size_t uInsertionPos)
{
- if(pMe->err) {
- /* Already in error state. */
- return;
- }
-
/* 0. Sanity check the UsefulOutBuf structure
* A "counter measure". If magic number is not the right number it
* probably means pMe was not initialized or it was corrupted. Attackers
@@ -262,6 +260,11 @@
return; /* Magic number is wrong due to uninitalization or corrption */
}
+ if(pMe->err) {
+ /* Already in error state. */
+ return;
+ }
+
/* Make sure valid data is less than buffer size. This would only occur
* if there was corruption of me, but it is also part of the checks to
* be sure there is no pointer arithmatic under/overflow.
@@ -358,11 +361,6 @@
* rarely used.
*/
- if(pMe->err) {
- /* Already in error state. */
- return;
- }
-
/* 0. Sanity check the UsefulOutBuf structure
*
* A "counter measure". If magic number is not the right number it
@@ -375,6 +373,11 @@
return; /* Magic number is wrong due to uninitalization or corrption */
}
+ if(pMe->err) {
+ /* Already in error state. */
+ return;
+ }
+
/* Make sure valid data is less than buffer size. This would only
* occur if there was corruption of me, but it is also part of the
* checks to be sure there is no pointer arithmatic
@@ -409,12 +412,12 @@
*/
UsefulBufC UsefulOutBuf_OutUBuf(UsefulOutBuf *pMe)
{
- if(pMe->err) {
+ if(pMe->magic != USEFUL_OUT_BUF_MAGIC) {
+ pMe->err = 1;
return NULLUsefulBufC;
}
- if(pMe->magic != USEFUL_OUT_BUF_MAGIC) {
- pMe->err = 1;
+ if(pMe->err) {
return NULLUsefulBufC;
}
@@ -442,9 +445,9 @@
*
* Code Reviewers: THIS FUNCTION DOES POINTER MATH
*/
-UsefulBufC UsefulOutBuf_SubString(UsefulOutBuf *pMe,
- const size_t uStart,
- const size_t uLen)
+UsefulBufC UsefulOutBuf_OutSubString(UsefulOutBuf *pMe,
+ const size_t uStart,
+ const size_t uLen)
{
const UsefulBufC Tmp = UsefulOutBuf_OutUBuf(pMe);
@@ -544,34 +547,53 @@
const uint8_t *pEnd;
const uint8_t *p1;
const uint8_t *p2;
+ const uint8_t *p1Start;
+ const uint8_t *p2Start;
const uint8_t *p1End;
const uint8_t *p2End;
int uComparison;
+ size_t uComparedLen1;
+ size_t uComparedLen2;
- pBase = pMe->UB.ptr;
- pEnd = (const uint8_t *)pBase + pMe->data_len;
- p1 = pBase + uStart1;
- p2 = pBase + uStart2;
- p1End = p1 + uLen1;
- p2End = p2 + uLen2;
+ pBase = pMe->UB.ptr;
+ pEnd = (const uint8_t *)pBase + pMe->data_len;
+ p1Start = pBase + uStart1;
+ p2Start = pBase + uStart2;
+ p1End = p1Start + uLen1;
+ p2End = p2Start + uLen2;
uComparison = 0;
- while(p1 < pEnd && p2 < pEnd && p1 < p1End && p2 < p2End) {
+ for(p1 = p1Start, p2 = p2Start;
+ p1 < pEnd && p2 < pEnd && p1 < p1End && p2 < p2End;
+ p1++, p2++) {
uComparison = *p2 - *p1;
if(uComparison != 0) {
- break;;
+ break;
}
- p1++;
- p2++;
}
- if(uComparison == 0 && p1 != p1End && p2 != p2End) {
- if(uLen1 > uLen2) {
+ /* Loop might have terminated because strings were off
+ * the end of the buffer. Compute actual lengths compared.
+ */
+ uComparedLen1 = uLen1;
+ if(p1 >= pEnd) {
+ uComparedLen1 = (size_t)(p1 - p1Start);
+ }
+ uComparedLen2 = uLen2;
+ if(p2 >= pEnd) {
+ uComparedLen2 = (size_t)(p2 - p2Start);
+ }
+
+ if(uComparison == 0) {
+ /* All bytes were equal, now check the lengths */
+ if(uComparedLen2 > uComparedLen1) {
+ /* string 1 is a substring of string 2 */
uComparison = 1;
- } else if(uLen2 < uLen1){
+ } else if(uComparedLen1 > uComparedLen2) {
+ /* string 2 is a substring of string 1 */
uComparison = -1;
- } else {
- return 0;
+ } else {
+ /* do nothing, uComparison already is 0 */
}
}
diff --git a/src/qcbor_main_encode.c b/src/qcbor_main_encode.c
index f2f65f4..22d537d 100644
--- a/src/qcbor_main_encode.c
+++ b/src/qcbor_main_encode.c
@@ -839,10 +839,10 @@
* change.
*/
static QCBORError
-QCBOREncodePriv_DecodeHead(UsefulInputBuf *pUInBuf,
- int *pnMajorType,
- uint64_t *puArgument,
- int *pnAdditionalInfo)
+QCBOREncode_Private_DecodeHead(UsefulInputBuf *pUInBuf,
+ int *pnMajorType,
+ uint64_t *puArgument,
+ int *pnAdditionalInfo)
{
QCBORError uReturn;
@@ -899,66 +899,94 @@
*
* @param[in] pInBuf UsefulInputBuf from which to consume item.
*
- * Recursive, but stack usage is light and encoding depth limit
+ * @retval QCBOR_SUCCESS The next item has been consumed successfully.
+ *
+ * @retval QCBOR_ERR_HIT_END The next item is a break or has hit the end.
+ *
+ * @retval a QCOR error code indicating some decode failure
+ *
+ * Recursive, but stack usage is light and encoding depth limited.
*/
static QCBORError
-QCBOR_Private_ConsumeNext(UsefulInputBuf *pInBuf)
+QCBOREncode_Private_ConsumeNext(UsefulInputBuf *pInBuf)
{
- int nMajor;
- uint64_t uArgument;
- int nAdditional;
- uint16_t uItemCount;
- uint16_t uMul;
- uint16_t i;
+ int nMajor;
+ uint64_t uArgument;
+ int nAdditional;
+ uint16_t uItemCountTotal;
+ uint16_t uMul;
+ uint16_t uItemCounter;
QCBORError uCBORError;
- uCBORError = QCBOREncodePriv_DecodeHead(pInBuf, &nMajor, &uArgument, &nAdditional);
+ uCBORError = QCBOREncode_Private_DecodeHead(pInBuf, &nMajor, &uArgument, &nAdditional);
if(uCBORError != QCBOR_SUCCESS) {
- return uCBORError;
+ goto Done;
}
uMul = 1;
+ /* This intentionally doesn't check for a some not-well-formed
+ * conditions such as the wrong type in an indefinite length
+ * string chunk and the disallowed forms of the simple type.
+ * These conditions should never occur because this only decodes
+ * what was encoded by QCBOR. This saves object code.
+ */
+
switch(nMajor) {
case CBOR_MAJOR_TYPE_POSITIVE_INT: /* Major type 0 */
case CBOR_MAJOR_TYPE_NEGATIVE_INT: /* Major type 1 */
break;
- case CBOR_MAJOR_TYPE_SIMPLE:
- return uArgument == CBOR_SIMPLE_BREAK ? 1 : 0;
- break;
-
- case CBOR_MAJOR_TYPE_BYTE_STRING:
- case CBOR_MAJOR_TYPE_TEXT_STRING:
+ case CBOR_MAJOR_TYPE_BYTE_STRING: /* Major type 2 */
+ case CBOR_MAJOR_TYPE_TEXT_STRING: /* Major type 3 */
if(nAdditional == LEN_IS_INDEFINITE) {
- /* Segments of indefinite length */
- while(QCBOR_Private_ConsumeNext(pInBuf) == 0);
+ /* Chunks of indefinite length string */
+ do {
+ uCBORError = QCBOREncode_Private_ConsumeNext(pInBuf);
+ } while(uCBORError == QCBOR_SUCCESS);
+ if(uCBORError != QCBOR_ERR_HIT_END) {
+ return uCBORError;
+ }
+ uCBORError = QCBOR_SUCCESS;
+ } else {
+ /* The bytes of a definite length string */
+ (void)UsefulInputBuf_GetBytes(pInBuf, uArgument);
}
- (void)UsefulInputBuf_GetBytes(pInBuf, uArgument);
break;
- case CBOR_MAJOR_TYPE_TAG:
- QCBOR_Private_ConsumeNext(pInBuf);
- break;
-
- case CBOR_MAJOR_TYPE_MAP:
+ case CBOR_MAJOR_TYPE_MAP: /* Major type 5 */
uMul = 2;
/* Fallthrough */
- case CBOR_MAJOR_TYPE_ARRAY:
- uItemCount = (uint16_t)uArgument * uMul;
+ case CBOR_MAJOR_TYPE_ARRAY: /* Major type 4 */
+ /* Cast is safe because this is decoding CBOR that was created
+ * by QCBOR and because QCBOREncode_Private_ConsumeNext() can
+ * never read off the end. */
+ uItemCountTotal = (uint16_t)uArgument * uMul;
if(nAdditional == LEN_IS_INDEFINITE) {
- uItemCount = UINT16_MAX;
+ uItemCountTotal = UINT16_MAX;
}
- for(i = uItemCount; i > 0; i--) {
- if(QCBOR_Private_ConsumeNext(pInBuf)) {
- /* End of indefinite length */
+ for(uItemCounter = uItemCountTotal; uItemCounter > 0; uItemCounter--) {
+ uCBORError = QCBOREncode_Private_ConsumeNext(pInBuf);
+ if(uCBORError != QCBOR_SUCCESS) {
break;
}
}
+ if(nAdditional == LEN_IS_INDEFINITE && uCBORError == QCBOR_ERR_HIT_END) {
+ uCBORError = QCBOR_SUCCESS;
+ }
+ break;
+
+ case CBOR_MAJOR_TYPE_TAG: /* Major type 6 */
+ uCBORError = QCBOREncode_Private_ConsumeNext(pInBuf);
+ break;
+
+ case CBOR_MAJOR_TYPE_SIMPLE: /* Major type 7 */
+ return uArgument == CBOR_SIMPLE_BREAK ? QCBOR_ERR_HIT_END : QCBOR_SUCCESS;
break;
}
- return QCBOR_SUCCESS;
+Done:
+ return uCBORError;
}
@@ -999,23 +1027,26 @@
UsefulInputBuf_Init(&InBuf, EncodedMapBytes);
/* This is always used on maps, so consume two, the label and the value */
- uCBORError = QCBOR_Private_ConsumeNext(&InBuf);
- if(uCBORError) {
+ uCBORError = QCBOREncode_Private_ConsumeNext(&InBuf);
+ if(uCBORError != QCBOR_SUCCESS) {
+ pMe->uError = (uint8_t)uCBORError;
+ Result.uLabelLen = 0;
return Result;
}
/* Cast is safe because this is QCBOR which limits sizes to UINT32_MAX */
Result.uLabelLen = (uint32_t)UsefulInputBuf_Tell(&InBuf);
- uCBORError = QCBOR_Private_ConsumeNext(&InBuf);
- if(uCBORError) {
+ uCBORError = QCBOREncode_Private_ConsumeNext(&InBuf);
+ if(uCBORError != QCBOR_SUCCESS) {
+ pMe->uError = (uint8_t)uCBORError;
Result.uLabelLen = 0;
return Result;
}
+ /* Cast is safe because this is QCBOR which limits sizes to UINT32_MAX */
Result.uItemLen = (uint32_t)UsefulInputBuf_Tell(&InBuf);
- /* Cast is safe because this is QCBOR which limits sizes to UINT32_MAX */
return Result;
}
@@ -1093,7 +1124,7 @@
}
uStart2 = uStart2 + Lens2.uItemLen;
}
- } while(bSwapped);
+ } while(bSwapped && pMe->uError == QCBOR_SUCCESS);
}
@@ -1331,5 +1362,5 @@
const size_t uEnd = QCBOREncode_Tell(pMe);
- return UsefulOutBuf_SubString(&(pMe->OutBuf), uStart, uEnd - uStart);
+ return UsefulOutBuf_OutSubString(&(pMe->OutBuf), uStart, uEnd - uStart);
}
diff --git a/src/qcbor_number_encode.c b/src/qcbor_number_encode.c
index e23c2d3..269d323 100644
--- a/src/qcbor_number_encode.c
+++ b/src/qcbor_number_encode.c
@@ -1,6 +1,6 @@
/* ===========================================================================
* Copyright (c) 2016-2018, The Linux Foundation.
- * Copyright (c) 2018-2024, Laurence Lundblade.
+ * Copyright (c) 2018-2025, Laurence Lundblade.
* Copyright (c) 2021, Arm Limited.
* All rights reserved.
*
@@ -226,7 +226,7 @@
/**
- * @brief Is there a carry when you subtract 1 from the BigNumber.
+ * @brief Is there a carry when you subtract 1 from the BigNumber?
*
* @param[in] BigNumber Big number to check for carry.
*
@@ -331,9 +331,9 @@
* @return Big number with no leading zeros.
*
* If the big number is all zeros, this returns a big number that is
- * one zero rather than the empty string.
+ * a single zero rather than the empty string.
*
- * RFC 8949 3.4.3 does not explicitly decoders MUST handle the empty
+ * RFC 8949 3.4.3 does not explicitly say decoders MUST handle the empty
* string, but does say decoders MUST handle leading zeros. So
* Postel's Law is applied here and 0 is not encoded as an empty
* string.