Python: Use LocalSourceNode throughout step

This commit does a lot of stuff all at once, so here are the main
highlights:

In `TypeTracker.qll`, we change `StepSummary::step` to step only between
source nodes. Because reads and writes of global variables happen in two
different (jump) steps, this requires the intermediate
`ModuleVariableNode` to _also_ be a `LocalSourceNode`, and we therefore
modify the charpred for that class accordingly. (This also means
changing a few of the tests to account for these new source nodes.)

In addition, we change `TypeTracker::step` to likewise step between
local source nodes.

Next, to enable the use of the `track` convenience method on nodes, we
add some pragmas to `TypeTracker::step` that prevent bad joins from
occurring. With this, we can eliminate all of the manual type tracker
join predicates.

Next, we observe that because `StepSummary::step` now uses `flowsTo`, it
automatically encapsulates all local-flow steps. In particular this
means we do not have to use `typePreservingStep` in `smallstep`, but can
use `jumpStep` directly. A similar observation applies to
`TypeTracker::smallstep`.

Having done this, we no longer need `typePreservingStep`, so we get rid
of it.

Author: tausbn

python/ql/src/Security/CVE-2018-1281/BindToAllInterfaces.ql

@@ -32,21 +32,7 @@ private DataFlow::LocalSourceNode vulnerableHostnameRef(DataFlow::TypeTracker t,
     result.asExpr() = allInterfacesStrConst
   )
   or
-  // Due to bad performance when using normal setup with `vulnerableHostnameRef(t2, hostname).track(t2, t)`
-  // we have inlined that code and forced a join
-  exists(DataFlow::TypeTracker t2 |
-    exists(DataFlow::StepSummary summary |
-      vulnerableHostnameRef_first_join(t2, hostname, result, summary) and
-      t = t2.append(summary)
-    )
-  )
-}
-
-pragma[nomagic]
-private predicate vulnerableHostnameRef_first_join(
-  DataFlow::TypeTracker t2, string hostname, DataFlow::Node res, DataFlow::StepSummary summary
-) {
-  DataFlow::StepSummary::step(vulnerableHostnameRef(t2, hostname), res, summary)
+  exists(DataFlow::TypeTracker t2 | result = vulnerableHostnameRef(t2, hostname).track(t2, t))
 }
 
 /** Gets a reference to a hostname that can be used to bind to all interfaces. */
@@ -59,21 +45,7 @@ private DataFlow::LocalSourceNode vulnerableAddressTuple(DataFlow::TypeTracker t
   t.start() and
   result.asExpr() = any(Tuple tup | tup.getElt(0) = vulnerableHostnameRef(hostname).asExpr())
   or
-  // Due to bad performance when using normal setup with `vulnerableAddressTuple(t2, hostname).track(t2, t)`
-  // we have inlined that code and forced a join
-  exists(DataFlow::TypeTracker t2 |
-    exists(DataFlow::StepSummary summary |
-      vulnerableAddressTuple_first_join(t2, hostname, result, summary) and
-      t = t2.append(summary)
-    )
-  )
-}
-
-pragma[nomagic]
-private predicate vulnerableAddressTuple_first_join(
-  DataFlow::TypeTracker t2, string hostname, DataFlow::Node res, DataFlow::StepSummary summary
-) {
-  DataFlow::StepSummary::step(vulnerableAddressTuple(t2, hostname), res, summary)
+  exists(DataFlow::TypeTracker t2 | result = vulnerableAddressTuple(t2, hostname).track(t2, t))
 }
 
 /** Gets a reference to a tuple for which the first element is a hostname that can be used to bind to all interfaces. */

python/ql/src/semmle/python/ApiGraphs.qll

@@ -433,18 +433,7 @@ module API {
       use(_, src) and
       result = src
       or
-      // Due to bad performance when using `trackUseNode(t2, attr_name).track(t2, t)`
-      // we have inlined that code and forced a join
-      exists(DataFlow::StepSummary summary |
-        t = trackUseNode_first_join(src, result, summary).append(summary)
-      )
-    }
-
-    pragma[nomagic]
-    private DataFlow::TypeTracker trackUseNode_first_join(
-      DataFlow::LocalSourceNode src, DataFlow::LocalSourceNode res, DataFlow::StepSummary summary
-    ) {
-      DataFlow::StepSummary::step(trackUseNode(src, result), res, summary)
+      exists(DataFlow::TypeTracker t2 | result = trackUseNode(src, t2).track(t2, t))
     }
 
     cached

