Merge pull request #2849 from geoffw0/model-gets

C++: Model for gets

Author: jbj

change-notes/1.24/analysis-cpp.md

@@ -44,6 +44,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
   * The `LocalScopeVariableReachability` library is deprecated in favor of
     `StackVariableReachability`. The functionality is the same.
 * The models library models `strlen` in more detail, and includes common variations such as `wcslen`.
+* The models library models `gets` and similar functions.
 * The taint tracking library (`semmle.code.cpp.dataflow.TaintTracking`) has had
   the following improvements:
   * The library now models data flow through `strdup` and similar functions.

cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -76,6 +76,8 @@ private class DefaultTaintTrackingCfg extends DataFlow::Configuration {
   }
 
   override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
+
+  override predicate isBarrierIn(DataFlow::Node node) { nodeIsBarrierIn(node) }
 }
 
 private class ToGlobalVarTaintTrackingCfg extends DataFlow::Configuration {
@@ -96,6 +98,8 @@ private class ToGlobalVarTaintTrackingCfg extends DataFlow::Configuration {
   }
 
   override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
+
+  override predicate isBarrierIn(DataFlow::Node node) { nodeIsBarrierIn(node) }
 }
 
 private class FromGlobalVarTaintTrackingCfg extends DataFlow2::Configuration {
@@ -119,6 +123,8 @@ private class FromGlobalVarTaintTrackingCfg extends DataFlow2::Configuration {
   }
 
   override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
+
+  override predicate isBarrierIn(DataFlow::Node node) { nodeIsBarrierIn(node) }
 }
 
 private predicate readsVariable(LoadInstruction load, Variable var) {
@@ -153,6 +159,11 @@ private predicate nodeIsBarrier(DataFlow::Node node) {
   )
 }
 
+private predicate nodeIsBarrierIn(DataFlow::Node node) {
+  // don't use dataflow into taint sources, as this leads to duplicate results.
+  node = getNodeForSource(any(Expr e))
+}
+
 private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   // Expressions computed from tainted data are also tainted
   exists(CallInstruction call, int argIndex | call = i2 |

cpp/ql/src/semmle/code/cpp/models/Models.qll

@@ -1,6 +1,7 @@
 private import implementations.Allocation
 private import implementations.Deallocation
 private import implementations.Fread
+private import implementations.Gets
 private import implementations.IdentityFunction
 private import implementations.Inet
 private import implementations.Memcpy

cpp/ql/src/semmle/code/cpp/models/implementations/Gets.qll

@@ -0,0 +1,45 @@
+import semmle.code.cpp.models.interfaces.DataFlow
+import semmle.code.cpp.models.interfaces.Taint
+import semmle.code.cpp.models.interfaces.ArrayFunction
+import semmle.code.cpp.models.interfaces.Alias
+import semmle.code.cpp.models.interfaces.SideEffect
+
+/**
+ * The standard functions `gets` and `fgets`.
+ */
+class GetsFunction extends DataFlowFunction, TaintFunction, ArrayFunction, AliasFunction,
+  SideEffectFunction {
+  GetsFunction() {
+    exists(string name | hasGlobalOrStdName(name) |
+      name = "gets" or // gets(str)
+      name = "fgets" or // fgets(str, num, stream)
+      name = "fgetws" // fgetws(wstr, num, stream)
+    )
+  }
+
+  override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
+    input.isParameter(0) and
+    output.isReturnValue()
+  }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    input.isParameter(2) and
+    output.isParameterDeref(0)
+  }
+
+  override predicate parameterNeverEscapes(int index) { index = 2 }
+
+  override predicate parameterEscapesOnlyViaReturn(int index) { index = 0 }
+
+  override predicate parameterIsAlwaysReturned(int index) { index = 0 }
+
+  override predicate hasOnlySpecificReadSideEffects() { any() }
+
+  override predicate hasOnlySpecificWriteSideEffects() { any() }
+
+  override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) {
+    i = 0 and
+    buffer = true and
+    mustWrite = true
+  }
+}

cpp/ql/test/library-tests/dataflow/security-taint/tainted.expected

@@ -67,3 +67,9 @@
 | test.cpp:83:28:83:33 | call to getenv | test.cpp:88:7:88:27 | (bool)... |  |
 | test.cpp:83:28:83:33 | call to getenv | test.cpp:88:14:88:17 | (const char *)... |  |
 | test.cpp:83:28:83:33 | call to getenv | test.cpp:88:14:88:17 | copy |  |
