C++: Model scanf and fscanf as flow sources

Author: jketema

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

@@ -27,7 +27,7 @@ private import implementations.StdString
 private import implementations.Swap
 private import implementations.GetDelim
 private import implementations.SmartPointer
-private import implementations.Sscanf
+private import implementations.Scanf
 private import implementations.Send
 private import implementations.Recv
 private import implementations.Accept

cpp/ql/lib/semmle/code/cpp/models/implementations/Sscanf.qll renamed to cpp/ql/lib/semmle/code/cpp/models/implementations/Scanf.qll

@@ -1,6 +1,6 @@
 /**
- * Provides implementation classes modeling `sscanf`, `fscanf` and various similar
- * functions. See `semmle.code.cpp.models.Models` for usage information.
+ * Provides implementation classes modeling the `scanf` family of functions.
+ * See `semmle.code.cpp.models.Models` for usage information.
  */
 
 import semmle.code.cpp.Function
@@ -9,18 +9,15 @@ import semmle.code.cpp.models.interfaces.ArrayFunction
 import semmle.code.cpp.models.interfaces.Taint
 import semmle.code.cpp.models.interfaces.Alias
 import semmle.code.cpp.models.interfaces.SideEffect
+import semmle.code.cpp.models.interfaces.FlowSource
 
 /**
- * The standard function `sscanf`, `fscanf` and its assorted variants
+ * The `scanf` family of functions.
  */
-private class SscanfModel extends ArrayFunction, TaintFunction, AliasFunction, SideEffectFunction {
-  SscanfModel() { this instanceof Sscanf or this instanceof Fscanf or this instanceof Snscanf }
-
+abstract private class ScanfFunctionModel extends ArrayFunction, TaintFunction, AliasFunction,
+  SideEffectFunction {
   override predicate hasArrayWithNullTerminator(int bufParam) {
     bufParam = this.(ScanfFunction).getFormatParameterIndex()
-    or
-    not this instanceof Fscanf and
-    bufParam = this.(ScanfFunction).getInputParameterIndex()
   }
 
   override predicate hasArrayInput(int bufParam) { this.hasArrayWithNullTerminator(bufParam) }
@@ -36,7 +33,7 @@ private class SscanfModel extends ArrayFunction, TaintFunction, AliasFunction, S
     )
   }
 
-  private int getArgsStartPosition() { result = this.getNumberOfParameters() }
+  int getArgsStartPosition() { result = this.getNumberOfParameters() }
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     input.isParameterDeref(this.(ScanfFunction).getInputParameterIndex()) and
@@ -70,3 +67,40 @@ private class SscanfModel extends ArrayFunction, TaintFunction, AliasFunction, S
       ]
   }
 }
+
+/**
+ * The standard function `scanf` and its assorted variants
+ */
+private class ScanfModel extends ScanfFunctionModel, LocalFlowSourceFunction {
+  ScanfModel() { this instanceof Scanf }
+
+  override predicate hasLocalFlowSource(FunctionOutput output, string description) {
+    output.isParameterDeref(any(int i | i >= this.getArgsStartPosition())) and
+    description = "Value read by " + this.getName()
+  }
+}
+
+/**
+ * The standard function `fscanf` and its assorted variants
+ */
+private class FscanfModel extends ScanfFunctionModel, RemoteFlowSourceFunction {
+  FscanfModel() { this instanceof Fscanf }
+
+  override predicate hasRemoteFlowSource(FunctionOutput output, string description) {
+    output.isParameterDeref(any(int i | i >= this.getArgsStartPosition())) and
+    description = "Value read by " + this.getName()
+  }
+}
+
+/**
+ * The standard function `sscanf` and its assorted variants
+ */
+private class SscanfModel extends ScanfFunctionModel {
+  SscanfModel() { this instanceof Sscanf or this instanceof Snscanf }
+
+  override predicate hasArrayWithNullTerminator(int bufParam) {
+    super.hasArrayWithNullTerminator(bufParam)
+    or
+    bufParam = this.(ScanfFunction).getInputParameterIndex()
+  }
+}

cpp/ql/test/library-tests/dataflow/source-sink-tests/local-flow.ql

@@ -11,8 +11,20 @@ class LocalFlowSourceTest extends InlineExpectationsTest {
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {
     tag = "local_source" and
-    value = "" and
-    exists(LocalFlowSource node |
+    exists(LocalFlowSource node, int n |
+      n =
+        strictcount(LocalFlowSource otherNode |
+          node.getLocation().getStartLine() = otherNode.getLocation().getStartLine()
+        ) and
+      (
+        n = 1 and value = ""
+        or
+        // If there is more than one node on this line
+        // we specify the location explicitly.
+        n > 1 and
+        value =
+          node.getLocation().getStartLine().toString() + ":" + node.getLocation().getStartColumn()
+      ) and
       location = node.getLocation() and
       element = node.toString()
     )

cpp/ql/test/library-tests/dataflow/source-sink-tests/remote-flow.ql

@@ -11,8 +11,20 @@ class RemoteFlowSourceTest extends InlineExpectationsTest {
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {
     tag = "remote_source" and
-    value = "" and
-    exists(RemoteFlowSource node |
+    exists(RemoteFlowSource node, int n |
+      n =
+        strictcount(RemoteFlowSource otherNode |
+          node.getLocation().getStartLine() = otherNode.getLocation().getStartLine()
+        ) and
+      (
+        n = 1 and value = ""
+        or
+        // If there is more than one node on this line
+        // we specify the location explicitly.
+        n > 1 and
+        value =
+          node.getLocation().getStartLine().toString() + ":" + node.getLocation().getStartColumn()
+      ) and
       location = node.getLocation() and
       element = node.toString()
     )
@@ -26,8 +38,20 @@ class RemoteFlowSinkTest extends InlineExpectationsTest {
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {
     tag = "remote_sink" and
-    value = "" and
-    exists(RemoteFlowSink node |
+    exists(RemoteFlowSink node, int n |
+      n =
+        strictcount(RemoteFlowSink otherNode |
+          node.getLocation().getStartLine() = otherNode.getLocation().getStartLine()
+        ) and
+      (
+        n = 1 and value = ""
+        or
+        // If there is more than one node on this line
+        // we specify the location explicitly.
+        n > 1 and
+        value =
+          node.getLocation().getStartLine().toString() + ":" + node.getLocation().getStartColumn()
+      ) and
       location = node.getLocation() and
       element = node.toString()
     )

cpp/ql/test/library-tests/dataflow/source-sink-tests/sources-and-sinks.cpp

@@ -26,3 +26,17 @@ void test_readv_and_writev(iovec* iovs) {
   readv(0, iovs, 16); // $ remote_source
   writev(0, iovs, 16); // $ remote_sink
 }
+
+struct FILE;
+
+int fscanf(FILE *stream, const char *format, ...);
+int scanf(const char *format, ...);
+
+void test_scanf(FILE *stream, int *d, char *buf) {
+  scanf(""); // Not a local source, as there are no output arguments
+  fscanf(stream, ""); // Not a remote source, as there are no output arguments
+  scanf("%d", d); // $ local_source
+  fscanf(stream, "%d", d);  // $ remote_source
+  scanf("%d %s", d, buf); // $ local_source=40:18 local_source=40:21
+  fscanf(stream, "%d %s", d, buf);  // $ remote_source=41:27 remote_source=41:30
+}