python/ql/src/semmle/python/Concepts.qll

@@ -570,21 +570,7 @@ module Cryptography {
         arg = any(KeyGeneration::Range r).getKeySizeArg() and
         result = arg.getALocalSource()
         or
-        // Due to bad performance when using normal setup with we have inlined that code and forced a join
-        exists(DataFlow::TypeBackTracker t2 |
-          exists(DataFlow::StepSummary summary |
-            keysizeBacktracker_first_join(t2, arg, result, summary) and
-            t = t2.prepend(summary)
-          )
-        )
-      }
-
-      pragma[nomagic]
-      private predicate keysizeBacktracker_first_join(
-        DataFlow::TypeBackTracker t2, DataFlow::Node arg, DataFlow::Node res,
-        DataFlow::StepSummary summary
-      ) {
-        DataFlow::StepSummary::step(res, keysizeBacktracker(t2, arg), summary)
+        exists(DataFlow::TypeBackTracker t2 | result = keysizeBacktracker(t2, arg).backtrack(t2, t))
       }
 
       /** Gets a back-reference to the keysize argument `arg` that was used to generate a new key-pair. */

python/ql/src/semmle/python/dataflow/new/TypeTracker.qll

@@ -51,8 +51,8 @@ module StepSummary {
    * heap and/or inter-procedural step from `nodeFrom` to `nodeTo`.
    */
   cached
-  predicate step(LocalSourceNode nodeFrom, Node nodeTo, StepSummary summary) {
-    exists(Node mid | typePreservingStep*(nodeFrom, mid) and smallstep(mid, nodeTo, summary))
+  predicate step(LocalSourceNode nodeFrom, LocalSourceNode nodeTo, StepSummary summary) {
+    exists(Node mid | nodeFrom.flowsTo(mid) and smallstep(mid, nodeTo, summary))
   }
 
   /**
@@ -63,7 +63,7 @@ module StepSummary {
    * type-preserving steps.
    */
   predicate smallstep(Node nodeFrom, Node nodeTo, StepSummary summary) {
-    typePreservingStep(nodeFrom, nodeTo) and
+    jumpStep(nodeFrom, nodeTo) and
     summary = LevelStep()
     or
     callStep(nodeFrom, nodeTo) and summary = CallStep()
@@ -80,12 +80,6 @@ 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) {
-  simpleLocalFlowStep(nodeFrom, nodeTo) or
-  jumpStep(nodeFrom, nodeTo)
-}
-
 /**
  * Gets a callable for the call where `nodeFrom` is used as the `i`'th argument.
  *
@@ -274,10 +268,10 @@ class TypeTracker extends TTypeTracker {
    * heap and/or inter-procedural step from `nodeFrom` to `nodeTo`.
    */
   pragma[inline]
-  TypeTracker step(LocalSourceNode nodeFrom, Node nodeTo) {
+  TypeTracker step(LocalSourceNode nodeFrom, LocalSourceNode nodeTo) {
     exists(StepSummary summary |
-      StepSummary::step(nodeFrom, nodeTo, summary) and
-      result = this.append(summary)
+      StepSummary::step(nodeFrom, pragma[only_bind_out](nodeTo), pragma[only_bind_into](summary)) and
+      result = this.append(pragma[only_bind_into](summary))
     )
   }
 
@@ -312,7 +306,7 @@ class TypeTracker extends TTypeTracker {
       result = this.append(summary)
     )
     or
-    typePreservingStep(nodeFrom, nodeTo) and
+    simpleLocalFlowStep(nodeFrom, nodeTo) and
     result = this
   }
 }
@@ -453,7 +447,7 @@ class TypeBackTracker extends TTypeBackTracker {
       this = result.prepend(summary)
     )
     or
-    typePreservingStep(nodeFrom, nodeTo) and
+    simpleLocalFlowStep(nodeFrom, nodeTo) and
     this = result
   }
 }

