Merge pull request #2680 from geoffw0/modelstrndup

Robert Marsh · web-flow · commit 79a72a34964b · 2020-01-27T15:19:52.000-05:00
CPP: Model strndup.
diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/TaintTrackingUtil.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/TaintTrackingUtil.qll
@@ -132,6 +132,9 @@ private predicate exprToExprStep(Expr exprIn, Expr exprOut) {
       // dest_ptr = strdup(tainted_ptr)
       inModel.isParameterDeref(argInIndex) and
       exprIn = call.getArgument(argInIndex)
+      or
+      inModel.isParameter(argInIndex) and
+      exprIn = call.getArgument(argInIndex)
     )
   )
   or
@@ -173,6 +176,9 @@ private predicate exprToDefinitionByReferenceStep(Expr exprIn, Expr argOut) {
       // memcpy(&dest_var, tainted_ptr, len)
       inModel.isParameterDeref(argInIndex) and
       exprIn = call.getArgument(argInIndex)
+      or
+      inModel.isParameter(argInIndex) and
+      exprIn = call.getArgument(argInIndex)
     )
   )
   or
diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll
@@ -47,20 +47,11 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction {
   }
 
   override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
-    (
-      // These always copy the full value of the input buffer to the output
-      // buffer
-      this.hasName("strcpy") or
-      this.hasName("_mbscpy") or
-      this.hasName("wcscpy")
-    ) and
-    (
-      input.isParameterDeref(1) and
-      output.isParameterDeref(0)
-      or
-      input.isParameterDeref(1) and
-      output.isReturnValueDeref()
-    )
+    input.isParameterDeref(1) and
+    output.isParameterDeref(0)
+    or
+    input.isParameterDeref(1) and
+    output.isReturnValueDeref()
     or
     input.isParameter(0) and
     output.isReturnValue()
@@ -77,10 +68,7 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction {
       this.hasName("wcsncpy") or
       this.hasName("_wcsncpy_l")
     ) and
