Merge pull request #2700 from RasmusWL/python-taint-iterable-unpacking

Python: Handle iterable unpacking in taint tracking

Author: tausbn

python/ql/src/semmle/python/Flow.qll

@@ -753,9 +753,8 @@ class DefinitionNode extends ControlFlowNode {
         or
         augstore(_, this)
         or
-        exists(Assign a | a.getATarget().(Tuple).getAnElt().getAFlowNode() = this)
-        or
-        exists(Assign a | a.getATarget().(List).getAnElt().getAFlowNode() = this)
+        // `x, y = 1, 2` where LHS is a combination of list or tuples
+        exists(Assign a | list_or_tuple_nested_element(a.getATarget()).getAFlowNode() = this)
         or
         exists(For for | for.getTarget().getAFlowNode() = this)
     }
@@ -768,6 +767,18 @@ class DefinitionNode extends ControlFlowNode {
     }
 }
 
+private Expr list_or_tuple_nested_element(Expr list_or_tuple) {
+    exists(Expr elt |
+        elt = list_or_tuple.(Tuple).getAnElt()
+        or
+        elt = list_or_tuple.(List).getAnElt()
+    |
+        result = elt
+        or
+        result = list_or_tuple_nested_element(elt)
+    )
+}
+
 /** A control flow node corresponding to a deletion statement, such as `del x`.
  * There can be multiple `DeletionNode`s for each `Delete` such that each
  * target has own `DeletionNode`. The CFG for `del a, x.y` looks like:
@@ -887,18 +898,38 @@ private AstNode assigned_value(Expr lhs) {
     /* lhs += x  =>  result = (lhs + x) */
     exists(AugAssign a, BinaryExpr b | b = a.getOperation() and result = b and lhs = b.getLeft())
     or
-    /* ..., lhs, ... = ..., result, ... */
-    exists(Assign a, Tuple target, Tuple values, int index |
-        a.getATarget() = target and
-        a.getValue() = values and
-        lhs = target.getElt(index) and
-        result = values.getElt(index)
-    )
+    /* ..., lhs, ... = ..., result, ...
+     * or
+     * ..., (..., lhs, ...), ... = ..., (..., result, ...), ...
+    */
+    exists(Assign a | nested_sequence_assign(a.getATarget(), a.getValue(), lhs, result))
     or
     /* for lhs in seq: => `result` is the `for` node, representing the `iter(next(seq))` operation. */
     result.(For).getTarget() = lhs
 }
 
