Merge branch 'main' into redsun82/swift-open-redirection

Author: redsun82

.github/workflows/check-query-ids.yml

@@ -0,0 +1,21 @@
+name: Check query IDs
+
+on:
+  pull_request:
+    paths:
+      - "**/src/**/*.ql"
+      - misc/scripts/check-query-ids.py
+      - .github/workflows/check-query-ids.yml
+    branches:
+      - main
+      - "rc/*"
+  workflow_dispatch:
+
+jobs:
+  check:
+    name: Check query IDs
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - name: Check for duplicate query IDs
+        run: python3 misc/scripts/check-query-ids.py

.github/workflows/swift.yml

@@ -65,6 +65,7 @@ jobs:
     if : ${{ github.event_name == 'pull_request' }}
     needs: build-and-test-macos
     runs-on: macos-12-xl
+    timeout-minutes: 60
     steps:
       - uses: actions/checkout@v3
       - uses: ./swift/actions/run-integration-tests

config/identical-files.json

@@ -470,6 +470,10 @@
     "javascript/ql/src/Comments/CommentedOutCodeReferences.inc.qhelp",
     "python/ql/src/Lexical/CommentedOutCodeReferences.inc.qhelp"
   ],
+  "ThreadResourceAbuse qhelp": [
+    "java/ql/src/experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.qhelp",
+    "java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qhelp"
+  ],
   "IDE Contextual Queries": [
     "cpp/ql/lib/IDEContextual.qll",
     "csharp/ql/lib/IDEContextual.qll",

cpp/ql/lib/change-notes/2022-12-08-support-scanf-dataflow.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `scanf` and `fscanf` functions and their variants are now recognized as flow sources.

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,36 @@ private class SscanfModel extends ArrayFunction, TaintFunction, AliasFunction, S
       ]
   }
 }
+
+/**
+ * The standard function `scanf` and its assorted variants
+ */
+private class ScanfModel extends ScanfFunctionModel, LocalFlowSourceFunction 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 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/src/Security/CWE/CWE-311/CleartextBufferWrite.ql

@@ -19,7 +19,25 @@ import semmle.code.cpp.ir.dataflow.TaintTracking
 import DataFlow::PathGraph
 
 /**
- * A taint flow configuration for flow from user input to a buffer write.
+ * A buffer write into a sensitive expression.
+ */
+class SensitiveBufferWrite extends Expr instanceof BufferWrite::BufferWrite {
+  SensitiveBufferWrite() { super.getDest() instanceof SensitiveExpr }
+
+  /**
+   * Gets a data source of this operation.
+   */
+  Expr getASource() { result = super.getASource() }
+
+  /**
+   * Gets the destination buffer of this operation.
+   */
+  Expr getDest() { result = super.getDest() }
+}
+
+/**
+ * A taint flow configuration for flow from user input to a buffer write
+ * into a sensitive expression.
  */
 class ToBufferConfiguration extends TaintTracking::Configuration {
   ToBufferConfiguration() { this = "ToBufferConfiguration" }
@@ -31,18 +49,17 @@ class ToBufferConfiguration extends TaintTracking::Configuration {
   }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(BufferWrite::BufferWrite w | w.getASource() = sink.asExpr())
+    exists(SensitiveBufferWrite w | w.getASource() = sink.asExpr())
   }
 }
 
 from
-  ToBufferConfiguration config, BufferWrite::BufferWrite w, DataFlow::PathNode sourceNode,
-  DataFlow::PathNode sinkNode, FlowSource source, SensitiveExpr dest
+  ToBufferConfiguration config, SensitiveBufferWrite w, DataFlow::PathNode sourceNode,
+  DataFlow::PathNode sinkNode, FlowSource source
 where
   config.hasFlowPath(sourceNode, sinkNode) and
   sourceNode.getNode() = source and
-  w.getASource() = sinkNode.getNode().asExpr() and
-  dest = w.getDest()
+  w.getASource() = sinkNode.getNode().asExpr()
 select w, sourceNode, sinkNode,
-  "This write into buffer '" + dest.toString() + "' may contain unencrypted data from $@.", source,
-  "user input (" + source.getSourceType() + ")"
+  "This write into buffer '" + w.getDest().toString() + "' may contain unencrypted data from $@.",
+  source, "user input (" + source.getSourceType() + ")"

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
+}