python/ql/src/semmle/python/dataflow/new/internal/LocalSources.qll

@@ -41,7 +41,6 @@ class LocalSourceNode extends Node {
   cached
   LocalSourceNode() {
     not comes_from_cfgnode(this) and
-    not this instanceof ModuleVariableNode and
     // Currently, we create synthetic post-update nodes for
     // - arguments to calls that may modify said argument
     // - direct reads a writes of object attributes

python/ql/src/semmle/python/frameworks/Cryptography.qll

@@ -83,23 +83,11 @@ private module CryptographyModel {
       result.(DataFlow::CallCfgNode).getFunction() = curveClassWithKeySize(keySize) and
       origin = result
       or
-      // Due to bad performance when using normal setup with we have inlined that code and forced a join
       exists(DataFlow::TypeTracker t2 |
-        exists(DataFlow::StepSummary summary |
-          curveClassInstanceWithKeySize_first_join(t2, keySize, origin, result, summary) and
-          t = t2.append(summary)
-        )
+        result = curveClassInstanceWithKeySize(t2, keySize, origin).track(t2, t)
       )
     }
 
-    pragma[nomagic]
-    private predicate curveClassInstanceWithKeySize_first_join(
-      DataFlow::TypeTracker t2, int keySize, DataFlow::Node origin, DataFlow::Node res,
-      DataFlow::StepSummary summary
-    ) {
-      DataFlow::StepSummary::step(curveClassInstanceWithKeySize(t2, keySize, origin), res, summary)
-    }
-
     /** Gets a reference to a predefined curve class instance with a specific key size (in bits), as well as the origin of the class. */
     DataFlow::Node curveClassInstanceWithKeySize(int keySize, DataFlow::Node origin) {
       curveClassInstanceWithKeySize(DataFlow::TypeTracker::end(), keySize, origin).flowsTo(result)

python/ql/src/semmle/python/regex.qll

@@ -82,21 +82,7 @@ private DataFlow::LocalSourceNode re_flag_tracker(string flag_name, DataFlow::Ty
     result.asCfgNode() = binop
   )
   or
-  // Due to bad performance when using normal setup with `re_flag_tracker(t2, attr_name).track(t2, t)`
-  // we have inlined that code and forced a join
-  exists(DataFlow::TypeTracker t2 |
-    exists(DataFlow::StepSummary summary |
-      re_flag_tracker_first_join(t2, flag_name, result, summary) and
-      t = t2.append(summary)
-    )
-  )
-}
-
-pragma[nomagic]
-private predicate re_flag_tracker_first_join(
-  DataFlow::TypeTracker t2, string flag_name, DataFlow::Node res, DataFlow::StepSummary summary
-) {
-  DataFlow::StepSummary::step(re_flag_tracker(flag_name, t2), res, summary)
+  exists(DataFlow::TypeTracker t2 | result = re_flag_tracker(flag_name, t2).track(t2, t))
 }
 
 /**

python/ql/test/experimental/dataflow/ApiGraphs/use.ql

@@ -9,7 +9,11 @@ class ApiUseTest extends InlineExpectationsTest {
   override string getARelevantTag() { result = "use" }
 
   private predicate relevant_node(API::Node a, DataFlow::Node n, Location l) {
-    n = a.getAUse() and l = n.getLocation()
+    n = a.getAUse() and
+    l = n.getLocation() and
+    // Module variable nodes have no suitable location, so it's best to simply exclude them entirely
+    // from the inline tests.
+    not n instanceof DataFlow::ModuleVariableNode
   }
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {

python/ql/test/experimental/dataflow/typetracking/moduleattr.expected

@@ -1,6 +1,7 @@
 module_tracker
 | import_as_attr.py:1:6:1:11 | ControlFlowNode for ImportExpr |
 module_attr_tracker
+| import_as_attr.py:0:0:0:0 | ModuleVariableNode for Global Variable attr_ref in Module import_as_attr |
 | import_as_attr.py:1:20:1:35 | ControlFlowNode for ImportMember |
 | import_as_attr.py:1:28:1:35 | GSSA Variable attr_ref |
 | import_as_attr.py:3:1:3:1 | GSSA Variable x |