Merge pull request github#2849 from geoffw0/model-gets

jbj · web-flow · commit f1ad0dafdc65 · 2020-03-18T11:06:23.000+01:00
C++: Model for gets
diff --git a/change-notes/1.24/analysis-cpp.md b/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.
diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/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 |
diff --git a/cpp/ql/src/semmle/code/cpp/models/Models.qll b/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
diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Gets.qll b/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
+  }
+}
diff --git a/cpp/ql/test/library-tests/dataflow/security-taint/tainted.expected b/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 |  |
diff --git a/cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected b/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 |
diff --git a/cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected b/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 |  |
diff --git a/cpp/ql/test/library-tests/dataflow/security-taint/test.cpp b/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);
+}
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/UnboundedWrite.expected b/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 |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/funcs/funcsLocal.expected b/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 |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected
@@ -1,7 +1,6 @@
 | test.c:21:17:21:17 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:18:13:18:16 | call to rand | Uncontrolled value |
 | test.c:35:5:35:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:34:13:34:18 | call to rand | Uncontrolled value |
 | test.c:40:5:40:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:39:13:39:21 | ... % ... | Uncontrolled value |
-| test.c:40:5:40:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:39:13:39:22 | call to rand | Uncontrolled value |
 | test.c:45:5:45:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:44:13:44:16 | call to rand | Uncontrolled value |
 | test.c:56:5:56:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:54:13:54:16 | call to rand | Uncontrolled value |
 | test.c:67:5:67:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:66:13:66:16 | call to rand | Uncontrolled value |

Original file line number	Diff line number	Diff line change
`@@ -76,6 +76,8 @@ private class DefaultTaintTrackingCfg extends DataFlow::Configuration {`
`76`	`76`	`}`
`77`	`77`
`78`	`78`	`override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }`
	`79`	`+`
	`80`	`+ override predicate isBarrierIn(DataFlow::Node node) { nodeIsBarrierIn(node) }`
`79`	`81`	`}`
`80`	`82`
`81`	`83`	`private class ToGlobalVarTaintTrackingCfg extends DataFlow::Configuration {`
`@@ -96,6 +98,8 @@ private class ToGlobalVarTaintTrackingCfg extends DataFlow::Configuration {`
`96`	`98`	`}`
`97`	`99`
`98`	`100`	`override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }`
	`101`	`+`
	`102`	`+ override predicate isBarrierIn(DataFlow::Node node) { nodeIsBarrierIn(node) }`
`99`	`103`	`}`
`100`	`104`
`101`	`105`	`private class FromGlobalVarTaintTrackingCfg extends DataFlow2::Configuration {`
`@@ -119,6 +123,8 @@ private class FromGlobalVarTaintTrackingCfg extends DataFlow2::Configuration {`
`119`	`123`	`}`
`120`	`124`
`121`	`125`	`override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }`
	`126`	`+`
	`127`	`+ override predicate isBarrierIn(DataFlow::Node node) { nodeIsBarrierIn(node) }`
`122`	`128`	`}`
`123`	`129`
`124`	`130`	`private predicate readsVariable(LoadInstruction load, Variable var) {`
`@@ -153,6 +159,11 @@ private predicate nodeIsBarrier(DataFlow::Node node) {`
`153`	`159`	`)`
`154`	`160`	`}`
`155`	`161`
	`162`	`+private predicate nodeIsBarrierIn(DataFlow::Node node) {`
	`163`	`+ // don't use dataflow into taint sources, as this leads to duplicate results.`
	`164`	`+ node = getNodeForSource(any(Expr e))`
	`165`	`+}`
	`166`	`+`
`156`	`167`	`private predicate instructionTaintStep(Instruction i1, Instruction i2) {`
`157`	`168`	`// Expressions computed from tainted data are also tainted`
`158`	`169`	`exists(CallInstruction call, int argIndex \| call = i2 \|`