Merge pull request #579 from knewbury01/knewbury01/fix-31

STR32-C: reduce false negative related to realloc model

c/cert/src/rules/STR32-C/NonNullTerminatedToFunctionThatExpectsAString.ql

@@ -17,6 +17,7 @@ import codingstandards.c.cert
 import codingstandards.cpp.Naming
 import codingstandards.cpp.dataflow.TaintTracking
 import codingstandards.cpp.PossiblyUnsafeStringOperation
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 
 /**
  * Models a function that is part of the standard library that expects a
@@ -43,32 +44,90 @@ class ExpectsNullTerminatedStringAsArgumentFunctionCall extends FunctionCall {
   Expr getAnExpectingExpr() { result = e }
 }
 
-from ExpectsNullTerminatedStringAsArgumentFunctionCall fc, Expr e, Expr target
-where
-  target = fc.getAnExpectingExpr() and
-  not isExcluded(fc, Strings1Package::nonNullTerminatedToFunctionThatExpectsAStringQuery()) and
-  (
-    exists(PossiblyUnsafeStringOperation op |
-      // don't report violations of the same function call.
-      not op = fc and
-      e = op and
-      TaintTracking::localTaint(DataFlow::exprNode(op.getAnArgument()), DataFlow::exprNode(target))
+class PossiblyUnsafeStringOperationSource extends Source {
+  PossiblyUnsafeStringOperation op;
+
+  PossiblyUnsafeStringOperationSource() { this.asExpr() = op.getAnArgument() }
+
+  PossiblyUnsafeStringOperation getOp() { result = op }
+}
+
+class CharArraySource extends Source {
+  CharArrayInitializedWithStringLiteral op;
+
+  CharArraySource() {
+    op.getContainerLength() <= op.getStringLiteralLength() and
+    this.asExpr() = op
+  }
+}
+
+abstract class Source extends DataFlow::Node { }
+
+class Sink extends DataFlow::Node {
+  Sink() {
+    exists(ExpectsNullTerminatedStringAsArgumentFunctionCall fc |
+      fc.getAnExpectingExpr() = this.asExpr()
     )
-    or
-    exists(CharArrayInitializedWithStringLiteral op |
-      e = op and
-      op.getContainerLength() <= op.getStringLiteralLength() and
-      TaintTracking::localTaint(DataFlow::exprNode(op), DataFlow::exprNode(target))
+  }
+}
+
+module MyFlowConfiguration implements DataFlow::ConfigSig {
+  predicate isSink(DataFlow::Node sink) {
+    sink instanceof Sink and
+    //don't report violations of the same function call
+    not sink instanceof Source
+  }
+
+  predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  predicate isAdditionalFlowStep(DataFlow::Node innode, DataFlow::Node outnode) {
+    exists(FunctionCall realloc, ReallocFunction fn |
+      fn.getACallToThisFunction() = realloc and
+      realloc.getArgument(0) = innode.asExpr() and
+      realloc = outnode.asExpr()
     )
-  ) and
-  // don't report cases flowing to this node where there is a flow from a
-  // literal assignment of a null terminator
-  not exists(AssignExpr aexp |
+  }
+}
+
+class ReallocFunction extends AllocationFunction {
+  ReallocFunction() { exists(this.getReallocPtrArg()) }
+}
+
+/**
+ * Determines if the string is acceptably null terminated
+ * The only condition we accept as a guarantee to null terminate is:
+ * `str[size_expr] = '\0';`
+ * where we do not check the value of the `size_expr` used
+ */
+predicate isGuarded(Expr guarded, Expr source) {
+  exists(AssignExpr aexp |
     aexp.getLValue() instanceof ArrayExpr and
     aexp.getRValue() instanceof Zero and
-    TaintTracking::localTaint(DataFlow::exprNode(aexp.getRValue()), DataFlow::exprNode(target)) and
-    // this must be AFTER the operation causing the non-null termination to be valid.
-    aexp.getAPredecessor*() = e
+    // this must be AFTER the operation causing the non-null termination
+    aexp.getAPredecessor+() = source and
+    //this guards anything after it
+    aexp.getASuccessor+() = guarded and
+    // no reallocs exist after this because they will be conservatively assumed to make the buffer smaller and remove the likliehood of this properly terminating
+    not exists(ReallocFunction realloc, FunctionCall fn |
+      fn = realloc.getACallToThisFunction() and
+      globalValueNumber(aexp.getLValue().(ArrayExpr).getArrayBase()) =
+        globalValueNumber(fn.getArgument(0)) and
+      aexp.getASuccessor+() = fn
+    )
   )
+}
+
+module MyFlow = TaintTracking::Global<MyFlowConfiguration>;
+
+from
+  DataFlow::Node source, DataFlow::Node sink, ExpectsNullTerminatedStringAsArgumentFunctionCall fc,
+  Expr e
+where
+  MyFlow::flow(source, sink) and
+  sink.asExpr() = fc.getAnExpectingExpr() and
+  not isGuarded(sink.asExpr(), source.asExpr()) and
+  if source instanceof PossiblyUnsafeStringOperationSource
+  then e = source.(PossiblyUnsafeStringOperationSource).getOp()
+  else e = source.asExpr()
 select fc, "String modified by $@ is passed to function expecting a null-terminated string.", e,
   "this expression"

