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 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 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 diff --git a/cpp/ql/src/Critical/OverflowCalculated.ql b/cpp/ql/src/Critical/OverflowCalculated.ql index 8958da3b22f1..4240468d9277 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,38 @@ predicate spaceProblem(FunctionCall append, string msg) { ) } +predicate wideCharSizeofProblem(FunctionCall call, string msg) { + exists(Variable buffer, SizeofExprOperator sizeofOp | + // 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 + ( + // 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." + ) + ) + ) +} + from Expr problem, string msg -where spaceProblem(problem, msg) +where spaceProblem(problem, msg) or wideCharSizeofProblem(problem, msg) select problem, msg 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. 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)