From 47486a4d8d67c2c9dbe05278ac6e146043a7ba68 Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Tue, 10 Jun 2025 16:34:58 -0400 Subject: [PATCH 1/8] Update OverflowCalculated.ql --- cpp/ql/src/Critical/OverflowCalculated.ql | 26 ++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/Critical/OverflowCalculated.ql b/cpp/ql/src/Critical/OverflowCalculated.ql index 8958da3b22f1..0e11ca18f159 100644 --- a/cpp/ql/src/Critical/OverflowCalculated.ql +++ b/cpp/ql/src/Critical/OverflowCalculated.ql @@ -1,7 +1,8 @@ /** - * @name Buffer not sufficient for string - * @description A buffer allocated using 'malloc' may not have enough space for a string that is being copied into it. The operation can cause a buffer overrun. Make sure that the buffer contains enough room for the string (including the zero terminator). + * @name Buffer overflow from insufficient space or incorrect size calculation + * @description A buffer allocated using 'malloc' may not have enough space for a string being copied into it, or wide character functions may receive incorrect size parameters causing buffer overrun. Make sure that buffers contain enough room for strings (including zero terminator) and that size parameters are correctly calculated. * @kind problem + * @precision medium * @id cpp/overflow-calculated * @problem.severity warning * @security-severity 9.8 @@ -40,6 +41,25 @@ predicate spaceProblem(FunctionCall append, string msg) { ) } +predicate wideCharSizeofProblem(FunctionCall call, string msg) { + exists( + Variable buffer, SizeofExprOperator sizeofOp, ArrayType arrayType + | + // Function call is to wcsftime + call.getTarget().hasGlobalOrStdName("wcsftime") and + // Second argument (count parameter) is a sizeof operation + call.getArgument(1) = sizeofOp and + // The sizeof is applied to a buffer variable + sizeofOp.getExprOperand() = buffer.getAnAccess() and + // The buffer is an array of wchar_t + arrayType = buffer.getType() and + arrayType.getBaseType().hasName("wchar_t") and + msg = + "Using sizeof(" + buffer.getName() + ") passes byte count instead of wchar_t element count to wcsftime. " + + "Use sizeof(" + buffer.getName() + ")/sizeof(wchar_t) or array length instead." + ) +} + from Expr problem, string msg -where spaceProblem(problem, msg) +where spaceProblem(problem, msg) or wideCharSizeofProblem(problem, msg) select problem, msg From 11786cd290ef011ff7085396b3b9710ca2898725 Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Wed, 11 Jun 2025 13:03:27 -0400 Subject: [PATCH 2/8] Add Tests and handle additional scenario: - Pointer case: sizeof(pointer) gives pointer size (e.g., 8 bytes), which is completely wrong --- cpp/ql/src/Critical/OverflowCalculated.ql | 28 +++++--- .../OverflowCalculated.expected | 7 +- .../Critical/OverflowCalculated/tests2.cpp | 69 +++++++++++++++++++ 3 files changed, 94 insertions(+), 10 deletions(-) diff --git a/cpp/ql/src/Critical/OverflowCalculated.ql b/cpp/ql/src/Critical/OverflowCalculated.ql index 0e11ca18f159..3205f04340db 100644 --- a/cpp/ql/src/Critical/OverflowCalculated.ql +++ b/cpp/ql/src/Critical/OverflowCalculated.ql @@ -2,7 +2,6 @@ * @name Buffer overflow from insufficient space or incorrect size calculation * @description A buffer allocated using 'malloc' may not have enough space for a string being copied into it, or wide character functions may receive incorrect size parameters causing buffer overrun. Make sure that buffers contain enough room for strings (including zero terminator) and that size parameters are correctly calculated. * @kind problem - * @precision medium * @id cpp/overflow-calculated * @problem.severity warning * @security-severity 9.8 @@ -43,7 +42,7 @@ predicate spaceProblem(FunctionCall append, string msg) { predicate wideCharSizeofProblem(FunctionCall call, string msg) { exists( - Variable buffer, SizeofExprOperator sizeofOp, ArrayType arrayType + Variable buffer, SizeofExprOperator sizeofOp | // Function call is to wcsftime call.getTarget().hasGlobalOrStdName("wcsftime") and @@ -51,12 +50,25 @@ predicate wideCharSizeofProblem(FunctionCall call, string msg) { call.getArgument(1) = sizeofOp and // The sizeof is applied to a buffer variable sizeofOp.getExprOperand() = buffer.getAnAccess() and - // The buffer is an array of wchar_t - arrayType = buffer.getType() and - arrayType.getBaseType().hasName("wchar_t") and - msg = - "Using sizeof(" + buffer.getName() + ") passes byte count instead of wchar_t element count to wcsftime. " + - "Use sizeof(" + buffer.getName() + ")/sizeof(wchar_t) or array length instead." + ( + // Case 1: Array of wchar_t - sizeof gives bytes instead of element count + exists(ArrayType arrayType | + arrayType = buffer.getType() and + arrayType.getBaseType().hasName("wchar_t") and + msg = + "Using sizeof(" + buffer.getName() + ") passes byte count instead of wchar_t element count to wcsftime. " + + "Use sizeof(" + buffer.getName() + ")/sizeof(wchar_t) or array length instead." + ) + or + // Case 2: Pointer to wchar_t - sizeof gives pointer size, which is completely wrong + exists(PointerType ptrType | + ptrType = buffer.getType() and + ptrType.getBaseType().hasName("wchar_t") and + msg = + "Using sizeof(" + buffer.getName() + ") passes pointer size instead of buffer size to wcsftime. " + + "Pass the actual element count or use a length variable instead." + ) + ) ) } diff --git a/cpp/ql/test/query-tests/Critical/OverflowCalculated/OverflowCalculated.expected b/cpp/ql/test/query-tests/Critical/OverflowCalculated/OverflowCalculated.expected index 8fe99858e1a2..58c5b4aac18c 100644 --- a/cpp/ql/test/query-tests/Critical/OverflowCalculated/OverflowCalculated.expected +++ b/cpp/ql/test/query-tests/Critical/OverflowCalculated/OverflowCalculated.expected @@ -1,2 +1,5 @@ -| tests2.cpp:34:4:34:9 | call to strcat | This buffer only contains enough room for 'str1' (copied on line 33) | -| tests2.cpp:52:4:52:9 | call to strcat | This buffer only contains enough room for 'str1' (copied on line 51) | +| tests2.cpp:48:4:48:9 | call to strcat | This buffer only contains enough room for 'str1' (copied on line 47) | +| tests2.cpp:66:4:66:9 | call to strcat | This buffer only contains enough room for 'str1' (copied on line 65) | +| tests2.cpp:118:4:118:11 | call to wcsftime | Using sizeof(buf) passes byte count instead of wchar_t element count to wcsftime. Use sizeof(buf)/sizeof(wchar_t) or array length instead. | +| tests2.cpp:142:4:142:11 | call to wcsftime | Using sizeof(smallBuf) passes byte count instead of wchar_t element count to wcsftime. Use sizeof(smallBuf)/sizeof(wchar_t) or array length instead. | +| tests2.cpp:151:5:151:12 | call to wcsftime | Using sizeof(dynamicBuf) passes pointer size instead of buffer size to wcsftime. Pass the actual element count or use a length variable instead. | \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Critical/OverflowCalculated/tests2.cpp b/cpp/ql/test/query-tests/Critical/OverflowCalculated/tests2.cpp index 696b566329a3..eb97e44d3226 100644 --- a/cpp/ql/test/query-tests/Critical/OverflowCalculated/tests2.cpp +++ b/cpp/ql/test/query-tests/Critical/OverflowCalculated/tests2.cpp @@ -2,8 +2,22 @@ typedef unsigned int size_t; +// Time structures and functions for wcsftime tests +struct tm { + int tm_sec; + int tm_min; + int tm_hour; + int tm_mday; + int tm_mon; + int tm_year; + int tm_wday; + int tm_yday; + int tm_isdst; +}; + size_t strlen(const char *str); size_t wcslen(const wchar_t *wcs); +size_t wcsftime(wchar_t *strDest, size_t maxsize, const wchar_t *format, const struct tm *timeptr); char *strcpy(char *destination, const char *source); wchar_t *wcscpy(wchar_t *strDestination, const wchar_t *strSource); @@ -95,6 +109,61 @@ void tests2(int case_num) wcscpy(wbuffer, wstr1); wcscat(wbuffer, wstr2); break; + + // wcsftime test cases + case 200: + { + wchar_t buf[80]; + struct tm timeinfo = {0}; + wcsftime(buf, sizeof(buf), L"%Y-%m-%d %H:%M:%S", &timeinfo); // BAD: sizeof(buf) returns bytes, not wchar_t count + break; + } + + case 201: + { + wchar_t buf[80]; + struct tm timeinfo = {0}; + wcsftime(buf, sizeof(buf) / sizeof(wchar_t), L"%Y-%m-%d %H:%M:%S", &timeinfo); // GOOD: correct element count + break; + } + + case 202: + { + wchar_t buf[80]; + struct tm timeinfo = {0}; + wcsftime(buf, 80, L"%Y-%m-%d %H:%M:%S", &timeinfo); // GOOD: direct array length + break; + } + + case 203: + { + wchar_t smallBuf[20]; + struct tm timeinfo = {0}; + wcsftime(smallBuf, sizeof(smallBuf), L"%Y-%m-%d %H:%M:%S", &timeinfo); // BAD: sizeof returns bytes + break; + } + + case 204: + { + wchar_t *dynamicBuf = (wchar_t *)malloc(50 * sizeof(wchar_t)); + struct tm timeinfo = {0}; + if (dynamicBuf) { + wcsftime(dynamicBuf, sizeof(dynamicBuf), L"%Y-%m-%d %H:%M:%S", &timeinfo); // BAD: sizeof on pointer + free(dynamicBuf); + } + break; + } + + case 205: + { + wchar_t *dynamicBuf = (wchar_t *)malloc(50 * sizeof(wchar_t)); + struct tm timeinfo = {0}; + if (dynamicBuf) { + wcsftime(dynamicBuf, 50, L"%Y-%m-%d %H:%M:%S", &timeinfo); // GOOD: correct element count + free(dynamicBuf); + } + break; + } } if (buffer != 0) From 6d91effde9300292eb4336cde112f042ec517c67 Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Wed, 11 Jun 2025 13:10:27 -0400 Subject: [PATCH 3/8] Adds a changenote --- ...2025-06-11-add-overflow-calculated-to-security-extended.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/src/change-notes/2025-06-11-add-overflow-calculated-to-security-extended.md diff --git a/cpp/ql/src/change-notes/2025-06-11-add-overflow-calculated-to-security-extended.md b/cpp/ql/src/change-notes/2025-06-11-add-overflow-calculated-to-security-extended.md new file mode 100644 index 000000000000..42e08302b1e0 --- /dev/null +++ b/cpp/ql/src/change-notes/2025-06-11-add-overflow-calculated-to-security-extended.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* Adds `cpp/overflow-calculated` to the `cpp-security-extended` query suite. From c728c7894c89c35464114974955fdc3d8a2fd754 Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Wed, 11 Jun 2025 13:12:02 -0400 Subject: [PATCH 4/8] * @precision medium --- cpp/ql/src/Critical/OverflowCalculated.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/ql/src/Critical/OverflowCalculated.ql b/cpp/ql/src/Critical/OverflowCalculated.ql index 3205f04340db..8647e36bdadf 100644 --- a/cpp/ql/src/Critical/OverflowCalculated.ql +++ b/cpp/ql/src/Critical/OverflowCalculated.ql @@ -2,6 +2,7 @@ * @name Buffer overflow from insufficient space or incorrect size calculation * @description A buffer allocated using 'malloc' may not have enough space for a string being copied into it, or wide character functions may receive incorrect size parameters causing buffer overrun. Make sure that buffers contain enough room for strings (including zero terminator) and that size parameters are correctly calculated. * @kind problem + * @precision medium * @id cpp/overflow-calculated * @problem.severity warning * @security-severity 9.8 From 31e7790bfe14a786406458f42c59dd70c501bafd Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Thu, 12 Jun 2025 11:22:34 -0400 Subject: [PATCH 5/8] codeql query format --- cpp/ql/src/Critical/OverflowCalculated.ql | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/cpp/ql/src/Critical/OverflowCalculated.ql b/cpp/ql/src/Critical/OverflowCalculated.ql index 8647e36bdadf..4240468d9277 100644 --- a/cpp/ql/src/Critical/OverflowCalculated.ql +++ b/cpp/ql/src/Critical/OverflowCalculated.ql @@ -42,9 +42,7 @@ predicate spaceProblem(FunctionCall append, string msg) { } predicate wideCharSizeofProblem(FunctionCall call, string msg) { - exists( - Variable buffer, SizeofExprOperator sizeofOp - | + exists(Variable buffer, SizeofExprOperator sizeofOp | // Function call is to wcsftime call.getTarget().hasGlobalOrStdName("wcsftime") and // Second argument (count parameter) is a sizeof operation @@ -57,8 +55,9 @@ predicate wideCharSizeofProblem(FunctionCall call, string msg) { arrayType = buffer.getType() and arrayType.getBaseType().hasName("wchar_t") and msg = - "Using sizeof(" + buffer.getName() + ") passes byte count instead of wchar_t element count to wcsftime. " + - "Use sizeof(" + buffer.getName() + ")/sizeof(wchar_t) or array length instead." + "Using sizeof(" + buffer.getName() + + ") passes byte count instead of wchar_t element count to wcsftime. " + "Use sizeof(" + + buffer.getName() + ")/sizeof(wchar_t) or array length instead." ) or // Case 2: Pointer to wchar_t - sizeof gives pointer size, which is completely wrong @@ -66,8 +65,9 @@ predicate wideCharSizeofProblem(FunctionCall call, string msg) { ptrType = buffer.getType() and ptrType.getBaseType().hasName("wchar_t") and msg = - "Using sizeof(" + buffer.getName() + ") passes pointer size instead of buffer size to wcsftime. " + - "Pass the actual element count or use a length variable instead." + "Using sizeof(" + buffer.getName() + + ") passes pointer size instead of buffer size to wcsftime. " + + "Pass the actual element count or use a length variable instead." ) ) ) From 7a72c49ae450f0c62609f9c85180d4de2cdde22d Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Thu, 12 Jun 2025 11:53:21 -0400 Subject: [PATCH 6/8] Update cpp-security-extended.qls.expected --- .../query-suite/cpp-security-extended.qls.expected | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/ql/integration-tests/query-suite/cpp-security-extended.qls.expected b/cpp/ql/integration-tests/query-suite/cpp-security-extended.qls.expected index f014b6d5dc51..e8b41c266850 100644 --- a/cpp/ql/integration-tests/query-suite/cpp-security-extended.qls.expected +++ b/cpp/ql/integration-tests/query-suite/cpp-security-extended.qls.expected @@ -4,6 +4,7 @@ ql/cpp/ql/src/Critical/DoubleFree.ql ql/cpp/ql/src/Critical/IncorrectCheckScanf.ql ql/cpp/ql/src/Critical/MissingCheckScanf.ql ql/cpp/ql/src/Critical/NewFreeMismatch.ql +ql/cpp/ql/src/Critical/OverflowCalculated.ql ql/cpp/ql/src/Critical/OverflowStatic.ql ql/cpp/ql/src/Critical/SizeCheck.ql ql/cpp/ql/src/Critical/SizeCheck2.ql From 02834660214d48a0620f0b337fa0878f14e2dfb4 Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Thu, 12 Jun 2025 12:05:32 -0400 Subject: [PATCH 7/8] Update cpp-security-and-quality.qls.expected --- .../query-suite/cpp-security-and-quality.qls.expected | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/ql/integration-tests/query-suite/cpp-security-and-quality.qls.expected b/cpp/ql/integration-tests/query-suite/cpp-security-and-quality.qls.expected index 9ef67d525cd0..cd31e4c39271 100644 --- a/cpp/ql/integration-tests/query-suite/cpp-security-and-quality.qls.expected +++ b/cpp/ql/integration-tests/query-suite/cpp-security-and-quality.qls.expected @@ -27,6 +27,7 @@ ql/cpp/ql/src/Critical/MissingCheckScanf.ql ql/cpp/ql/src/Critical/NewArrayDeleteMismatch.ql ql/cpp/ql/src/Critical/NewDeleteArrayMismatch.ql ql/cpp/ql/src/Critical/NewFreeMismatch.ql +ql/cpp/ql/src/Critical/OverflowCalculated.ql ql/cpp/ql/src/Critical/OverflowStatic.ql ql/cpp/ql/src/Critical/SizeCheck.ql ql/cpp/ql/src/Critical/SizeCheck2.ql From 34cc7f294eab7dcac48710edce9764e6e29906e1 Mon Sep 17 00:00:00 2001 From: Chad Bentz <1760475+felickz@users.noreply.github.com> Date: Thu, 12 Jun 2025 12:05:54 -0400 Subject: [PATCH 8/8] Update not_included_in_qls.expected --- .../integration-tests/query-suite/not_included_in_qls.expected | 1 - 1 file changed, 1 deletion(-) diff --git a/cpp/ql/integration-tests/query-suite/not_included_in_qls.expected b/cpp/ql/integration-tests/query-suite/not_included_in_qls.expected index 20ffa7c40c0d..9fe56aa00494 100644 --- a/cpp/ql/integration-tests/query-suite/not_included_in_qls.expected +++ b/cpp/ql/integration-tests/query-suite/not_included_in_qls.expected @@ -37,7 +37,6 @@ ql/cpp/ql/src/Critical/MemoryNeverFreed.ql ql/cpp/ql/src/Critical/MissingNegativityTest.ql ql/cpp/ql/src/Critical/MissingNullTest.ql ql/cpp/ql/src/Critical/NotInitialised.ql -ql/cpp/ql/src/Critical/OverflowCalculated.ql ql/cpp/ql/src/Critical/OverflowDestination.ql ql/cpp/ql/src/Critical/ReturnStackAllocatedObject.ql ql/cpp/ql/src/Critical/ReturnValueIgnored.ql