+| test.cpp:100:12:100:15 | call to gets | test.cpp:98:8:98:14 | pointer |  |
+| test.cpp:100:12:100:15 | call to gets | test.cpp:100:2:100:8 | pointer |  |
+| test.cpp:100:12:100:15 | call to gets | test.cpp:100:12:100:15 | call to gets |  |
+| test.cpp:100:17:100:22 | buffer | test.cpp:93:18:93:18 | s |  |
+| test.cpp:100:17:100:22 | buffer | test.cpp:97:7:97:12 | buffer |  |
+| test.cpp:100:17:100:22 | buffer | test.cpp:100:17:100:22 | buffer |  |

cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected

@@ -11,3 +11,6 @@
 | test.cpp:83:28:83:33 | call to getenv | test.cpp:11:20:11:21 | s1 | AST only |
 | test.cpp:83:28:83:33 | call to getenv | test.cpp:85:8:85:11 | copy | AST only |
 | test.cpp:83:28:83:33 | call to getenv | test.cpp:86:9:86:12 | copy | AST only |
+| test.cpp:100:12:100:15 | call to gets | test.cpp:100:2:100:8 | pointer | AST only |
+| test.cpp:100:17:100:22 | buffer | test.cpp:97:7:97:12 | buffer | AST only |
+| test.cpp:100:17:100:22 | buffer | test.cpp:100:17:100:22 | array to pointer conversion | IR only |

cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected

@@ -52,3 +52,8 @@
 | test.cpp:83:28:83:33 | call to getenv | test.cpp:88:7:88:27 | (bool)... |  |
 | test.cpp:83:28:83:33 | call to getenv | test.cpp:88:14:88:17 | (const char *)... |  |
 | test.cpp:83:28:83:33 | call to getenv | test.cpp:88:14:88:17 | copy |  |
+| test.cpp:100:12:100:15 | call to gets | test.cpp:98:8:98:14 | pointer |  |
+| test.cpp:100:12:100:15 | call to gets | test.cpp:100:12:100:15 | call to gets |  |
+| test.cpp:100:17:100:22 | buffer | test.cpp:93:18:93:18 | s |  |
+| test.cpp:100:17:100:22 | buffer | test.cpp:100:17:100:22 | array to pointer conversion |  |
+| test.cpp:100:17:100:22 | buffer | test.cpp:100:17:100:22 | buffer |  |

cpp/ql/test/library-tests/dataflow/security-taint/test.cpp

@@ -88,4 +88,14 @@ void mallocBuffer() {
 	if (!strcmp(copy, "admin")) { // copy should be tainted
 		isAdmin = true;
 	}
-}
+}
+
+char *gets(char *s);
+
+void test_gets()
+{
+	char buffer[1024];
+	char *pointer;
+
+	pointer = gets(buffer);
+}

cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected

@@ -1,10 +1,5 @@
 | tests.c:28:3:28:9 | call to sprintf | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:28:22:28:25 | argv | argv |
 | tests.c:29:3:29:9 | call to sprintf | This 'call to sprintf' with input from $@ may overflow the destination. | tests.c:29:28:29:31 | argv | argv |
-| tests.c:31:15:31:23 | buffer100 | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:28:22:28:25 | argv | argv |
-| tests.c:31:15:31:23 | buffer100 | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:29:28:29:31 | argv | argv |
 | tests.c:31:15:31:23 | buffer100 | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:31:15:31:23 | buffer100 | buffer100 |
-| tests.c:33:21:33:29 | buffer100 | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:28:22:28:25 | argv | argv |
-| tests.c:33:21:33:29 | buffer100 | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:29:28:29:31 | argv | argv |
-| tests.c:33:21:33:29 | buffer100 | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:31:15:31:23 | buffer100 | buffer100 |
 | tests.c:33:21:33:29 | buffer100 | This 'scanf string argument' with input from $@ may overflow the destination. | tests.c:33:21:33:29 | buffer100 | buffer100 |
 | tests.c:34:25:34:33 | buffer100 | This 'sscanf string argument' with input from $@ may overflow the destination. | tests.c:34:10:34:13 | argv | argv |

cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/funcs/funcsLocal.expected

@@ -1,5 +1,7 @@
 | funcsLocal.c:17:9:17:10 | i1 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:16:8:16:9 | i1 | fread |
 | funcsLocal.c:27:9:27:10 | i3 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:26:8:26:9 | i3 | fgets |
 | funcsLocal.c:32:9:32:10 | i4 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:31:13:31:17 | call to fgets | fgets |
+| funcsLocal.c:32:9:32:10 | i4 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:31:19:31:21 | i41 | fgets |
 | funcsLocal.c:37:9:37:10 | i5 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:36:7:36:8 | i5 | gets |
 | funcsLocal.c:42:9:42:10 | i6 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:41:13:41:16 | call to gets | gets |
+| funcsLocal.c:42:9:42:10 | i6 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:41:18:41:20 | i61 | gets |