c/cert/test/rules/STR32-C/NonNullTerminatedToFunctionThatExpectsAString.expected

@@ -1,16 +1,22 @@
-| test.c:19:3:19:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:7:20:7:24 | Cod | this expression |
-| test.c:20:3:20:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:7:20:7:24 | Cod | this expression |
-| test.c:22:3:22:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:13:3:13:9 | call to strncpy | this expression |
-| test.c:23:3:23:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:13:3:13:9 | call to strncpy | this expression |
-| test.c:24:3:24:8 | call to strlen | String modified by $@ is passed to function expecting a null-terminated string. | test.c:13:3:13:9 | call to strncpy | this expression |
-| test.c:33:3:33:9 | call to wprintf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:30:24:30:29 | Cod | this expression |
-| test.c:46:3:46:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:41:3:41:10 | call to snprintf | this expression |
-| test.c:47:3:47:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:41:3:41:10 | call to snprintf | this expression |
-| test.c:55:3:55:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:53:3:53:9 | call to strncat | this expression |
-| test.c:56:3:56:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:53:3:53:9 | call to strncat | this expression |
-| test.c:62:3:62:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:60:20:60:24 | Cod | this expression |
-| test.c:63:3:63:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:60:20:60:24 | Cod | this expression |
-| test.c:75:3:75:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:72:20:72:24 | Cod | this expression |
-| test.c:76:3:76:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:72:20:72:24 | Cod | this expression |
-| test.c:85:3:85:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:83:3:83:9 | call to strncpy | this expression |
-| test.c:86:3:86:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:83:3:83:9 | call to strncpy | this expression |
+| test.c:20:3:20:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:8:20:8:24 | Cod | this expression |
+| test.c:21:3:21:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:8:20:8:24 | Cod | this expression |
+| test.c:23:3:23:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:14:3:14:9 | call to strncpy | this expression |
+| test.c:24:3:24:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:14:3:14:9 | call to strncpy | this expression |
+| test.c:25:3:25:8 | call to strlen | String modified by $@ is passed to function expecting a null-terminated string. | test.c:14:3:14:9 | call to strncpy | this expression |
+| test.c:34:3:34:9 | call to wprintf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:31:24:31:29 | Cod | this expression |
+| test.c:47:3:47:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:42:3:42:10 | call to snprintf | this expression |
+| test.c:48:3:48:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:42:3:42:10 | call to snprintf | this expression |
+| test.c:56:3:56:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:54:3:54:9 | call to strncat | this expression |
+| test.c:57:3:57:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:54:3:54:9 | call to strncat | this expression |
+| test.c:63:3:63:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:61:20:61:24 | Cod | this expression |
+| test.c:64:3:64:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:61:20:61:24 | Cod | this expression |
+| test.c:76:3:76:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:73:20:73:24 | Cod | this expression |
+| test.c:77:3:77:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:73:20:73:24 | Cod | this expression |
+| test.c:86:3:86:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:84:3:84:9 | call to strncpy | this expression |
+| test.c:87:3:87:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:84:3:84:9 | call to strncpy | this expression |
+| test.c:95:3:95:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:93:17:93:21 | Cod | this expression |
+| test.c:95:3:95:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:94:3:94:9 | call to strncpy | this expression |
+| test.c:98:3:98:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:93:17:93:21 | Cod | this expression |
+| test.c:98:3:98:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:94:3:94:9 | call to strncpy | this expression |
+| test.c:122:3:122:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:117:17:117:21 | Cod | this expression |
+| test.c:122:3:122:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:118:3:118:9 | call to strncpy | this expression |

