Merge pull request github#7807 from RasmusWL/dataflow-improvements

Python: Dataflow improvements

Author: yoff

python/ql/lib/change-notes/2022-02-04-improve-field-flow.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Improved analysis of attributes for data-flow and taint tracking queries, so `getattr`/`setattr` are supported, and a write to an attribute properly stops flow for the old value in that attribute.
+* Added post-update nodes (`DataFlow::PostUpdateNode`) for arguments in calls that can't be resolved.

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

@@ -204,6 +204,8 @@ abstract class AttrRead extends AttrRef, Node, LocalSourceNode { }
 private class AttributeReadAsAttrRead extends AttrRead, CfgNode {
   override AttrNode node;
 
+  AttributeReadAsAttrRead() { node.isLoad() }
+
   override Node getObject() { result.asCfgNode() = node.getObject() }
 
   override ExprNode getAttributeNameExpr() {

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

@@ -126,7 +126,7 @@ module syntheticPostUpdateNode {
    * Certain arguments, such as implicit self arguments are already post-update nodes
    * and should not have an extra node synthesised.
    */
-  ArgumentNode argumentPreUpdateNode() {
+  Node argumentPreUpdateNode() {
     result = any(FunctionCall c).getArg(_)
     or
     // Avoid argument 0 of method calls as those have read post-update nodes.
@@ -136,6 +136,11 @@ module syntheticPostUpdateNode {
     or
     // Avoid argument 0 of class calls as those have non-synthetic post-update nodes.
     exists(ClassCall c, int n | n > 0 | result = c.getArg(n))
+    or
+    // any argument of any call that we have not been able to resolve
+    exists(CallNode call | not call = any(DataFlowCall c).getNode() |
+      result.(CfgNode).getNode() in [call.getArg(_), call.getArgByName(_)]
+    )
   }
 
   /** An object might have its value changed after a store. */
@@ -704,7 +709,7 @@ newtype TDataFlowCall =
   TFunctionCall(CallNode call) { call = any(FunctionValue f).getAFunctionCall() } or
   /** Bound methods need to make room for the explicit self parameter */
   TMethodCall(CallNode call) { call = any(FunctionValue f).getAMethodCall() } or
-  TClassCall(CallNode call) { call = any(ClassValue c).getACall() } or
+  TClassCall(CallNode call) { call = any(ClassValue c | not c.isAbsent()).getACall() } or
   TSpecialCall(SpecialMethodCallNode special)
 
 /** Represents a call. */
@@ -1067,19 +1072,18 @@ predicate comprehensionStoreStep(CfgNode nodeFrom, Content c, CfgNode nodeTo) {
 }
 
 /**
- * Holds if `nodeFrom` flows into an attribute (corresponding to `c`) of `nodeTo` via an attribute assignment.
+ * Holds if `nodeFrom` flows into the attribute `c` of `nodeTo` via an attribute assignment.
  *
  * For example, in
  * ```python
  * obj.foo = x
  * ```
- * data flows from `x` to (the post-update node for) `obj` via assignment to `foo`.
+ * data flows from `x` to the attribute `foo` of  (the post-update node for) `obj`.
  */
-predicate attributeStoreStep(CfgNode nodeFrom, AttributeContent c, PostUpdateNode nodeTo) {
-  exists(AttrNode attr |
-    nodeFrom.asCfgNode() = attr.(DefinitionNode).getValue() and
-    attr.getName() = c.getAttribute() and
-    attr.getObject() = nodeTo.getPreUpdateNode().(CfgNode).getNode()
+predicate attributeStoreStep(Node nodeFrom, AttributeContent c, PostUpdateNode nodeTo) {
+  exists(AttrWrite write |
+    write.accesses(nodeTo.getPreUpdateNode(), c.getAttribute()) and
+    nodeFrom = write.getValue()
   )
 }
 
@@ -1923,21 +1927,16 @@ pragma[noinline]
 TupleElementContent small_tuple() { result.getIndex() <= 7 }
 
 /**
- * Holds if `nodeTo` is a read of an attribute (corresponding to `c`) of the object in `nodeFrom`.
+ * Holds if `nodeTo` is a read of the attribute `c` of the object `nodeFrom`.
  *
- * For example, in
+ * For example
  * ```python
  * obj.foo
  * ```
- * data flows from `obj` to `obj.foo` via a read from `foo`.
+ * is a read of the attribute `foo` from the object `obj`.
  */
-predicate attributeReadStep(CfgNode nodeFrom, AttributeContent c, CfgNode nodeTo) {
-  exists(AttrNode attr |
-    nodeFrom.asCfgNode() = attr.getObject() and
-    nodeTo.asCfgNode() = attr and
-    attr.getName() = c.getAttribute() and
-    attr.isLoad()
-  )
+predicate attributeReadStep(Node nodeFrom, AttributeContent c, AttrRead nodeTo) {
+  nodeTo.accesses(nodeFrom, c.getAttribute())
 }
 
 /**
@@ -1973,6 +1972,18 @@ predicate clearsContent(Node n, Content c) {
   kwOverflowClearStep(n, c)
   or
   matchClearStep(n, c)
+  or
+  attributeClearStep(n, c)
+}
+
+/**
+ * Holds if values stored inside attribute `c` are cleared at node `n`.
+ *
+ * In `obj.foo = x` any old value stored in `foo` is cleared at the pre-update node
+ * associated with `obj`
+ */
+predicate attributeClearStep(Node n, AttributeContent c) {
+  exists(PostUpdateNode post | post.getPreUpdateNode() = n | attributeStoreStep(_, c, post))
 }
 
 //--------

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

@@ -9,6 +9,7 @@
 
 private import python
 private import semmle.python.dataflow.new.DataFlow
+private import semmle.python.dataflow.new.internal.DataFlowPrivate as DataFlowPrivate
 
 /**
  * INTERNAL: Do not use.
@@ -66,7 +67,12 @@ string prettyNodeForInlineTest(DataFlow::Node node) {
     result = "[post]" + prettyExpr(e)
   )
   or
+  exists(Expr e | e = node.(DataFlowPrivate::SyntheticPreUpdateNode).getPostUpdateNode().asExpr() |
+    result = "[pre]" + prettyExpr(e)
+  )
+  or
   not exists(node.asExpr()) and
   not exists(node.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr()) and
+  not exists(node.(DataFlowPrivate::SyntheticPreUpdateNode).getPostUpdateNode().asExpr()) and
   result = node.toString()
 }

python/ql/test/experimental/attrs/AttrReads.expected

@@ -0,0 +1,2 @@
+| test.py:10:1:10:9 | ControlFlowNode for Attribute | test.py:10:1:10:5 | ControlFlowNode for myobj | foo |
+| test.py:13:1:13:21 | ControlFlowNode for getattr() | test.py:13:9:13:13 | ControlFlowNode for myobj | foo |

python/ql/test/experimental/attrs/AttrReads.ql

@@ -0,0 +1,5 @@
+import python
+private import semmle.python.dataflow.new.DataFlow
+
+from DataFlow::AttrRead read
+select read, read.getObject(), read.getAttributeName()

python/ql/test/experimental/attrs/AttrWrites.expected

@@ -0,0 +1,4 @@
+| test.py:5:9:5:16 | ControlFlowNode for __init__ | test.py:4:1:4:20 | ControlFlowNode for ClassExpr | __init__ | test.py:5:5:5:28 | ControlFlowNode for FunctionExpr |
+| test.py:6:9:6:16 | ControlFlowNode for Attribute | test.py:6:9:6:12 | ControlFlowNode for self | foo | test.py:6:20:6:22 | ControlFlowNode for foo |
+| test.py:9:1:9:9 | ControlFlowNode for Attribute | test.py:9:1:9:5 | ControlFlowNode for myobj | foo | test.py:9:13:9:17 | ControlFlowNode for Str |
+| test.py:12:1:12:25 | ControlFlowNode for setattr() | test.py:12:9:12:13 | ControlFlowNode for myobj | foo | test.py:12:23:12:24 | ControlFlowNode for IntegerLiteral |

python/ql/test/experimental/attrs/AttrWrites.ql

@@ -0,0 +1,5 @@
+import python
+private import semmle.python.dataflow.new.DataFlow
+
+from DataFlow::AttrWrite write
+select write, write.getObject(), write.getAttributeName(), write.getValue()

python/ql/test/experimental/attrs/test.py

@@ -0,0 +1,13 @@
+# This file is a simple test of which nodes are included with AttrRead/AttrWrite.
+# For actual data-flow tests, see fieldflow/ dir.
+
+class MyObj(object):
+    def __init__(self, foo):
+        self.foo = foo
+
+myobj = MyObj("foo")
+myobj.foo = "bar"
+myobj.foo
+
+setattr(myobj, "foo", 42)
+getattr(myobj, "foo")

python/ql/test/experimental/dataflow/TestUtil/NormalDataflowTest.qll

@@ -0,0 +1,33 @@
+import python
+import experimental.dataflow.TestUtil.FlowTest
+import experimental.dataflow.testConfig
+private import semmle.python.dataflow.new.internal.PrintNode
+
+class DataFlowTest extends FlowTest {
+  DataFlowTest() { this = "DataFlowTest" }
+
+  override string flowTag() { result = "flow" }
+
+  override predicate relevantFlow(DataFlow::Node source, DataFlow::Node sink) {
+    exists(TestConfiguration cfg | cfg.hasFlow(source, sink))
+  }
+}
+
+query predicate missingAnnotationOnSINK(Location location, string error, string element) {
+  error = "ERROR, you should add `# $ MISSING: flow` annotation" and
+  exists(DataFlow::Node sink |
+    exists(DataFlow::CallCfgNode call |
+      // note: we only care about `SINK` and not `SINK_F`, so we have to reconstruct manually.
+      call.getFunction().asCfgNode().(NameNode).getId() = "SINK" and
+      (sink = call.getArg(_) or sink = call.getArgByName(_))
+    ) and
+    location = sink.getLocation() and
+    element = prettyExpr(sink.asExpr()) and
+    not any(TestConfiguration config).hasFlow(_, sink) and
+    not exists(FalseNegativeExpectation missingResult |
+      missingResult.getTag() = "flow" and
+      missingResult.getLocation().getFile() = location.getFile() and
+      missingResult.getLocation().getStartLine() = location.getStartLine()
+    )
+  )
+}