From ac99201c0ebde616279214e600e49fdf506f7469 Mon Sep 17 00:00:00 2001 From: David Steele Date: Sun, 2 Oct 2022 17:32:48 -1000 Subject: [PATCH] Add bufferSize to cvtUInt64FromVarInt128() to further limit reads. The current call site, manifestFileUnpack(), does not know the total buffer size but the buffer has always been maintained in memory so there should be no corruption. However, there are upcoming use cases where the buffer will be read from IO, the buffer size will be known, and additional sanity checking on buffer overruns will be valuable. Also rename params to align better with cvtUInt64ToVarInt128(). --- src/common/type/convert.c | 27 ++++++++++++++---------- src/common/type/convert.h | 2 +- src/info/manifest.c | 20 +++++++++--------- test/src/module/common/typeConvertTest.c | 9 +++++--- 4 files changed, 33 insertions(+), 25 deletions(-) diff --git a/src/common/type/convert.c b/src/common/type/convert.c index c558b0116..3e5015718 100644 --- a/src/common/type/convert.c +++ b/src/common/type/convert.c @@ -603,30 +603,35 @@ cvtUInt64ToVarInt128(uint64_t value, uint8_t *const buffer, size_t *const buffer } uint64_t -cvtUInt64FromVarInt128(const uint8_t *const value, size_t *const valuePos) +cvtUInt64FromVarInt128(const uint8_t *const buffer, size_t *const bufferPos, const size_t bufferSize) { FUNCTION_TEST_BEGIN(); - FUNCTION_TEST_PARAM_P(VOID, value); - FUNCTION_TEST_PARAM_P(UINT64, valuePos); + FUNCTION_TEST_PARAM_P(VOID, buffer); + FUNCTION_TEST_PARAM_P(SIZE, bufferPos); + FUNCTION_TEST_PARAM(SIZE, bufferSize); FUNCTION_TEST_END(); - ASSERT(value != NULL); - ASSERT(valuePos != NULL); + ASSERT(buffer != NULL); + ASSERT(bufferPos != NULL); // Decode all bytes uint64_t result = 0; uint8_t byte; - for (unsigned int valueIdx = 0; valueIdx < CVT_VARINT128_BUFFER_SIZE; valueIdx++) + for (unsigned int bufferIdx = 0; bufferIdx < CVT_VARINT128_BUFFER_SIZE; bufferIdx++) { + // Error if the buffer position is beyond the buffer size + if (*bufferPos >= bufferSize) + THROW(FormatError, "buffer position is beyond buffer size"); + // Get the next encoded byte - byte = value[*valuePos]; + byte = buffer[*bufferPos]; // Shift the lower order 7 encoded bits into the uint64 in reverse order - result |= (uint64_t)(byte & 0x7f) << (7 * valueIdx); + result |= (uint64_t)(byte & 0x7f) << (7 * bufferIdx); - // Increment value position to indicate that the byte has been processed - (*valuePos)++; + // Increment buffer position to indicate that the byte has been processed + (*bufferPos)++; // Done if the high order bit is not set to indicate more data if (byte < 0x80) @@ -634,7 +639,7 @@ cvtUInt64FromVarInt128(const uint8_t *const value, size_t *const valuePos) } // By this point all bytes should have been read so error if this is not the case. This could be due to a coding error or - // corrupton in the data stream. + // corruption in the data stream. if (byte >= 0x80) THROW(FormatError, "unterminated varint-128 integer"); diff --git a/src/common/type/convert.h b/src/common/type/convert.h index bc9e8d3f7..4d371ad09 100644 --- a/src/common/type/convert.h +++ b/src/common/type/convert.h @@ -117,7 +117,7 @@ cvtZSubNToUInt64(const char *const value, const size_t offset, const size_t size // Convert uint64 to base-128 varint and vice versa void cvtUInt64ToVarInt128(uint64_t value, uint8_t *buffer, size_t *bufferPos, size_t bufferSize); -uint64_t cvtUInt64FromVarInt128(const uint8_t *value, size_t *valuePos); +uint64_t cvtUInt64FromVarInt128(const uint8_t *buffer, size_t *bufferPos, size_t bufferSize); // Convert boolean to zero-terminated string. Use cvtBoolToConstZ() whenever possible since it is more efficient. size_t cvtBoolToZ(bool value, char *buffer, size_t bufferSize); diff --git a/src/info/manifest.c b/src/info/manifest.c index 6ce767f6d..3d5ebf2f4 100644 --- a/src/info/manifest.c +++ b/src/info/manifest.c @@ -246,14 +246,14 @@ manifestFileUnpack(const Manifest *const manifest, const ManifestFilePack *const bufferPos += sizeof(StringPub) + strSize(result.name) + 1; // Flags - const uint64_t flag = cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos); + const uint64_t flag = cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX); // Size - result.size = cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos); + result.size = cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX); // Timestamp result.timestamp = - manifestPackBaseTime - (time_t)cvtInt64FromZigZag(cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos)); + manifestPackBaseTime - (time_t)cvtInt64FromZigZag(cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX)); // Checksum page result.checksumPage = flag & (1 << manifestFilePackFlagChecksumPage) ? true : false; @@ -264,33 +264,33 @@ manifestFileUnpack(const Manifest *const manifest, const ManifestFilePack *const // Reference if (flag & (1 << manifestFilePackFlagReference)) - result.reference = (const String *)(uintptr_t)cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos); + result.reference = (const String *)(uintptr_t)cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX); // Mode if (flag & (1 << manifestFilePackFlagMode)) - result.mode = (mode_t)cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos); + result.mode = (mode_t)cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX); else result.mode = manifest->fileModeDefault; // User/group if (flag & (1 << manifestFilePackFlagUser)) - result.user = (const String *)(uintptr_t)cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos); + result.user = (const String *)(uintptr_t)cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX); else if (!(flag & (1 << manifestFilePackFlagUserNull))) result.user = manifest->fileUserDefault; if (flag & (1 << manifestFilePackFlagGroup)) - result.group = (const String *)(uintptr_t)cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos); + result.group = (const String *)(uintptr_t)cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX); else if (!(flag & (1 << manifestFilePackFlagGroupNull))) result.group = manifest->fileGroupDefault; // Repo size - result.sizeRepo = cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos); + result.sizeRepo = cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX); // Bundle if (flag & (1 << manifestFilePackFlagBundle)) { - result.bundleId = cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos); - result.bundleOffset = cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos); + result.bundleId = cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX); + result.bundleOffset = cvtUInt64FromVarInt128((const uint8_t *)filePack, &bufferPos, UINT_MAX); } // Checksum page error diff --git a/test/src/module/common/typeConvertTest.c b/test/src/module/common/typeConvertTest.c index 9925924ba..9e3531a74 100644 --- a/test/src/module/common/typeConvertTest.c +++ b/test/src/module/common/typeConvertTest.c @@ -224,7 +224,10 @@ testRun(void) TEST_RESULT_UINT(bufferPos, 10, "check buffer position"); bufferPos = 0; - TEST_RESULT_UINT(cvtUInt64FromVarInt128(buffer, &bufferPos), 0xFFFFFFFFFFFFFFFF, "to uint64"); + TEST_ERROR(cvtUInt64FromVarInt128(buffer, &bufferPos, 1), FormatError, "buffer position is beyond buffer size"); + + bufferPos = 0; + TEST_RESULT_UINT(cvtUInt64FromVarInt128(buffer, &bufferPos, sizeof(buffer)), 0xFFFFFFFFFFFFFFFF, "to uint64"); TEST_RESULT_UINT(bufferPos, 10, "check buffer position"); bufferPos = 0; @@ -232,13 +235,13 @@ testRun(void) TEST_RESULT_UINT(bufferPos, 1, "check buffer position"); bufferPos = 0; - TEST_RESULT_UINT(cvtUInt64FromVarInt128(buffer, &bufferPos), 0, "to uint64"); + TEST_RESULT_UINT(cvtUInt64FromVarInt128(buffer, &bufferPos, sizeof(buffer)), 0, "to uint64"); TEST_RESULT_UINT(bufferPos, 1, "check buffer position"); uint8_t buffer2[CVT_VARINT128_BUFFER_SIZE + 1]; memset(buffer2, 0xFF, sizeof(buffer2)); bufferPos = 0; - TEST_ERROR(cvtUInt64FromVarInt128(buffer2, &bufferPos), FormatError, "unterminated varint-128 integer"); + TEST_ERROR(cvtUInt64FromVarInt128(buffer2, &bufferPos, sizeof(buffer)), FormatError, "unterminated varint-128 integer"); } FUNCTION_HARNESS_RETURN_VOID();