+predicate nested_sequence_assign(Expr left_parent, Expr right_parent,
+        Expr left_result, Expr right_result) {
+    exists(int i, Expr left_elem, Expr right_elem
+    |
+        (
+            left_elem = left_parent.(Tuple).getElt(i)
+            or
+            left_elem = left_parent.(List).getElt(i)
+        )
+        and
+        (
+            right_elem = right_parent.(Tuple).getElt(i)
+            or
+            right_elem = right_parent.(List).getElt(i)
+        )
+    |
+        left_result = left_elem and right_result = right_elem
+        or
+        nested_sequence_assign(left_elem, right_elem, left_result, right_result)
+    )
+}
+
 /** A flow node for a `for` statement. */
 class ForNode extends ControlFlowNode {
 
@@ -1037,6 +1068,17 @@ class NameConstantNode extends NameNode {
     */
 }
 
+/** A control flow node correspoinding to a starred expression, `*a`. */
+class StarredNode extends ControlFlowNode {
+    StarredNode() {
+        toAst(this) instanceof Starred
+    }
+
+    ControlFlowNode getValue() {
+        toAst(result) = toAst(this).(Starred).getValue()
+    }
+}
+
 private module Scopes {
 
     private predicate fast_local(NameNode n) {

python/ql/src/semmle/python/dataflow/Configuration.qll

@@ -4,7 +4,6 @@ private import semmle.python.objects.ObjectInternal
 private import semmle.python.dataflow.Implementation
 
 module TaintTracking {
-
     class Source = TaintSource;
 
     class Sink = TaintSink;
@@ -16,13 +15,11 @@ module TaintTracking {
     class PathSink = TaintTrackingNode;
 
     abstract class Configuration extends string {
-
         /* Required to prevent compiler warning */
         bindingset[this]
         Configuration() { this = this }
 
         /* Old implementation API */
-
         predicate isSource(Source src) { none() }
 
         predicate isSink(Sink sink) { none() }
@@ -32,7 +29,6 @@ module TaintTracking {
         predicate isExtension(Extension extension) { none() }
 
         /* New implementation API */
-
         /**
          * Holds if `src` is a source of taint of `kind` that is relevant
          * for this configuration.
@@ -66,7 +62,9 @@ module TaintTracking {
         /**
          * Holds if `src -> dest` is a flow edge converting taint from `srckind` to `destkind`.
          */
-        predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node dest, TaintKind srckind, TaintKind destkind) {
+        predicate isAdditionalFlowStep(
+            DataFlow::Node src, DataFlow::Node dest, TaintKind srckind, TaintKind destkind
+        ) {
             none()
         }
 
@@ -79,9 +77,7 @@ module TaintTracking {
          * Holds if `node` should be considered as a barrier to flow of `kind`.
          */
         predicate isBarrier(DataFlow::Node node, TaintKind kind) {
-            exists(Sanitizer sanitizer |
-                this.isSanitizer(sanitizer)
-                |
+            exists(Sanitizer sanitizer | this.isSanitizer(sanitizer) |
                 sanitizer.sanitizingNode(kind, node.asCfgNode())
                 or
                 sanitizer.sanitizingEdge(kind, node.asVariable())
@@ -112,16 +108,18 @@ module TaintTracking {
          * Holds if flow from `src` to `dest` is prohibited when the incoming taint is `srckind` and the outgoing taint is `destkind`.
          * Note that `srckind` and `destkind` can be the same.
          */
-        predicate isBarrierEdge(DataFlow::Node src, DataFlow::Node dest, TaintKind srckind, TaintKind destkind) { none() }
+        predicate isBarrierEdge(
+            DataFlow::Node src, DataFlow::Node dest, TaintKind srckind, TaintKind destkind
+        ) {
+            none()
+        }
 
         /* Common query API */
-
         predicate hasFlowPath(PathSource src, PathSink sink) {
             this.(TaintTrackingImplementation).hasFlowPath(src, sink)
         }
 
         /* Old query API */
-
         /* deprecated */
         deprecated predicate hasFlow(Source src, Sink sink) {
             exists(PathSource psrc, PathSink psink |
@@ -132,15 +130,12 @@ module TaintTracking {
         }
 
         /* New query API */
-
         predicate hasSimpleFlow(DataFlow::Node src, DataFlow::Node sink) {
             exists(PathSource psrc, PathSink psink |
                 this.hasFlowPath(psrc, psink) and
                 src = psrc.getNode() and
                 sink = psink.getNode()
             )
         }
-
     }
-
 }

python/ql/src/semmle/python/dataflow/DataFlow.qll

@@ -1 +1 @@
-import semmle.python.security.TaintTracking
+import semmle.python.security.TaintTracking

python/ql/src/semmle/python/dataflow/Files.qll

@@ -2,25 +2,18 @@ import python
 import semmle.python.security.TaintTracking
 
 class OpenFile extends TaintKind {
-
     OpenFile() { this = "file.open" }
 
     override string repr() { result = "an open file" }
-
-
 }
 
 class OpenFileConfiguration extends TaintTracking::Configuration {
-
-    OpenFileConfiguration() {  this = "Open file configuration" }
+    OpenFileConfiguration() { this = "Open file configuration" }
 
     override predicate isSource(DataFlow::Node src, TaintKind kind) {
         theOpenFunction().(FunctionObject).getACall() = src.asCfgNode() and
         kind instanceof OpenFile
     }
 
-    override predicate isSink(DataFlow::Node sink, TaintKind kind) {
-        none()
-    }
-
+    override predicate isSink(DataFlow::Node sink, TaintKind kind) { none() }
 }