Merge pull request github#13738 from RasmusWL/path-steps

Python: Include all assignments in data flow paths

Author: yoff

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll

@@ -513,15 +513,21 @@ class CastNode extends Node {
  * explanations.
  */
 predicate neverSkipInPathGraph(Node n) {
-  // We include read- and store steps here to force them to be
-  // shown in path explanations.
-  // This hack is necessary, because we have included some of these
-  // steps as default taint steps, making them be suppressed in path
-  // explanations.
-  // We should revert this once, we can remove this steps from the
-  // default taint steps; this should be possible once we have
-  // implemented flow summaries and recursive content.
-  readStep(_, _, n) or storeStep(_, _, n)
+  // NOTE: We could use RHS of a definition, but since we have use-use flow, in an
+  // example like
+  // ```py
+  // x = SOURCE()
+  // if <cond>:
+  //     y = x
+  // SINK(x)
+  // ```
+  // we would end up saying that the path MUST not skip the x in `y = x`, which is just
+  // annoying and doesn't help the path explanation become clearer.
+  n.asVar() instanceof EssaDefinition and
+  // For a parameter we have flow from ControlFlowNode to SSA node, and then onwards
+  // with use-use flow, and since the CFN is already part of the path graph, we don't
+  // want to force showing the SSA node as well.
+  not n.asVar() instanceof ParameterDefinition
 }
 
 /**

python/ql/src/change-notes/2023-08-17-improved-path-graph.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Updated path explanations for `@kind path-problem` queries to always include left hand side of assignments, making paths easier to understand.

python/ql/test/experimental/dataflow/basic/globalStep.expected

@@ -1,6 +1,5 @@
 | test.py:1:1:1:21 | ControlFlowNode for FunctionExpr | test.py:1:5:1:17 | GSSA Variable obfuscated_id |
 | test.py:1:1:1:21 | ControlFlowNode for FunctionExpr | test.py:1:5:1:17 | GSSA Variable obfuscated_id |
-| test.py:1:1:1:21 | ControlFlowNode for FunctionExpr | test.py:7:5:7:17 | ControlFlowNode for obfuscated_id |
 | test.py:1:5:1:17 | GSSA Variable obfuscated_id | test.py:7:5:7:17 | ControlFlowNode for obfuscated_id |
 | test.py:1:19:1:19 | ControlFlowNode for x | test.py:1:19:1:19 | SSA variable x |
 | test.py:1:19:1:19 | ControlFlowNode for x | test.py:1:19:1:19 | SSA variable x |
@@ -14,18 +13,6 @@
 | test.py:1:19:1:19 | ControlFlowNode for x | test.py:2:7:2:7 | ControlFlowNode for x |
 | test.py:1:19:1:19 | ControlFlowNode for x | test.py:2:7:2:7 | ControlFlowNode for x |
 | test.py:1:19:1:19 | ControlFlowNode for x | test.py:2:7:2:7 | ControlFlowNode for x |
-| test.py:1:19:1:19 | ControlFlowNode for x | test.py:3:3:3:3 | SSA variable z |
-| test.py:1:19:1:19 | ControlFlowNode for x | test.py:3:3:3:3 | SSA variable z |
-| test.py:1:19:1:19 | ControlFlowNode for x | test.py:3:3:3:3 | SSA variable z |
-| test.py:1:19:1:19 | ControlFlowNode for x | test.py:3:3:3:3 | SSA variable z |
-| test.py:1:19:1:19 | ControlFlowNode for x | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:1:19:1:19 | ControlFlowNode for x | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:1:19:1:19 | ControlFlowNode for x | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:1:19:1:19 | ControlFlowNode for x | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:1:19:1:19 | ControlFlowNode for x | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:1:19:1:19 | ControlFlowNode for x | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:1:19:1:19 | ControlFlowNode for x | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:1:19:1:19 | ControlFlowNode for x | test.py:4:10:4:10 | ControlFlowNode for z |
 | test.py:1:19:1:19 | SSA variable x | test.py:2:3:2:3 | SSA variable y |
 | test.py:1:19:1:19 | SSA variable x | test.py:2:3:2:3 | SSA variable y |
 | test.py:1:19:1:19 | SSA variable x | test.py:2:3:2:3 | SSA variable y |
@@ -34,18 +21,6 @@
 | test.py:1:19:1:19 | SSA variable x | test.py:2:7:2:7 | ControlFlowNode for x |
 | test.py:1:19:1:19 | SSA variable x | test.py:2:7:2:7 | ControlFlowNode for x |
 | test.py:1:19:1:19 | SSA variable x | test.py:2:7:2:7 | ControlFlowNode for x |
-| test.py:1:19:1:19 | SSA variable x | test.py:3:3:3:3 | SSA variable z |
-| test.py:1:19:1:19 | SSA variable x | test.py:3:3:3:3 | SSA variable z |
-| test.py:1:19:1:19 | SSA variable x | test.py:3:3:3:3 | SSA variable z |
-| test.py:1:19:1:19 | SSA variable x | test.py:3:3:3:3 | SSA variable z |
-| test.py:1:19:1:19 | SSA variable x | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:1:19:1:19 | SSA variable x | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:1:19:1:19 | SSA variable x | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:1:19:1:19 | SSA variable x | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:1:19:1:19 | SSA variable x | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:1:19:1:19 | SSA variable x | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:1:19:1:19 | SSA variable x | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:1:19:1:19 | SSA variable x | test.py:4:10:4:10 | ControlFlowNode for z |
 | test.py:2:3:2:3 | SSA variable y | test.py:3:3:3:3 | SSA variable z |
 | test.py:2:3:2:3 | SSA variable y | test.py:3:3:3:3 | SSA variable z |
 | test.py:2:3:2:3 | SSA variable y | test.py:3:3:3:3 | SSA variable z |
@@ -54,26 +29,10 @@
 | test.py:2:3:2:3 | SSA variable y | test.py:3:7:3:7 | ControlFlowNode for y |
 | test.py:2:3:2:3 | SSA variable y | test.py:3:7:3:7 | ControlFlowNode for y |
 | test.py:2:3:2:3 | SSA variable y | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:2:3:2:3 | SSA variable y | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:2:3:2:3 | SSA variable y | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:2:3:2:3 | SSA variable y | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:2:3:2:3 | SSA variable y | test.py:4:10:4:10 | ControlFlowNode for z |
 | test.py:2:7:2:7 | ControlFlowNode for x | test.py:2:3:2:3 | SSA variable y |
 | test.py:2:7:2:7 | ControlFlowNode for x | test.py:2:3:2:3 | SSA variable y |
 | test.py:2:7:2:7 | ControlFlowNode for x | test.py:2:3:2:3 | SSA variable y |
 | test.py:2:7:2:7 | ControlFlowNode for x | test.py:2:3:2:3 | SSA variable y |
-| test.py:2:7:2:7 | ControlFlowNode for x | test.py:3:3:3:3 | SSA variable z |
-| test.py:2:7:2:7 | ControlFlowNode for x | test.py:3:3:3:3 | SSA variable z |
-| test.py:2:7:2:7 | ControlFlowNode for x | test.py:3:3:3:3 | SSA variable z |
-| test.py:2:7:2:7 | ControlFlowNode for x | test.py:3:3:3:3 | SSA variable z |
-| test.py:2:7:2:7 | ControlFlowNode for x | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:2:7:2:7 | ControlFlowNode for x | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:2:7:2:7 | ControlFlowNode for x | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:2:7:2:7 | ControlFlowNode for x | test.py:3:7:3:7 | ControlFlowNode for y |
-| test.py:2:7:2:7 | ControlFlowNode for x | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:2:7:2:7 | ControlFlowNode for x | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:2:7:2:7 | ControlFlowNode for x | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:2:7:2:7 | ControlFlowNode for x | test.py:4:10:4:10 | ControlFlowNode for z |
 | test.py:3:3:3:3 | SSA variable z | test.py:4:10:4:10 | ControlFlowNode for z |
 | test.py:3:3:3:3 | SSA variable z | test.py:4:10:4:10 | ControlFlowNode for z |
 | test.py:3:3:3:3 | SSA variable z | test.py:4:10:4:10 | ControlFlowNode for z |
@@ -82,18 +41,12 @@
 | test.py:3:7:3:7 | ControlFlowNode for y | test.py:3:3:3:3 | SSA variable z |
 | test.py:3:7:3:7 | ControlFlowNode for y | test.py:3:3:3:3 | SSA variable z |
 | test.py:3:7:3:7 | ControlFlowNode for y | test.py:3:3:3:3 | SSA variable z |
-| test.py:3:7:3:7 | ControlFlowNode for y | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:3:7:3:7 | ControlFlowNode for y | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:3:7:3:7 | ControlFlowNode for y | test.py:4:10:4:10 | ControlFlowNode for z |
-| test.py:3:7:3:7 | ControlFlowNode for y | test.py:4:10:4:10 | ControlFlowNode for z |
 | test.py:4:10:4:10 | ControlFlowNode for z | test.py:7:5:7:20 | ControlFlowNode for obfuscated_id() |
 | test.py:4:10:4:10 | ControlFlowNode for z | test.py:7:5:7:20 | ControlFlowNode for obfuscated_id() |
 | test.py:6:1:6:1 | GSSA Variable a | test.py:7:19:7:19 | ControlFlowNode for a |
 | test.py:6:1:6:1 | GSSA Variable a | test.py:7:19:7:19 | ControlFlowNode for a |
 | test.py:6:5:6:6 | ControlFlowNode for IntegerLiteral | test.py:6:1:6:1 | GSSA Variable a |
 | test.py:6:5:6:6 | ControlFlowNode for IntegerLiteral | test.py:6:1:6:1 | GSSA Variable a |
-| test.py:6:5:6:6 | ControlFlowNode for IntegerLiteral | test.py:7:19:7:19 | ControlFlowNode for a |
-| test.py:6:5:6:6 | ControlFlowNode for IntegerLiteral | test.py:7:19:7:19 | ControlFlowNode for a |
 | test.py:7:5:7:20 | ControlFlowNode for obfuscated_id() | test.py:7:1:7:1 | GSSA Variable b |
 | test.py:7:19:7:19 | ControlFlowNode for a | test.py:1:19:1:19 | ControlFlowNode for x |
 | test.py:7:19:7:19 | ControlFlowNode for a | test.py:1:19:1:19 | ControlFlowNode for x |

python/ql/test/experimental/dataflow/path-graph/PathNodes.expected

@@ -0,0 +1,2 @@
+failures
+testFailures

python/ql/test/experimental/dataflow/path-graph/PathNodes.ql

@@ -0,0 +1,34 @@
+/**
+ * @kind path-problem
+ */
+
+import python
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.dataflow.new.TaintTracking
+import experimental.dataflow.testConfig
+import TestUtilities.InlineExpectationsTest
+
+module TestTaintFlow = TaintTracking::Global<TestConfig>;
+
+module PathNodeTest implements TestSig {
+  string getARelevantTag() { result = "path-node" }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    exists(TestTaintFlow::PathNode pn |
+      location = pn.getNode().getLocation() and
+      tag = "path-node" and
+      value = "" and
+      element = pn.toString()
+    )
+  }
+}
+
+import MakeTest<PathNodeTest>
+// running the query to inspect the results can be quite nice!
+// just uncomment these lines!
+// import TestTaintFlow::PathGraph
+// from TestTaintFlow::PathNode source, TestTaintFlow::PathNode sink
+// where TestTaintFlow::flowPath(source, sink)
+// select sink.getNode(), source, sink,
+//   sink.getNode().getEnclosingCallable().toString() + ": --> " +
+//     sink.getNode().getLocation().getStartLine().toString()

