Skip to content

Enhance cpp/overflow-calculated - detect out-of-bounds write caused by passing the buffer size in bytes (using sizeof) instead of the number of elements to wcsftime, allowing the function to overrun the allocated buffer. #19722

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

@felickz felickz Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hand rolled this, security-and-quality, and not_included_in

ql/cpp/ql/src/Critical/OverflowStatic.ql
ql/cpp/ql/src/Critical/SizeCheck.ql
ql/cpp/ql/src/Critical/SizeCheck2.ql
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
39 changes: 36 additions & 3 deletions cpp/ql/src/Critical/OverflowCalculated.ql
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor Author

@felickz felickz Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will add to security-extended suite. Review findings in DCA to ensure FP rate is low. FN's have not been evaluated by me - only added coverage for the above scenario only.

* @id cpp/overflow-calculated
* @problem.severity warning
* @security-severity 9.8
Expand Down Expand Up @@ -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
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: newQuery
Copy link
Contributor Author

@felickz felickz Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This query did exist previously but did not have any precision so it was not included in any suite.

Also enhanced the query to find additional scenarios - what would be best category?

---
* Adds `cpp/overflow-calculated` to the `cpp-security-extended` query suite.
Original file line number Diff line number Diff line change
@@ -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. |
69 changes: 69 additions & 0 deletions cpp/ql/test/query-tests/Critical/OverflowCalculated/tests2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down