-    (
-      input.isParameter(2) or
-      input.isParameterDeref(1)
-    ) and
+    input.isParameter(2) and
     (
       output.isParameterDeref(0) or
       output.isReturnValueDeref()
diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Strdup.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Strdup.qll
@@ -9,17 +9,14 @@ import semmle.code.cpp.models.interfaces.Taint
 class StrdupFunction extends AllocationFunction, ArrayFunction, DataFlowFunction {
   StrdupFunction() {
     exists(string name |
-      hasGlobalOrStdName(name) and
+      hasGlobalName(name) and
       (
         // strdup(str)
         name = "strdup"
         or
         // wcsdup(str)
         name = "wcsdup"
-      )
-      or
-      hasGlobalName(name) and
-      (
+        or
         // _strdup(str)
         name = "_strdup"
         or
@@ -37,9 +34,32 @@ class StrdupFunction extends AllocationFunction, ArrayFunction, DataFlowFunction
   override predicate hasArrayWithNullTerminator(int bufParam) { bufParam = 0 }
 
   override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
-    // These always copy the full value of the input buffer to the result
-    // buffer
     input.isParameterDeref(0) and
     output.isReturnValueDeref()
   }
 }
+
+/**
+ * A `strndup` style allocation function.
+ */
+class StrndupFunction extends AllocationFunction, ArrayFunction, DataFlowFunction {
+  StrndupFunction() {
+    exists(string name |
+      hasGlobalName(name) and
+      // strndup(str, maxlen)
+      name = "strndup"
+    )
+  }
+
+  override predicate hasArrayInput(int bufParam) { bufParam = 0 }
+
+  override predicate hasArrayWithNullTerminator(int bufParam) { bufParam = 0 }
+
+  override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
+    (
+      input.isParameterDeref(0) or
+      input.isParameter(1)
+    ) and
+    output.isReturnValueDeref()
+  }
+}
diff --git a/cpp/ql/src/semmle/code/cpp/models/interfaces/DataFlow.qll b/cpp/ql/src/semmle/code/cpp/models/interfaces/DataFlow.qll
@@ -12,8 +12,8 @@ import FunctionInputsAndOutputs
 import semmle.code.cpp.models.Models
 
 /**
- * A library function for which a value is copied from a parameter or qualifier
- * to an output buffer, return value, or qualifier.
+ * A library function for which a value is or may be copied from a parameter
+ * or qualifier to an output buffer, return value, or qualifier.
  *
  * Note that this does not include partial copying of values or partial writes
  * to destinations; that is covered by `TaintModel.qll`.
diff --git a/cpp/ql/src/semmle/code/cpp/models/interfaces/Taint.qll b/cpp/ql/src/semmle/code/cpp/models/interfaces/Taint.qll
@@ -16,7 +16,9 @@ import semmle.code.cpp.models.Models
  * from a parameter or qualifier to an output buffer, return value, or qualifier.
  *
  * Note that this does not include direct copying of values; that is covered by
- * DataFlowModel.qll
+ * DataFlowModel.qll. If a value is sometimes copied in full, and sometimes
+ * altered (for example copying a string with `strncpy`), this is also considered
+ * data flow.
  */
 abstract class TaintFunction extends Function {
   abstract predicate hasTaintFlow(FunctionInput input, FunctionOutput output);
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected
@@ -337,9 +337,13 @@
 | taint.cpp:370:13:370:26 | hello, world | taint.cpp:370:6:370:11 | call to strdup | TAINT |
 | taint.cpp:371:6:371:12 | call to strndup | taint.cpp:371:2:371:25 | ... = ... |  |
 | taint.cpp:371:6:371:12 | call to strndup | taint.cpp:374:7:374:7 | c |  |
+| taint.cpp:371:14:371:19 | source | taint.cpp:371:6:371:12 | call to strndup | TAINT |
+| taint.cpp:371:22:371:24 | 100 | taint.cpp:371:6:371:12 | call to strndup | TAINT |
 | taint.cpp:377:23:377:28 | source | taint.cpp:381:30:381:35 | source |  |
 | taint.cpp:381:6:381:12 | call to strndup | taint.cpp:381:2:381:36 | ... = ... |  |
 | taint.cpp:381:6:381:12 | call to strndup | taint.cpp:382:7:382:7 | a |  |
+| taint.cpp:381:14:381:27 | hello, world | taint.cpp:381:6:381:12 | call to strndup | TAINT |
+| taint.cpp:381:30:381:35 | source | taint.cpp:381:6:381:12 | call to strndup | TAINT |
 | taint.cpp:385:27:385:32 | source | taint.cpp:389:13:389:18 | source |  |
 | taint.cpp:389:6:389:11 | call to wcsdup | taint.cpp:389:2:389:19 | ... = ... |  |
 | taint.cpp:389:6:389:11 | call to wcsdup | taint.cpp:391:7:391:7 | a |  |
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp
@@ -371,15 +371,15 @@ void test_strdup(char *source)
 	c = strndup(source, 100);
 	sink(a); // tainted
 	sink(b);
-	sink(c); // tainted [NOT DETECTED]
+	sink(c); // tainted
 }
 
 void test_strndup(int source)
 {
 	char *a;
 
 	a = strndup("hello, world", source);
-	sink(a);
+	sink(a); // tainted
 }
 
 void test_wcsdup(wchar_t *source)
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected
@@ -38,6 +38,8 @@
 | taint.cpp:351:7:351:7 | a | taint.cpp:330:6:330:11 | call to source |
 | taint.cpp:352:7:352:7 | b | taint.cpp:330:6:330:11 | call to source |
 | taint.cpp:372:7:372:7 | a | taint.cpp:365:24:365:29 | source |
+| taint.cpp:374:7:374:7 | c | taint.cpp:365:24:365:29 | source |
+| taint.cpp:382:7:382:7 | a | taint.cpp:377:23:377:28 | source |
 | taint.cpp:391:7:391:7 | a | taint.cpp:385:27:385:32 | source |
 | taint.cpp:423:7:423:7 | a | taint.cpp:422:14:422:19 | call to source |
 | taint.cpp:424:9:424:17 | call to getMember | taint.cpp:422:14:422:19 | call to source |
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected
@@ -25,6 +25,8 @@
 | taint.cpp:351:7:351:7 | taint.cpp:330:6:330:11 | AST only |
 | taint.cpp:352:7:352:7 | taint.cpp:330:6:330:11 | AST only |
 | taint.cpp:372:7:372:7 | taint.cpp:365:24:365:29 | AST only |
+| taint.cpp:374:7:374:7 | taint.cpp:365:24:365:29 | AST only |
+| taint.cpp:382:7:382:7 | taint.cpp:377:23:377:28 | AST only |
 | taint.cpp:391:7:391:7 | taint.cpp:385:27:385:32 | AST only |
 | taint.cpp:423:7:423:7 | taint.cpp:422:14:422:19 | AST only |
 | taint.cpp:424:9:424:17 | taint.cpp:422:14:422:19 | AST only |

Original file line number	Diff line number	Diff line change
`@@ -132,6 +132,9 @@ private predicate exprToExprStep(Expr exprIn, Expr exprOut) {`
`132`	`132`	`// dest_ptr = strdup(tainted_ptr)`
`133`	`133`	`inModel.isParameterDeref(argInIndex) and`
`134`	`134`	`exprIn = call.getArgument(argInIndex)`
	`135`	`+ or`
	`136`	`+ inModel.isParameter(argInIndex) and`
	`137`	`+ exprIn = call.getArgument(argInIndex)`
`135`	`138`	`)`
`136`	`139`	`)`
`137`	`140`	`or`
`@@ -173,6 +176,9 @@ private predicate exprToDefinitionByReferenceStep(Expr exprIn, Expr argOut) {`
`173`	`176`	`// memcpy(&dest_var, tainted_ptr, len)`
`174`	`177`	`inModel.isParameterDeref(argInIndex) and`
`175`	`178`	`exprIn = call.getArgument(argInIndex)`
	`179`	`+ or`
	`180`	`+ inModel.isParameter(argInIndex) and`
	`181`	`+ exprIn = call.getArgument(argInIndex)`
`176`	`182`	`)`
`177`	`183`	`)`
`178`	`184`	`or`
Original file line number	Diff line number	Diff line change
`@@ -371,15 +371,15 @@ void test_strdup(char *source)`
`371`	`371`	`c = strndup(source, 100);`
`372`	`372`	`sink(a); // tainted`
`373`	`373`	`sink(b);`
`374`		`- sink(c); // tainted [NOT DETECTED]`
	`374`	`+ sink(c); // tainted`
`375`	`375`	`}`
`376`	`376`
`377`	`377`	`void test_strndup(int source)`
`378`	`378`	`{`
`379`	`379`	`char *a;`
`380`	`380`
`381`	`381`	`a = strndup("hello, world", source);`
`382`		`- sink(a);`
	`382`	`+ sink(a); // tainted`
`383`	`383`	`}`
`384`	`384`
`385`	`385`	`void test_wcsdup(wchar_t *source)`