Merge pull request #4828 from yoff/yoff-python-add-source-nodes

tausbn · web-flow · commit 75cfec863fe0 · 2021-01-05T15:07:51.000+01:00
Python: add source nodes
diff --git a/python/ql/src/semmle/python/Concepts.qll b/python/ql/src/semmle/python/Concepts.qll
@@ -338,7 +338,7 @@ module HTTP {
         /** Gets the URL pattern for this route, if it can be statically determined. */
         string getUrlPattern() {
           exists(StrConst str |
-            DataFlow::localFlow(DataFlow::exprNode(str), this.getUrlPatternArg()) and
+            DataFlow::exprNode(str).(DataFlow::LocalSourceNode).flowsTo(this.getUrlPatternArg()) and
             result = str.getText()
           )
         }
@@ -405,7 +405,9 @@ module HTTP {
         /** Gets the mimetype of this HTTP response, if it can be statically determined. */
         string getMimetype() {
           exists(StrConst str |
-            DataFlow::localFlow(DataFlow::exprNode(str), this.getMimetypeOrContentTypeArg()) and
+            DataFlow::exprNode(str)
+                .(DataFlow::LocalSourceNode)
+                .flowsTo(this.getMimetypeOrContentTypeArg()) and
             result = str.getText().splitAt(";", 0)
           )
           or
diff --git a/python/ql/src/semmle/python/Exprs.qll b/python/ql/src/semmle/python/Exprs.qll
@@ -584,18 +584,40 @@ class Slice extends Slice_ {
   }
 }
 
+/**
+ * Returns all string prefixes in the database that are explicitly marked as Unicode strings.
+ *
+ * Helper predicate for `StrConst::isUnicode`.
+ */
+pragma[nomagic]
+private string unicode_prefix() {
+  result = any(Str_ s).getPrefix() and
+  result.charAt(_) in ["u", "U"]
+}
+
+/**
+ * Returns all string prefixes in the database that are _not_ explicitly marked as bytestrings.
+ *
+ * Helper predicate for `StrConst::isUnicode`.
+ */
+pragma[nomagic]
+private string non_byte_prefix() {
+  result = any(Str_ s).getPrefix() and
+  not result.charAt(_) in ["b", "B"]
+}
+
 /** A string constant. */
 class StrConst extends Str_, ImmutableLiteral {
   /* syntax: "hello" */
   predicate isUnicode() {
-    this.getPrefix().charAt(_) = "u"
-    or
-    this.getPrefix().charAt(_) = "U"
-    or
-    not this.getPrefix().charAt(_) = "b" and major_version() = 3
+    this.getPrefix() = unicode_prefix()
     or
-    not this.getPrefix().charAt(_) = "b" and
-    this.getEnclosingModule().hasFromFuture("unicode_literals")
+    this.getPrefix() = non_byte_prefix() and
+    (
+      major_version() = 3
+      or
+      this.getEnclosingModule().hasFromFuture("unicode_literals")
+    )
   }
 
   deprecated override string strValue() { result = this.getS() }
diff --git a/python/ql/src/semmle/python/dataflow/new/TypeTracker.qll b/python/ql/src/semmle/python/dataflow/new/TypeTracker.qll
@@ -51,7 +51,7 @@ module StepSummary {
    * heap and/or inter-procedural step from `nodeFrom` to `nodeTo`.
    */
   cached
-  predicate step(Node nodeFrom, Node nodeTo, StepSummary summary) {
+  predicate step(LocalSourceNode nodeFrom, Node nodeTo, StepSummary summary) {
     exists(Node mid | typePreservingStep*(nodeFrom, mid) and smallstep(mid, nodeTo, summary))
   }
 
@@ -82,9 +82,8 @@ module StepSummary {
 
 /** Holds if it's reasonable to expect the data flow step from `nodeFrom` to `nodeTo` to preserve types. */
 private predicate typePreservingStep(Node nodeFrom, Node nodeTo) {
-  EssaFlow::essaFlowStep(nodeFrom, nodeTo) or
-  jumpStep(nodeFrom, nodeTo) or
-  nodeFrom = nodeTo.(PostUpdateNode).getPreUpdateNode()
+  simpleLocalFlowStep(nodeFrom, nodeTo) or
+  jumpStep(nodeFrom, nodeTo)
 }
 
 /**
@@ -142,11 +141,11 @@ predicate returnStep(ReturnNode nodeFrom, Node nodeTo) {
  * function. This means we will track the fact that `x.attr` can have the type of `y` into the
  * assignment to `z` inside `bar`, even though this attribute write happens _after_ `bar` is called.
  */
-predicate basicStoreStep(Node nodeFrom, Node nodeTo, string attr) {
+predicate basicStoreStep(Node nodeFrom, LocalSourceNode nodeTo, string attr) {
   exists(AttrWrite a |
     a.mayHaveAttributeName(attr) and
     nodeFrom = a.getValue() and
-    simpleLocalFlowStep*(nodeTo, a.getObject())
+    nodeTo.flowsTo(a.getObject())
   )
 }
 
@@ -275,7 +274,7 @@ class TypeTracker extends TTypeTracker {
    * heap and/or inter-procedural step from `nodeFrom` to `nodeTo`.
    */
   pragma[inline]
-  TypeTracker step(Node nodeFrom, Node nodeTo) {
+  TypeTracker step(LocalSourceNode nodeFrom, Node nodeTo) {
     exists(StepSummary summary |
       StepSummary::step(nodeFrom, nodeTo, summary) and
       result = this.append(summary)
diff --git a/python/ql/src/semmle/python/dataflow/new/internal/Attributes.qll b/python/ql/src/semmle/python/dataflow/new/internal/Attributes.qll
@@ -30,8 +30,8 @@ abstract class AttrRef extends Node {
   predicate mayHaveAttributeName(string attrName) {
     attrName = this.getAttributeName()
     or
-    exists(Node nodeFrom |
-      localFlow(nodeFrom, this.getAttributeNameExpr()) and
+    exists(LocalSourceNode nodeFrom |
+      nodeFrom.flowsTo(this.getAttributeNameExpr()) and
       attrName = nodeFrom.asExpr().(StrConst).getText()
     )
   }
diff --git a/python/ql/src/semmle/python/dataflow/new/internal/DataFlowPrivate.qll b/python/ql/src/semmle/python/dataflow/new/internal/DataFlowPrivate.qll
@@ -186,6 +186,7 @@ module EssaFlow {
  * data flow. It is a strict subset of the `localFlowStep` predicate, as it
  * excludes SSA flow through instance fields.
  */
+cached
 predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   // If there is ESSA-flow out of a node `node`, we want flow
   // both out of `node` and any post-update node of `node`.
@@ -219,12 +220,9 @@ private predicate localEssaStep(EssaNode nodeFrom, EssaNode nodeTo) {
  * Holds if `result` is either `node`, or the post-update node for `node`.
  */
 private Node update(Node node) {
-  exists(PostUpdateNode pun |
-    node = pun.getPreUpdateNode() and
-    result = pun
-  )
-  or
   result = node
+  or
+  result.(PostUpdateNode).getPreUpdateNode() = node
 }
 
 // TODO: Make modules for these headings
diff --git a/python/ql/src/semmle/python/dataflow/new/internal/DataFlowPublic.qll b/python/ql/src/semmle/python/dataflow/new/internal/DataFlowPublic.qll
@@ -376,6 +376,19 @@ class BarrierGuard extends GuardNode {
   }
 }
 
+/**
+ * A data flow node that is a source of local flow. This includes things like
+ * - Expressions
+ * - Function parameters
+ */
+class LocalSourceNode extends Node {
+  LocalSourceNode() { not simpleLocalFlowStep(_, this) }
+
+  /** Holds if this `LocalSourceNode` can flow to `nodeTo` in one or more local flow steps. */
+  cached
+  predicate flowsTo(Node nodeTo) { simpleLocalFlowStep*(this, nodeTo) }
+}
+
 /**
  * Algebraic datatype for tracking data content associated with values.
  * Content can be collection elements or object attributes.
diff --git a/python/ql/src/semmle/python/dataflow/new/internal/DataFlowUtil.qll b/python/ql/src/semmle/python/dataflow/new/internal/DataFlowUtil.qll
@@ -46,7 +46,7 @@ Node importNode(string name) {
       or
       name = alias.getValue().(ImportExpr).getImportedModuleName()
     ) and
-    result.(EssaNode).getVar().(AssignmentDefinition).getSourceVariable() = var
+    result.asExpr() = alias.getValue()
   )
   or
   // Although it may seem superfluous to consider the `foo` part of `from foo import bar as baz` to
diff --git a/python/ql/src/semmle/python/frameworks/Django.qll b/python/ql/src/semmle/python/frameworks/Django.qll
@@ -1841,7 +1841,7 @@ private module Django {
 
     DjangoRouteRegex() {
       this instanceof StrConst and
-      DataFlow::localFlow(DataFlow::exprNode(this), rePathCall.getUrlPatternArg())
+      DataFlow::exprNode(this).(DataFlow::LocalSourceNode).flowsTo(rePathCall.getUrlPatternArg())
     }
 
     DjangoRegexRouteSetup getRouteSetup() { result = rePathCall }
diff --git a/python/ql/src/semmle/python/frameworks/Flask.qll b/python/ql/src/semmle/python/frameworks/Flask.qll
@@ -319,9 +319,9 @@ private module FlaskModel {
     }
 
     override Function getARouteHandler() {
-      exists(DataFlow::Node view_func_arg, DataFlow::Node func_src |
+      exists(DataFlow::Node view_func_arg, DataFlow::LocalSourceNode func_src |
         view_func_arg.asCfgNode() in [node.getArg(2), node.getArgByName("view_func")] and
-        DataFlow::localFlow(func_src, view_func_arg) and
+        func_src.flowsTo(view_func_arg) and
         func_src.asExpr().(CallableExpr) = result.getDefinition()
       )
     }
diff --git a/python/ql/src/semmle/python/objects/TObject.qll b/python/ql/src/semmle/python/objects/TObject.qll
@@ -229,23 +229,32 @@ predicate class_method(
   PointsToInternal::pointsTo(instantiation.getArg(0), context, function, _)
 }
 
+/**
+ * Holds if the literal corresponding to the control flow node `n` has class `cls`.
+ *
+ * Helper predicate for `literal_instantiation`. Prevents a bad join with
+ * `PointsToContext::appliesTo` from occuring.
+ */
+pragma[nomagic]
+private predicate literal_node_class(ControlFlowNode n, ClassObjectInternal cls) {
+  n instanceof ListNode and cls = ObjectInternal::builtin("list")
+  or
+  n instanceof DictNode and cls = ObjectInternal::builtin("dict")
+  or
+  n instanceof SetNode and cls = ObjectInternal::builtin("set")
+  or
+  n.getNode() instanceof ImaginaryLiteral and cls = ObjectInternal::builtin("complex")
+  or
+  n.getNode() instanceof ListComp and cls = ObjectInternal::builtin("list")
+  or
+  n.getNode() instanceof SetComp and cls = ObjectInternal::builtin("set")
+  or
+  n.getNode() instanceof DictComp and cls = ObjectInternal::builtin("dict")
+}
+
 predicate literal_instantiation(ControlFlowNode n, ClassObjectInternal cls, PointsToContext context) {
   context.appliesTo(n) and
-  (
-    n instanceof ListNode and cls = ObjectInternal::builtin("list")
-    or
-    n instanceof DictNode and cls = ObjectInternal::builtin("dict")
-    or
-    n instanceof SetNode and cls = ObjectInternal::builtin("set")
-    or
-    n.getNode() instanceof ImaginaryLiteral and cls = ObjectInternal::builtin("complex")
-    or
-    n.getNode() instanceof ListComp and cls = ObjectInternal::builtin("list")
-    or
-    n.getNode() instanceof SetComp and cls = ObjectInternal::builtin("set")
-    or
-    n.getNode() instanceof DictComp and cls = ObjectInternal::builtin("dict")
-  )
+  literal_node_class(n, cls)
 }
 
 predicate super_instantiation(
diff --git a/python/ql/src/semmle/python/regex.qll b/python/ql/src/semmle/python/regex.qll
@@ -19,6 +19,19 @@ private predicate re_module_function(string name, int flags) {
   name = "subn" and flags = 4
 }
 
+/**
+ * Gets the names and corresponding values of attributes of the `re` module that are likely to be
+ * methods taking regular expressions as arguments.
+ *
+ * This is a helper predicate that fixes a bad join order, and should not be inlined without checking
+ * that this is safe.
+ */
+pragma[nomagic]
+private Value relevant_re_attr(string name) {
+  result = Module::named("re").attr(name) and
+  name != "escape"
+}
+
 /**
  * Holds if `s` is used as a regex with the `re` module, with the regex-mode `mode` (if known).
  * If regex mode is not known, `mode` will be `"None"`.
@@ -28,8 +41,7 @@ predicate used_as_regex(Expr s, string mode) {
   /* Call to re.xxx(regex, ... [mode]) */
   exists(CallNode call, string name |
     call.getArg(0).pointsTo(_, _, s.getAFlowNode()) and
-    call.getFunction().pointsTo(Module::named("re").attr(name)) and
-    not name = "escape"
+    call.getFunction().pointsTo(relevant_re_attr(name))
   |
     mode = "None"
     or
diff --git a/python/ql/src/semmle/python/types/Builtins.qll b/python/ql/src/semmle/python/types/Builtins.qll
@@ -104,7 +104,8 @@ class Builtin extends @py_cobject {
     ) and
     exists(string quoted_string |
       quoted_string = this.getName() and
-      result = quoted_string.regexpCapture("[bu]'([\\s\\S]*)'", 1)
+      // Remove prefix ("b" or "u") and leading and trailing quotes (both "'").
+      result = quoted_string.substring(2, quoted_string.length() - 1)
     )
   }
 }
diff --git a/python/ql/test/experimental/dataflow/import-helper/ImportHelper.expected b/python/ql/test/experimental/dataflow/import-helper/ImportHelper.expected
@@ -1,27 +1,23 @@
 | test1.py:1:8:1:12 | ControlFlowNode for ImportExpr | mypkg |
-| test1.py:1:8:1:12 | GSSA Variable mypkg | mypkg |
 | test2.py:1:6:1:10 | ControlFlowNode for ImportExpr | mypkg |
 | test2.py:1:6:1:10 | ControlFlowNode for ImportExpr | mypkg |
-| test2.py:1:19:1:21 | GSSA Variable foo | mypkg.foo |
-| test2.py:1:24:1:26 | GSSA Variable bar | mypkg.bar |
+| test2.py:1:19:1:21 | ControlFlowNode for ImportMember | mypkg.foo |
+| test2.py:1:24:1:26 | ControlFlowNode for ImportMember | mypkg.bar |
+| test3.py:1:8:1:16 | ControlFlowNode for ImportExpr | mypkg |
 | test3.py:1:8:1:16 | ControlFlowNode for ImportExpr | mypkg.foo |
+| test3.py:2:8:2:16 | ControlFlowNode for ImportExpr | mypkg |
 | test3.py:2:8:2:16 | ControlFlowNode for ImportExpr | mypkg.bar |
-| test3.py:2:8:2:16 | GSSA Variable mypkg | mypkg |
 | test4.py:1:8:1:16 | ControlFlowNode for ImportExpr | mypkg.foo |
-| test4.py:1:21:1:24 | GSSA Variable _foo | mypkg.foo |
 | test4.py:2:8:2:16 | ControlFlowNode for ImportExpr | mypkg.bar |
-| test4.py:2:21:2:24 | GSSA Variable _bar | mypkg.bar |
 | test5.py:1:8:1:12 | ControlFlowNode for ImportExpr | mypkg |
-| test5.py:1:8:1:12 | GSSA Variable mypkg | mypkg |
 | test5.py:9:6:9:10 | ControlFlowNode for ImportExpr | mypkg |
-| test5.py:9:26:9:29 | GSSA Variable _bar | mypkg.bar |
+| test5.py:9:19:9:29 | ControlFlowNode for ImportMember | mypkg.bar |
 | test6.py:1:8:1:12 | ControlFlowNode for ImportExpr | mypkg |
-| test6.py:1:8:1:12 | GSSA Variable mypkg | mypkg |
+| test6.py:5:8:5:16 | ControlFlowNode for ImportExpr | mypkg |
 | test6.py:5:8:5:16 | ControlFlowNode for ImportExpr | mypkg.foo |
-| test6.py:5:8:5:16 | GSSA Variable mypkg | mypkg |
 | test7.py:1:6:1:10 | ControlFlowNode for ImportExpr | mypkg |
-| test7.py:1:19:1:21 | GSSA Variable foo | mypkg.foo |
+| test7.py:1:19:1:21 | ControlFlowNode for ImportMember | mypkg.foo |
+| test7.py:5:8:5:16 | ControlFlowNode for ImportExpr | mypkg |
 | test7.py:5:8:5:16 | ControlFlowNode for ImportExpr | mypkg.foo |
-| test7.py:5:8:5:16 | GSSA Variable mypkg | mypkg |
 | test7.py:9:6:9:10 | ControlFlowNode for ImportExpr | mypkg |
-| test7.py:9:19:9:21 | GSSA Variable foo | mypkg.foo |
+| test7.py:9:19:9:21 | ControlFlowNode for ImportMember | mypkg.foo |

Original file line number	Diff line number	Diff line change
`@@ -338,7 +338,7 @@ module HTTP {`
`338`	`338`	`/** Gets the URL pattern for this route, if it can be statically determined. */`
`339`	`339`	`string getUrlPattern() {`
`340`	`340`	`exists(StrConst str \|`
`341`		`- DataFlow::localFlow(DataFlow::exprNode(str), this.getUrlPatternArg()) and`
	`341`	`+ DataFlow::exprNode(str).(DataFlow::LocalSourceNode).flowsTo(this.getUrlPatternArg()) and`
`342`	`342`	`result = str.getText()`
`343`	`343`	`)`
`344`	`344`	`}`
`@@ -405,7 +405,9 @@ module HTTP {`
`405`	`405`	`/** Gets the mimetype of this HTTP response, if it can be statically determined. */`
`406`	`406`	`string getMimetype() {`
`407`	`407`	`exists(StrConst str \|`
`408`		`- DataFlow::localFlow(DataFlow::exprNode(str), this.getMimetypeOrContentTypeArg()) and`
	`408`	`+ DataFlow::exprNode(str)`
	`409`	`+ .(DataFlow::LocalSourceNode)`
	`410`	`+ .flowsTo(this.getMimetypeOrContentTypeArg()) and`
`409`	`411`	`result = str.getText().splitAt(";", 0)`
`410`	`412`	`)`
`411`	`413`	`or`
Original file line number	Diff line number	Diff line change
`@@ -30,8 +30,8 @@ abstract class AttrRef extends Node {`
`30`	`30`	`predicate mayHaveAttributeName(string attrName) {`
`31`	`31`	`attrName = this.getAttributeName()`
`32`	`32`	`or`
`33`		`- exists(Node nodeFrom \|`
`34`		`- localFlow(nodeFrom, this.getAttributeNameExpr()) and`
	`33`	`+ exists(LocalSourceNode nodeFrom \|`
	`34`	`+ nodeFrom.flowsTo(this.getAttributeNameExpr()) and`
`35`	`35`	`attrName = nodeFrom.asExpr().(StrConst).getText()`
`36`	`36`	`)`
`37`	`37`	`}`
Original file line number	Diff line number	Diff line change
`@@ -46,7 +46,7 @@ Node importNode(string name) {`
`46`	`46`	`or`
`47`	`47`	`name = alias.getValue().(ImportExpr).getImportedModuleName()`
`48`	`48`	`) and`
`49`		`- result.(EssaNode).getVar().(AssignmentDefinition).getSourceVariable() = var`
	`49`	`+ result.asExpr() = alias.getValue()`
`50`	`50`	`)`
`51`	`51`	`or`
`52`	`52`	// Although it may seem superfluous to consider the `foo` part of `from foo import bar as baz` to
Original file line number	Diff line number	Diff line change
`@@ -1841,7 +1841,7 @@ private module Django {`
`1841`	`1841`
`1842`	`1842`	`DjangoRouteRegex() {`
`1843`	`1843`	`this instanceof StrConst and`
`1844`		`- DataFlow::localFlow(DataFlow::exprNode(this), rePathCall.getUrlPatternArg())`
	`1844`	`+ DataFlow::exprNode(this).(DataFlow::LocalSourceNode).flowsTo(rePathCall.getUrlPatternArg())`
`1845`	`1845`	`}`
`1846`	`1846`
`1847`	`1847`	`DjangoRegexRouteSetup getRouteSetup() { result = rePathCall }`
Original file line number	Diff line number	Diff line change
`@@ -319,9 +319,9 @@ private module FlaskModel {`
`319`	`319`	`}`
`320`	`320`
`321`	`321`	`override Function getARouteHandler() {`
`322`		`- exists(DataFlow::Node view_func_arg, DataFlow::Node func_src \|`
	`322`	`+ exists(DataFlow::Node view_func_arg, DataFlow::LocalSourceNode func_src \|`
`323`	`323`	`view_func_arg.asCfgNode() in [node.getArg(2), node.getArgByName("view_func")] and`
`324`		`- DataFlow::localFlow(func_src, view_func_arg) and`
	`324`	`+ func_src.flowsTo(view_func_arg) and`
`325`	`325`	`func_src.asExpr().(CallableExpr) = result.getDefinition()`
`326`	`326`	`)`
`327`	`327`	`}`