C++: Tidy up the ql file and accept test changes.

Author: MathiasVP

cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.ql

@@ -11,54 +11,32 @@
  */
 
 import cpp
+import semmle.code.cpp.models.implementations.Strcat
 import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 
 /**
- * A call to `strncat` of the form `strncat(buff, str, someExpr - strlen(buf))`, for some expression `someExpr` equal to `sizeof(buff)`.
+ * Holds if `call` is a call to `strncat` such that `sizeArg` and `destArg` are the size and
+ * destination arguments, respectively.
  */
-class WrongCallStrncat extends FunctionCall {
-  Expr leftsomeExpr;
-
-  WrongCallStrncat() {
-    this.getTarget().hasGlobalOrStdName("strncat") and
-    // the expression of the first argument in `strncat` and `strnlen` is identical
-    globalValueNumber(this.getArgument(0)) =
-      globalValueNumber(this.getArgument(2).(SubExpr).getRightOperand().(StrlenCall).getStringExpr()) and
-    // using a string constant often speaks of manually calculating the length of the required buffer.
-    (
-      not this.getArgument(1) instanceof StringLiteral and
-      not this.getArgument(1) instanceof CharLiteral
-    ) and
-    // for use in predicates
-    leftsomeExpr = this.getArgument(2).(SubExpr).getLeftOperand()
-  }
-
-  /**
-   * Holds if the left side of the expression `someExpr` equal to `sizeof(buf)`.
-   */
-  predicate isExpressionEqualSizeof() {
-    // the left side of the expression `someExpr` is `sizeof(buf)`.
-    globalValueNumber(this.getArgument(0)) =
-      globalValueNumber(leftsomeExpr.(SizeofExprOperator).getExprOperand())
-    or
-    // value of the left side of the expression `someExpr` equal  `sizeof(buf)` value, and `buf` is array.
-    leftsomeExpr.getValue().toInt() = this.getArgument(0).getType().getSize()
-  }
-
-  /**
-   * Holds if the left side of the expression `someExpr` equal to variable containing the length of the memory allocated for the buffer.
-   */
-  predicate isVariableEqualValueSizegBuffer() {
-    // the left side of expression `someExpr` is the variable that was used in the function of allocating memory for the buffer`.
-    exists(AllocationExpr alc |
-      leftsomeExpr.(VariableAccess).getTarget() =
-        alc.(FunctionCall).getArgument(0).(VariableAccess).getTarget()
-    )
-  }
+predicate interestringCallWithArgs(Call call, Expr sizeArg, Expr destArg) {
+  exists(StrcatFunction strcat |
+    strcat = call.getTarget() and
+    sizeArg = call.getArgument(strcat.getParamSize()) and
+    destArg = call.getArgument(strcat.getParamDest())
+  )
 }
 
-from WrongCallStrncat sc
+from FunctionCall call, Expr sizeArg, Expr destArg, SubExpr sub, int n
 where
-  sc.isExpressionEqualSizeof() or
-  sc.isVariableEqualValueSizegBuffer()
-select sc, "if the used buffer is full, writing out of the buffer is possible"
+  interestringCallWithArgs(call, sizeArg, destArg) and
+  // The destination buffer is an array of size n
+  destArg.getUnspecifiedType().(ArrayType).getSize() = n and
+  // The size argument is equivalent to a subtraction
+  globalValueNumber(sizeArg).getAnExpr() = sub and
+  // ... where the left side of the subtraction is the constant n
+  globalValueNumber(sub.getLeftOperand()).getAnExpr().getValue().toInt() = n and
+  // ... and the right side of the subtraction is a call to `strlen` where the argument is the
+  // destination buffer.
+  globalValueNumber(sub.getRightOperand()).getAnExpr().(StrlenCall).getStringExpr() =
+    globalValueNumber(destArg).getAnExpr()
+select call, "Possible out-of-bounds write due to incorrect size argument."

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.expected

@@ -1,3 +1,5 @@
-| test.c:8:3:8:9 | call to strncat | if the used buffer is full, writing out of the buffer is possible |
-| test.c:17:3:17:9 | call to strncat | if the used buffer is full, writing out of the buffer is possible |
-| test.c:25:3:25:9 | call to strncat | if the used buffer is full, writing out of the buffer is possible |
+| test.c:8:3:8:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
+| test.c:9:3:9:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
+| test.c:17:3:17:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
+| test.c:18:3:18:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
+| test.c:46:3:46:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/test.c

@@ -6,7 +6,7 @@ void strncat_test1(char *s) {
   char buf[80];
   strncat(buf, s, sizeof(buf) - strlen(buf) - 1); // GOOD
   strncat(buf, s, sizeof(buf) - strlen(buf));  // BAD
-  strncat(buf, "fix", sizeof(buf)-strlen(buf)); // BAD [NOT DETECTED]
+  strncat(buf, "fix", sizeof(buf)-strlen(buf)); // BAD
 }
 
 #define MAX_SIZE 80
@@ -15,14 +15,14 @@ void strncat_test2(char *s) {
   char buf[MAX_SIZE];
   strncat(buf, s, MAX_SIZE - strlen(buf) - 1); // GOOD
   strncat(buf, s, MAX_SIZE - strlen(buf));  // BAD
-  strncat(buf, "fix", MAX_SIZE - strlen(buf)); // BAD [NOT DETECTED]
+  strncat(buf, "fix", MAX_SIZE - strlen(buf)); // BAD
 }
 
 void strncat_test3(char *s) {
   int len = 80;
   char* buf = (char *) malloc(len);
   strncat(buf, s, len - strlen(buf) - 1); // GOOD
-  strncat(buf, s, len - strlen(buf));  // BAD
+  strncat(buf, s, len - strlen(buf));  // BAD [NOT DETECTED]
   strncat(buf, "fix", len - strlen(buf)); // BAD [NOT DETECTED]
 }
 
@@ -43,7 +43,7 @@ void strncat_test5(char* s, struct buffers* buffers) {
   unsigned len_array = strlen(buffers->array);
   unsigned max_size = sizeof(buffers->array);
   unsigned free_size = max_size - len_array;
-  strncat(buffers->array, s, free_size); // BAD [NOT DETECTED]
+  strncat(buffers->array, s, free_size); // BAD
 }
 
 void strlen_test1(){