Merge pull request #9998 from d10c/use-strcpyfunction-in-bad-strncpy-size

Use StrcpyFunction in `cpp/bad-strncpy-size`

This PR:

- Uses the [StrcpyFunction](https://github.com/github/codeql/blob/main/cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll#L14) class in the [StrncpyFlippedArgs](https://github.com/github/codeql/blob/main/cpp/ql/src/Likely%20Bugs/Memory%20Management/StrncpyFlippedArgs.ql) query instead of an ad-hoc predicate for finding strcpy-like functions.
- Tests this by adding one previously unsupported strcpy-like function (`wcsxfrm_l`) to StrncpyFlippedArgs's test.cpp.

Author: d10c

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,
-  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
-  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

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.

cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected

@@ -1,18 +1,23 @@
 | 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. |
+| 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. |

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);	// 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
+}