c/cert/test/rules/STR32-C/test.c

@@ -1,4 +1,5 @@
 #include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <wchar.h>
 
@@ -84,4 +85,39 @@ f5() {
 
   printf("%s", a1_nnt); // NON_COMPLIANT
   printf(a1_nnt);       // NON_COMPLIANT
-}
+}
+
+void test_fn_reported_in_31_simple() {
+  char *str;
+  str = (char *)malloc(3);
+  char a31[3] = "Cod"; // is NOT null terminated
+  strncpy(str, a31, 3);
+  printf(str); // NON_COMPLIANT
+  size_t cur_msg_size = 1024;
+  str = realloc(str, (cur_msg_size / 2 + 1) * sizeof(char));
+  printf(str); // NON_COMPLIANT
+}
+
+void test_fn_reported_in_31_simple_safe() {
+  char *str;
+  str = (char *)malloc(3);
+  char a31[3] = "Cod"; // is NOT null terminated
+  strncpy(str, a31, 3);
+  size_t cur_msg_size = 1024;
+  size_t temp_size = cur_msg_size / 2 + 1;
+  str = realloc(str, temp_size * sizeof(char));
+  str[temp_size - 1] = L'\0'; // Properly null-terminate str
+  printf(str);                // COMPLIANT
+}
+
+void test_fn_reported_in_31_simple_relloc() {
+  char *str;
+  size_t cur_msg_size = 1024;
+  str = (char *)malloc(cur_msg_size);
+  char a31[3] = "Cod"; // is NOT null terminated
+  strncpy(str, a31, 3);
+  str[cur_msg_size - 1] = L'\0'; // Properly null-terminate str
+  size_t temp_size = cur_msg_size / 2 + 1;
+  str = realloc(str, temp_size * sizeof(char));
+  printf(str); // NON_COMPLIANT
+}

change_notes/2024-05-06-fix-fn-31-STR32C.md

@@ -0,0 +1,2 @@
+- `STR32-C` - `NonNullTerminatedToFunctionThatExpectsAString.ql`:
+  - Fixes #31. Realloc was not modelled previously.

cpp/common/src/codingstandards/cpp/PossiblyUnsafeStringOperation.qll

@@ -37,18 +37,25 @@ class PossiblyUnsafeStringOperation extends FunctionCall {
         bwc.getTarget() instanceof StrcatFunction
         or
         // Case 2: Consider the `strncpy(dest, src, n)` function. We do not
-        // consider `strcpy` since it is a banned function. The behavior of
-        // strncpy(dest, src, n) is that it will copy null terminators only if n
-        // > sizeof(src). If `src` is null-terminated then it will be null
-        // terminated if n >= sizeof(src). We take the conservative approach and
-        // use strictly greater. Thus this can be violated under the condition
-        // that n <  strlen(src). Note that a buffer overflow is possible if
+        // consider `strcpy` since it is a banned function.
+        // We cannot know if the string is already null terminated or not and thus
+        // the conservative assumption is that it is not
+        // The behavior of strncpy(dest, src, n) is that  if sizeof(src) < n
+        // then it will fill remainder of dst with ‘\0’ characters
+        // ie it is only in this case that it is guaranteed to null terminate
+        // Otherwise, dst is not terminated
+        // If `src` is already null-terminated then it will be null
+        // terminated if n >= sizeof(src). but we do not assume on this.
+        // Note that a buffer overflow is possible if
         // `n` is greater than sizeof(dest). The point of this query is not to
         // check for buffer overflows but we would certainly want to indicate
         // this would be a case where a string will not be null terminated.
         bwc.getTarget() instanceof StrcpyFunction and
         (
-          (bwc.getExplicitLimit() / bwc.getCharSize()) < getBufferSize(src, _) or
+          // n <= sizeof(src) might not null terminate
+          (bwc.getExplicitLimit() / bwc.getCharSize()) <= getBufferSize(src, _)
+          or
+          // sizeof(dest) < n might not null terminate
           getBufferSize(dest, _) < (bwc.getExplicitLimit() / bwc.getCharSize())
         )
         or