From 554aea1bb816029bb1b699ea305114d5c8242601 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Tue, 9 Aug 2022 14:52:45 +0200 Subject: [PATCH 1/5] New strcpy-variant in StrncpyFlippedArgs test Added wcsxfrm_l, which is not currently caught by the query, meaning that in this case a successful test implies missing functionality. --- .../StrncpyFlippedArgs.expected | 22 +++++++++---------- .../StrncpyFlippedArgs/test.cpp | 16 ++++++++++++++ 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected index 88a953b0196..afefbe20256 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected @@ -1,18 +1,18 @@ | test.c:22:2:22:8 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | | test.c:33:2:33:8 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | -| test.cpp:19:2:19:8 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | -| test.cpp:20:2:20:8 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | | test.cpp:21:2:21:8 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | -| test.cpp:30:2:30:8 | call to wcsncpy | Potentially unsafe call to wcsncpy; third argument should be size of destination. | +| test.cpp:22:2:22:8 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | +| test.cpp:23:2:23:8 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | | test.cpp:32:2:32:8 | call to wcsncpy | Potentially unsafe call to wcsncpy; third argument should be size of destination. | -| test.cpp:33:2:33:8 | call to wcsncpy | Potentially unsafe call to wcsncpy; third argument should be size of destination. | | test.cpp:34:2:34:8 | call to wcsncpy | Potentially unsafe call to wcsncpy; third argument should be size of destination. | | test.cpp:35:2:35:8 | call to wcsncpy | Potentially unsafe call to wcsncpy; third argument should be size of destination. | -| test.cpp:45:2:45:9 | call to strcpy_s | Potentially unsafe call to strcpy_s; second argument should be size of destination. | -| test.cpp:46:2:46:9 | call to strcpy_s | Potentially unsafe call to strcpy_s; second argument should be size of destination. | +| test.cpp:36:2:36:8 | call to wcsncpy | Potentially unsafe call to wcsncpy; third argument should be size of destination. | +| test.cpp:37:2:37:8 | call to wcsncpy | Potentially unsafe call to wcsncpy; third argument should be size of destination. | | test.cpp:47:2:47:9 | call to strcpy_s | Potentially unsafe call to strcpy_s; second argument should be size of destination. | -| test.cpp:60:3:60:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | -| test.cpp:63:3:63:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | -| test.cpp:68:2:68:8 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | -| test.cpp:79:3:79:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | -| test.cpp:82:3:82:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | +| test.cpp:48:2:48:9 | call to strcpy_s | Potentially unsafe call to strcpy_s; second argument should be size of destination. | +| test.cpp:49:2:49:9 | call to strcpy_s | Potentially unsafe call to strcpy_s; second argument should be size of destination. | +| test.cpp:62:3:62:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | +| test.cpp:65:3:65:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | +| test.cpp:70:2:70:8 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | +| test.cpp:81:3:81:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | +| test.cpp:84:3:84:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/test.cpp index b31e8762467..fac671d70dc 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/test.cpp @@ -1,9 +1,11 @@ typedef unsigned int size_t; typedef unsigned int errno_t; +typedef void *locale_t; char *strncpy(char *__restrict destination, const char *__restrict source, size_t num); wchar_t *wcsncpy(wchar_t *__restrict destination, const wchar_t *__restrict source, size_t num); +size_t wcsxfrm_l(wchar_t *ws1, const wchar_t *ws2, size_t n, locale_t locale); errno_t strcpy_s(char *strDestination, size_t numberOfElements, const char *strSource); size_t strlen(const char *str); @@ -93,3 +95,17 @@ void test8(char x[], char y[]) { // that it will be a false positive if we report it. strncpy(x, y, 32); } + +void test9() +{ + wchar_t buf1[10]; + wchar_t buf2[20]; + const wchar_t *str = L"01234567890123456789"; + + wcsxfrm_l(buf1, str, sizeof(buf1), nullptr); // (bad, but not a strncpyflippedargs bug) + wcsxfrm_l(buf1, str, sizeof(buf1) / sizeof(wchar_t), nullptr); + wcsxfrm_l(buf1, str, wcslen(str), nullptr); // BAD + wcsxfrm_l(buf1, str, wcslen(str) + 1, nullptr); // BAD + wcsxfrm_l(buf1, buf2, sizeof(buf2), nullptr); // BAD + wcsxfrm_l(buf1, buf2, sizeof(buf2) / sizeof(wchar_t), nullptr); // BAD [NOT DETECTED] +} From df419003ad6b365616621579e282d6d0590edfd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Tue, 9 Aug 2022 15:59:41 +0200 Subject: [PATCH 2/5] Use Strcpy.qll in StrncpyFlippedArgs.ql As a result, the query gets access to more types of strncpy-like functions, as demonstrated by test.cpp, which now "fails" (i.e. works) for the new test cases instroduced in the previous commit. --- .../Memory Management/StrncpyFlippedArgs.ql | 34 +++---------------- .../StrncpyFlippedArgs/test.cpp | 6 ++-- 2 files changed, 8 insertions(+), 32 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgs.ql b/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgs.ql index f7eca2304b3..1c38305f8a6 100644 --- a/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgs.ql +++ b/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgs.ql @@ -18,6 +18,7 @@ import cpp import Buffer private import semmle.code.cpp.valuenumbering.GlobalValueNumbering +private import semmle.code.cpp.models.implementations.Strcpy predicate isSizePlus(Expr e, BufferSizeExpr baseSize, int plus) { // baseSize @@ -41,33 +42,6 @@ predicate isSizePlus(Expr e, BufferSizeExpr baseSize, int plus) { ) } -predicate strncpyFunction(Function f, int argDest, int argSrc, int argLimit) { - exists(string name | name = f.getName() | - name = - [ - "strcpy_s", // strcpy_s(dst, max_amount, src) - "wcscpy_s", // wcscpy_s(dst, max_amount, src) - "_mbscpy_s" // _mbscpy_s(dst, max_amount, src) - ] and - argDest = 0 and - argSrc = 2 and - argLimit = 1 - or - name = - [ - "strncpy", // strncpy(dst, src, max_amount) - "strncpy_l", // strncpy_l(dst, src, max_amount, locale) - "wcsncpy", // wcsncpy(dst, src, max_amount) - "_wcsncpy_l", // _wcsncpy_l(dst, src, max_amount, locale) - "_mbsncpy", // _mbsncpy(dst, src, max_amount) - "_mbsncpy_l" // _mbsncpy_l(dst, src, max_amount, locale) - ] and - argDest = 0 and - argSrc = 1 and - argLimit = 2 - ) -} - string nthString(int num) { num = 0 and result = "first" @@ -96,11 +70,13 @@ int arrayExprFixedSize(Expr e) { } from - Function f, FunctionCall fc, int argDest, int argSrc, int argLimit, int charSize, Access copyDest, + StrcpyFunction f, FunctionCall fc, int argDest, int argSrc, int argLimit, int charSize, Access copyDest, Access copySource, string name, string nth where f = fc.getTarget() and - strncpyFunction(f, argDest, argSrc, argLimit) and + argDest = f.getParamDest() and + argSrc = f.getParamSrc() and + argLimit = f.getParamSize() and copyDest = fc.getArgument(argDest) and copySource = fc.getArgument(argSrc) and // Some of the functions operate on a larger char type, like `wchar_t`, so we diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/test.cpp index fac671d70dc..ad2e39b748e 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/test.cpp @@ -102,10 +102,10 @@ void test9() wchar_t buf2[20]; const wchar_t *str = L"01234567890123456789"; - wcsxfrm_l(buf1, str, sizeof(buf1), nullptr); // (bad, but not a strncpyflippedargs bug) - wcsxfrm_l(buf1, str, sizeof(buf1) / sizeof(wchar_t), nullptr); + wcsxfrm_l(buf1, str, sizeof(buf1), nullptr); // BAD (but not a StrncpyFlippedArgs bug) + wcsxfrm_l(buf1, str, sizeof(buf1) / sizeof(wchar_t), nullptr); // GOOD wcsxfrm_l(buf1, str, wcslen(str), nullptr); // BAD wcsxfrm_l(buf1, str, wcslen(str) + 1, nullptr); // BAD wcsxfrm_l(buf1, buf2, sizeof(buf2), nullptr); // BAD - wcsxfrm_l(buf1, buf2, sizeof(buf2) / sizeof(wchar_t), nullptr); // BAD [NOT DETECTED] + wcsxfrm_l(buf1, buf2, sizeof(buf2) / sizeof(wchar_t), nullptr); // BAD } From 8e60a4a478195a237465c4212913a4f16639be83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Tue, 9 Aug 2022 17:53:04 +0200 Subject: [PATCH 3/5] Update StrncpyFlippedArgs.expected Add output lines for the newly implemented test case, test.cpp/test9(). --- .../StrncpyFlippedArgs/StrncpyFlippedArgs.expected | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected index afefbe20256..1fe46acbdde 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected @@ -16,3 +16,8 @@ | test.cpp:70:2:70:8 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | | test.cpp:81:3:81:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | | test.cpp:84:3:84:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | +| test.cpp:105:2:105:10 | call to wcsxfrm_l | Potentially unsafe call to wcsxfrm_l; third argument should be size of destination. | +| test.cpp:107:2:107:10 | call to wcsxfrm_l | Potentially unsafe call to wcsxfrm_l; third argument should be size of destination. | +| test.cpp:108:2:108:10 | call to wcsxfrm_l | Potentially unsafe call to wcsxfrm_l; third argument should be size of destination. | +| test.cpp:109:2:109:10 | call to wcsxfrm_l | Potentially unsafe call to wcsxfrm_l; third argument should be size of destination. | +| test.cpp:110:2:110:10 | call to wcsxfrm_l | Potentially unsafe call to wcsxfrm_l; third argument should be size of destination. | From 05f4f98aa0e940cf9c16c31c43d0d3de55bbb948 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Wed, 10 Aug 2022 13:25:45 +0200 Subject: [PATCH 4/5] Add change note --- .../2021-08-10-use-strcpyfunction-in-bad-strncpy-size.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/src/change-notes/2021-08-10-use-strcpyfunction-in-bad-strncpy-size.md diff --git a/cpp/ql/src/change-notes/2021-08-10-use-strcpyfunction-in-bad-strncpy-size.md b/cpp/ql/src/change-notes/2021-08-10-use-strcpyfunction-in-bad-strncpy-size.md new file mode 100644 index 00000000000..3468fec4c8d --- /dev/null +++ b/cpp/ql/src/change-notes/2021-08-10-use-strcpyfunction-in-bad-strncpy-size.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The query `cpp/bad-strncpy-size` now covers more `strncpy`-like functions than before, including `strxfrm`(`_l`), `wcsxfrm`(`_l`), and `stpncpy`. Users of this query may see an increase in results. From 60f40493882f3151304d4adafab701d2727d8cb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Wed, 10 Aug 2022 14:14:42 +0200 Subject: [PATCH 5/5] Re-autoformat StrncpyFlippedArgs.ql --- .../src/Likely Bugs/Memory Management/StrncpyFlippedArgs.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgs.ql b/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgs.ql index 1c38305f8a6..a3a2c5b83be 100644 --- a/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgs.ql +++ b/cpp/ql/src/Likely Bugs/Memory Management/StrncpyFlippedArgs.ql @@ -70,8 +70,8 @@ int arrayExprFixedSize(Expr e) { } from - StrcpyFunction f, FunctionCall fc, int argDest, int argSrc, int argLimit, int charSize, Access copyDest, - Access copySource, string name, string nth + StrcpyFunction f, FunctionCall fc, int argDest, int argSrc, int argLimit, int charSize, + Access copyDest, Access copySource, string name, string nth where f = fc.getTarget() and argDest = f.getParamDest() and