python/ql/test/experimental/dataflow/path-graph/test.py

@@ -0,0 +1,96 @@
+def assign():
+    x = SOURCE # $ path-node
+
+    y = x # $ path-node
+
+    SINK(y) # $ path-node
+
+
+def aug_assign():
+    x = SOURCE # $ path-node
+    z = ""
+
+    z += x # $ path-node
+
+    SINK(z) # $ path-node
+
+
+def dont_use_rhs(cond):
+    # like noted in the original Ruby PR: https://github.com/github/codeql/pull/12566
+    x = SOURCE # $ path-node
+
+    if cond:
+        y = x
+
+    SINK(x) # $ path-node
+
+
+def flow_through_function():
+    def identify(x): # $ path-node
+        return x # $ path-node
+
+    x = SOURCE # $ path-node
+
+    y = identify(x) # $ path-node
+
+    SINK(y) # $ path-node
+
+
+def attribute():
+    class X: pass
+    x = X()
+    x.attr = SOURCE # $ path-node
+
+    y = x # $ path-node
+
+    SINK(y.attr) # $ path-node
+
+
+def list_loop():
+    x = SOURCE # $ path-node
+    l = list()
+
+    l.append(x) # $ path-node
+
+    for y in l: # $ path-node
+
+        SINK(y) # $ path-node
+
+
+def list_index():
+    x = SOURCE # $ path-node
+    l = list()
+
+    l.append(x) # $ path-node
+
+    z = l[0] # $ path-node
+
+    SINK(z) # $ path-node
+
+
+def test_tuple():
+    x = SOURCE # $ path-node
+
+    y = ((x, 1), 2) # $ path-node
+
+    (z, _), _ = y # $ path-node
+
+    SINK(z) # $ path-node
+
+
+def test_with():
+    x = SOURCE # $ path-node
+
+    with x as y: # $ path-node
+
+        SINK(y) # $ path-node
+
+
+def test_match():
+    x = SOURCE # $ path-node
+
+    match x:
+
+        case y: # $ path-node
+
+            SINK(y) # $ path-node