Merge pull request #1707 from asger-semmle/canonical-name-call-graph

Approved by xiemaisi

change-notes/1.23/analysis-javascript.md

@@ -5,6 +5,8 @@
 * Support for the following frameworks and libraries has been improved:
   - [firebase](https://www.npmjs.com/package/firebase)
 
+* The call graph has been improved to resolve method calls in more cases. This may produce more security alerts.
+
 ## New queries
 
 | **Query**                                                                 | **Tags**                                                          | **Purpose**                                                                                                                                                                            |

javascript/externs/es/es5.js

@@ -236,20 +236,13 @@ Date.prototype.toISOString = function() {};
 Date.prototype.toJSON = function(opt_ignoredKey) {};
 
 
-/**
- * A fake type to model the JSON object.
- * @constructor
- */
-function JSONType() {}
-
-
 /**
  * @param {string} jsonStr The string to parse.
  * @param {(function(string, *) : *)=} opt_reviver
  * @return {*} The JSON object.
  * @throws {Error}
  */
-JSONType.prototype.parse = function(jsonStr, opt_reviver) {};
+JSON.parse = function(jsonStr, opt_reviver) {};
 
 
 /**
@@ -259,11 +252,4 @@ JSONType.prototype.parse = function(jsonStr, opt_reviver) {};
  * @return {string} JSON string which represents jsonObj.
  * @throws {Error}
  */
-JSONType.prototype.stringify = function(jsonObj, opt_replacer, opt_space) {};
-
-
-/**
- * @type {!JSONType}
- * @suppress {duplicate}
- */
-var JSON;
+JSON.stringify = function(jsonObj, opt_replacer, opt_space) {};

javascript/ql/src/semmle/javascript/Closure.qll

@@ -212,45 +212,34 @@ module Closure {
       or
       name = namespace
     )
+    or
+    name = "goog" // The closure libraries themselves use the "goog" namespace
+  }
+
+  /**
+   * Holds if a prefix of `name` is a closure namespace.
+   */
+  bindingset[name]
+  private predicate hasClosureNamespacePrefix(string name) {
+    isClosureNamespace(name.substring(0, name.indexOf(".")))
+    or
+    isClosureNamespace(name)
   }
 
   /**
    * Gets the closure namespace path addressed by the given data flow node, if any.
    */
   string getClosureNamespaceFromSourceNode(DataFlow::SourceNode node) {
-    isClosureNamespace(result) and
-    node = DataFlow::globalVarRef(result)
-    or
-    exists(DataFlow::SourceNode base, string basePath, string prop |
-      basePath = getClosureNamespaceFromSourceNode(base) and
-      node = base.getAPropertyRead(prop) and
-      result = basePath + "." + prop and
-      // ensure finiteness
-      (
-        isClosureNamespace(basePath)
-        or
-        // direct access, no indirection
-        node.(DataFlow::PropRead).getBase() = base
-      )
-    )
-    or
-    // Associate an access path with the immediate RHS of a store on a closure namespace.
-    // This is to support patterns like:
-    // foo.bar = { baz() {} }
-    exists(DataFlow::PropWrite write |
-      node = write.getRhs() and
-      result = getWrittenClosureNamespace(write)
-    )
-    or
-    result = node.(ClosureNamespaceAccess).getClosureNamespace()
+    result = GlobalAccessPath::getAccessPath(node) and
+    hasClosureNamespacePrefix(result)
   }
 
   /**
    * Gets the closure namespace path written to by the given property write, if any.
    */
   string getWrittenClosureNamespace(DataFlow::PropWrite node) {
-    result = getClosureNamespaceFromSourceNode(node.getBase().getALocalSource()) + "." +
-        node.getPropertyName()
+    result = GlobalAccessPath::fromRhs(node.getRhs()) and
+    hasClosureNamespacePrefix(result)
   }
 
   /**

javascript/ql/src/semmle/javascript/GlobalAccessPaths.qll

@@ -41,6 +41,7 @@ module GlobalAccessPath {
    * })(NS = NS || {});
    * ``` 
    */
+  cached
   string fromReference(DataFlow::Node node) {
     result = fromReference(node.getImmediatePredecessor())
     or
@@ -142,6 +143,7 @@ module GlobalAccessPath {
    *  })(foo = foo || {});
    * ```
    */
+  cached
   string fromRhs(DataFlow::Node node) {
     exists(DataFlow::SourceNode base, string baseName, string name |
       node = base.getAPropertyWrite(name).getRhs() and
@@ -152,9 +154,9 @@ module GlobalAccessPath {
       baseName = fromRhs(base)
     )
     or
-    exists(AssignExpr assign |
-      node = assign.getRhs().flow() and
-      result = assign.getLhs().(GlobalVarAccess).getName()
+    exists(GlobalVariable var |
+      node = var.getAnAssignedExpr().flow() and
+      result = var.getName()
     )
     or
     exists(FunctionDeclStmt fun |
@@ -166,6 +168,16 @@ module GlobalAccessPath {
       node = DataFlow::valueNode(cls) and
       result = cls.getIdentifier().(GlobalVarDecl).getName()
     )
+    or
+    exists(EnumDeclaration decl |
+      node = DataFlow::valueNode(decl) and
+      result = decl.getIdentifier().(GlobalVarDecl).getName()
+    )
+    or
+    exists(NamespaceDeclaration decl |
+      node = DataFlow::valueNode(decl) and
+      result = decl.getId().(GlobalVarDecl).getName()
+    )
   }
 
   /**

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

@@ -43,7 +43,8 @@ module DataFlow {
     } or
     THtmlAttributeNode(HTML::Attribute attr) or
     TExceptionalFunctionReturnNode(Function f) or
-    TExceptionalInvocationReturnNode(InvokeExpr e)
+    TExceptionalInvocationReturnNode(InvokeExpr e) or
+    TGlobalAccessPathRoot()
 
   /**
    * A node in the data flow graph.
@@ -120,6 +121,12 @@ module DataFlow {
     /** Gets a function value that may reach this node. */
     FunctionNode getAFunctionValue() {
       result.getAstNode() = analyze().getAValue().(AbstractCallable).getFunction()
+      or
+      exists(string name |
+        GlobalAccessPath::isAssignedInUniqueFile(name) and
+        GlobalAccessPath::fromRhs(result) = name and
+        GlobalAccessPath::fromReference(this) = name
+      )
     }
 
     /**
@@ -912,6 +919,20 @@ module DataFlow {
     DataFlow::InvokeNode getInvocation() { result = invoke.flow() }
   }
 
+  /**
+   * A pseudo-node representing the root of a global access path.
+   */
+  private class GlobalAccessPathRoot extends TGlobalAccessPathRoot, DataFlow::Node {
+    override string toString() { result = "global access path" }
+  }
+
+  /**
+   * INTERNAL. DO NOT USE.
+   *
+   * Gets a pseudo-node representing the root of a global access path.
+   */
+  DataFlow::Node globalAccessPathRootPseudoNode() { result instanceof TGlobalAccessPathRoot }
+
   /**
    * Provides classes representing various kinds of calls.
    *

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

@@ -693,6 +693,7 @@ class ClassNode extends DataFlow::SourceNode {
   /**
    * Gets a dataflow node that refers to this class object.
    */
+  cached
   DataFlow::SourceNode getAClassReference() {
     result = getAClassReference(DataFlow::TypeTracker::end())
   }
@@ -709,6 +710,15 @@ class ClassNode extends DataFlow::SourceNode {
     t.start() and
     result = getAReceiverNode()
     or
+    // Use a parameter type as starting point of type tracking.
+    // Use `t.call()` to emulate the value being passed in through an unseen
+    // call site, but not out of the call again.
+    t.call() and
+    exists(Parameter param |
+      this = param.getTypeAnnotation().getClass() and
+      result = DataFlow::parameterNode(param)
+    )
+    or
     result = getAnInstanceReferenceAux(t) and
     // Avoid tracking into the receiver of other classes.
     // Note that this also blocks flows into a property of the receiver,
@@ -724,6 +734,7 @@ class ClassNode extends DataFlow::SourceNode {
   /**
    * Gets a dataflow node that refers to an instance of this class.
    */
+  cached
   DataFlow::SourceNode getAnInstanceReference() {
     result = getAnInstanceReference(DataFlow::TypeTracker::end())
   }
@@ -844,6 +855,12 @@ module ClassNode {
     override DataFlow::Node getASuperClassNode() { result = astNode.getSuperClass().flow() }
   }
 
+  private DataFlow::PropRef getAPrototypeReferenceInFile(string name, File f) {
+    GlobalAccessPath::getAccessPath(result.getBase()) = name and
+    result.getPropertyName() = "prototype" and
+    result.getFile() = f
+  }
+
   /**
    * A function definition with prototype manipulation as a `ClassNode` instance.
    */
@@ -854,9 +871,16 @@ module ClassNode {
 
     FunctionStyleClass() {
       function.getFunction() = astNode and
-      exists(DataFlow::PropRef read |
-        read.getPropertyName() = "prototype" and
-        read.getBase().analyze().getAValue() = function
+      (
+        exists (DataFlow::PropRef read |
+          read.getPropertyName() = "prototype" and
+          read.getBase().analyze().getAValue() = function
+        )
+        or
+        exists(string name |
+          name = GlobalAccessPath::fromRhs(this) and
+          exists(getAPrototypeReferenceInFile(name, getFile()))
+        )
       )
     }
 
@@ -916,11 +940,16 @@ module ClassNode {
         result = base.getAPropertyRead("prototype")
         or
         result = base.getAPropertySource("prototype")
-        or
-        exists(ExtendCall call |
-          call.getDestinationOperand() = base.getAPropertyRead("prototype") and
-          result = call.getASourceOperand()
-        )
+      )
+      or
+      exists(string name |
+        GlobalAccessPath::fromRhs(this) = name and
+        result = getAPrototypeReferenceInFile(name, getFile())
+      )
+      or
+      exists(ExtendCall call |
+        call.getDestinationOperand() = getAPrototypeReference() and
+        result = call.getASourceOperand()
       )
     }
 

javascript/ql/src/semmle/javascript/dataflow/Sources.qll

@@ -250,6 +250,8 @@ module SourceNode {
       DataFlow::thisNode(this, _)
       or
       this = DataFlow::destructuredModuleImportNode(_)
+      or
+      this = DataFlow::globalAccessPathRootPseudoNode()
     }
   }
 }

javascript/ql/src/semmle/javascript/dataflow/TypeInference.qll

@@ -47,14 +47,6 @@ class AnalyzedNode extends DataFlow::Node {
    */
   AnalyzedNode localFlowPred() { result = getAPredecessor() }
 
-  /**
-   * INTERNAL. Do not use.
-   *
-   * Gets another data flow node whose value flows into this node in a global step
-   * (this is, involving global variables).
-   */
-  AnalyzedNode globalFlowPred() { none() }
-
   /**
    * Gets an abstract value that this node may evaluate to at runtime.
    *
@@ -92,9 +84,6 @@ class AnalyzedNode extends DataFlow::Node {
     exists(DataFlow::Incompleteness cause |
       isIncomplete(cause) and result = TIndefiniteAbstractValue(cause)
     )
-    or
-    result = globalFlowPred().getALocalValue() and
-    shouldTrackGlobally(result)
   }
 
   /** Gets a type inferred for this node. */
@@ -295,8 +284,3 @@ private class AnalyzedAsyncFunction extends AnalyzedFunction {
 
   override AbstractValue getAReturnValue() { result = TAbstractOtherObject() }
 }
-
-/**
- * Holds if the given value should be propagated along `globalFlowPred()` edges.
- */
-private predicate shouldTrackGlobally(AbstractValue value) { value instanceof AbstractCallable }

javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll

@@ -10,7 +10,11 @@ private import javascript
 private import internal.FlowSteps
 
 private class PropertyName extends string {
-  PropertyName() { this = any(DataFlow::PropRef pr).getPropertyName() }
+  PropertyName() {
+    this = any(DataFlow::PropRef pr).getPropertyName()
+    or
+    GlobalAccessPath::isAssignedInUniqueFile(this)
+  }
 }
 
 private class OptionalPropertyName extends string {
@@ -89,6 +93,20 @@ module StepSummary {
     or
     any(AdditionalTypeTrackingStep st).step(pred, succ) and
     summary = LevelStep()
+    or
+    exists(string name |
+      name = GlobalAccessPath::fromRhs(pred) and
+      GlobalAccessPath::isAssignedInUniqueFile(name) and
+      succ = DataFlow::globalAccessPathRootPseudoNode() and
+      summary = StoreStep(name)
+    )
+    or
+    exists(string name |
+      name = GlobalAccessPath::fromReference(succ) and
+      GlobalAccessPath::isAssignedInUniqueFile(name) and
+      pred = DataFlow::globalAccessPathRootPseudoNode() and
+      summary = LoadStep(name)
+    )
   }
 }
 
@@ -158,6 +176,12 @@ class TypeTracker extends TTypeTracker {
    */
   predicate start() { hasCall = false and prop = "" }
 
+  /**
+   * Holds if this is the starting point of type tracking
+   * when tracking a parameter into a call, but not out of it.
+   */
+  predicate call() { hasCall = true and prop = "" }
+
   /**
    * Holds if this is the end point of type tracking.
    */

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll

@@ -119,12 +119,30 @@ private module CachedSteps {
   predicate calls(DataFlow::InvokeNode invk, Function f) {
     f = invk.getACallee(0)
     or
-    exists(DataFlow::ClassNode cls, string name |
-      callResolvesToMember(invk, cls, name) and
-      f = cls.getInstanceMethod(name).getFunction()
+    exists(DataFlow::ClassNode cls |
+      // Call to class member
+      exists(string name |
+        callResolvesToMember(invk, cls, name) and
+        f = cls.getInstanceMethod(name).getFunction()
+        or
+        invk = cls.getAClassReference().getAMethodCall(name) and
+        f = cls.getStaticMethod(name).getFunction()
+      )
       or
-      invk = cls.getAClassReference().getAMethodCall(name) and
-      f = cls.getStaticMethod(name).getFunction()
+      // Call to constructor
+      invk = cls.getAClassReference().getAnInvocation() and
+      f = cls.getConstructor().getFunction()
+      or
+      // Super call to constructor
+      invk.asExpr().(SuperCall).getBinder() = cls.getConstructor().getFunction() and
+      f = cls.getADirectSuperClass().getConstructor().getFunction()
+    )
+    or
+    // Call from `foo.bar.baz()` to `foo.bar.baz = function()`
+    exists(string name |
+      GlobalAccessPath::isAssignedInUniqueFile(name) and
+      GlobalAccessPath::fromRhs(f.flow()) = name and
+      GlobalAccessPath::fromReference(invk.getCalleeNode()) = name
     )
   }
 

javascript/ql/src/semmle/javascript/dataflow/internal/InterModuleTypeInference.qll

@@ -394,13 +394,6 @@ private class AnalyzedClosureGlobalAccessPath extends AnalyzedNode, AnalyzedProp
     accessPath = Closure::getClosureNamespaceFromSourceNode(this)
   }
 
-  override AnalyzedNode globalFlowPred() {
-    exists(DataFlow::PropWrite write |
-      Closure::getWrittenClosureNamespace(write) = accessPath and
-      result = write.getRhs()
-    )
-  }
-
   override predicate reads(AbstractValue base, string propName) {
     exists(Closure::ClosureModule mod |
       mod.getClosureNamespace() = accessPath and

javascript/ql/test/library-tests/CallGraphs/FullTest/tests.expected

@@ -114,6 +114,7 @@ test_getAFunctionValue
 | tst3.js:2:1:2:23 | functio ... n2() {} | tst3.js:2:1:2:23 | functio ... n2() {} |
 | tst.js:1:1:1:15 | function f() {} | tst.js:1:1:1:15 | function f() {} |
 | tst.js:2:9:2:21 | function() {} | tst.js:2:9:2:21 | function() {} |
+| tst.js:3:1:3:1 | h | tst.js:3:5:3:17 | function() {} |
 | tst.js:3:1:3:17 | h = function() {} | tst.js:3:5:3:17 | function() {} |
 | tst.js:3:5:3:17 | function() {} | tst.js:3:5:3:17 | function() {} |
 | tst.js:4:1:4:5 | k = g | tst.js:2:9:2:21 | function() {} |

javascript/ql/test/library-tests/Closure/CallGraph.ql

@@ -1,4 +1,6 @@
 import javascript
+import semmle.javascript.dataflow.internal.FlowSteps
 
-from DataFlow::InvokeNode node, int imprecision
-select node, node.getACallee(imprecision), imprecision
+from DataFlow::InvokeNode node, Function callee
+where calls(node, callee)
+select node, callee, 0

javascript/ql/test/library-tests/Closure/moduleImport.expected

@@ -1,6 +1,5 @@
 | foo.bar | tests/nestedAccess.js:3:14:3:36 | goog.re ... o.bar') |
 | foo.bar.x | tests/nestedAccess.js:5:1:5:8 | fooBar.x |
-| foo.bar.x | tests/nestedAccess.js:10:9:10:11 | z.x |
 | foo.bar.x.y | tests/nestedAccess.js:5:1:5:10 | fooBar.x.y |
 | foo.bar.x.y.z | tests/nestedAccess.js:5:1:5:12 | fooBar.x.y.z |
 | goog | tests/es6Module.js:1:1:1:4 | goog |

javascript/ql/test/library-tests/DataFlow/noBasicBlock.expected

@@ -1,3 +1,4 @@
+| file://:0:0:0:0 | global access path |
 | tst.js:1:10:1:11 | fs |
 | tst.js:1:10:1:11 | fs |
 | tst.js:1:20:1:23 | 'fs' |

javascript/ql/test/library-tests/DataFlow/sources.expected

@@ -3,6 +3,7 @@
 | eval.js:1:1:5:1 | functio ... eval`\\n} |
 | eval.js:3:3:3:6 | eval |
 | eval.js:3:3:3:16 | eval("x = 23") |
+| file://:0:0:0:0 | global access path |
 | sources.js:1:1:1:0 | this |
 | sources.js:1:1:1:12 | new (x => x) |
 | sources.js:1:6:1:6 | x |

javascript/ql/test/library-tests/TypeTracking/ClassStyle.expected

@@ -2,6 +2,13 @@ test_ApiObject
 | tst.js:4:11:4:21 | new myapi() |
 | tst.js:16:10:16:21 | api.chain1() |
 | tst.js:16:10:16:30 | api.cha ... hain2() |
+| tst.js:62:40:62:51 | api.chain1() |
+| tst.js:62:40:62:60 | api.cha ... hain2() |
+| tst.js:63:38:63:49 | api.chain1() |
+| tst.js:63:38:63:58 | api.cha ... hain2() |
+| tst_conflict.js:3:11:3:21 | new myapi() |
+| tst_conflict.js:6:38:6:49 | api.chain1() |
+| tst_conflict.js:6:38:6:58 | api.cha ... hain2() |
 test_Connection
 | tst.js:7:15:7:18 | conn |
 | tst.js:11:5:11:19 | this.connection |
@@ -12,6 +19,10 @@ test_Connection
 | tst.js:48:7:48:21 | getConnection() |
 | tst.js:54:37:54:51 | getConnection() |
 | tst.js:57:14:57:48 | config. ... ction') |
+| tst.js:62:40:62:79 | api.cha ... ction() |
+| tst.js:63:38:63:77 | api.cha ... ction() |
+| tst.js:67:14:67:47 | MyAppli ... nection |
+| tst_conflict.js:6:38:6:77 | api.cha ... ction() |
 test_DataCallback
 | tst.js:10:11:10:12 | cb |
 | tst.js:21:1:23:1 | functio ... ata);\\n} |
@@ -22,9 +33,11 @@ test_DataCallback
 | tst.js:45:19:45:20 | cb |
 | tst.js:48:32:48:60 | identit ... llback) |
 | tst.js:58:16:58:22 | x => {} |
+| tst.js:68:16:70:3 | data => ... a);\\n  } |
 test_DataValue
 | tst.js:21:18:21:21 | data |
 | tst.js:25:19:25:22 | data |
 | tst.js:33:17:33:20 | data |
 | tst.js:38:10:38:13 | data |
 | tst.js:58:16:58:16 | x |
+| tst.js:68:16:68:19 | data |

javascript/ql/test/library-tests/TypeTracking/PredicateStyle.expected

@@ -2,6 +2,13 @@ apiObject
 | tst.js:4:11:4:21 | new myapi() |
 | tst.js:16:10:16:21 | api.chain1() |
 | tst.js:16:10:16:30 | api.cha ... hain2() |
+| tst.js:62:40:62:51 | api.chain1() |
+| tst.js:62:40:62:60 | api.cha ... hain2() |
+| tst.js:63:38:63:49 | api.chain1() |
+| tst.js:63:38:63:58 | api.cha ... hain2() |
+| tst_conflict.js:3:11:3:21 | new myapi() |
+| tst_conflict.js:6:38:6:49 | api.chain1() |
+| tst_conflict.js:6:38:6:58 | api.cha ... hain2() |
 connection
 | type tracker with call steps | tst.js:7:15:7:18 | conn |
 | type tracker with call steps | tst.js:11:5:11:19 | this.connection |
@@ -13,6 +20,14 @@ connection
 | type tracker without call steps | tst.js:48:7:48:21 | getConnection() |
 | type tracker without call steps | tst.js:54:37:54:51 | getConnection() |
 | type tracker without call steps | tst.js:57:14:57:48 | config. ... ction') |
+| type tracker without call steps | tst.js:62:40:62:79 | api.cha ... ction() |
+| type tracker without call steps | tst.js:63:38:63:77 | api.cha ... ction() |
+| type tracker without call steps | tst.js:67:14:67:47 | MyAppli ... nection |
+| type tracker without call steps | tst_conflict.js:6:38:6:77 | api.cha ... ction() |
+| type tracker without call steps with property MyApplication.namespace.connection | file://:0:0:0:0 | global access path |
+| type tracker without call steps with property conflict | tst.js:63:3:63:25 | MyAppli ... mespace |
+| type tracker without call steps with property conflict | tst_conflict.js:6:3:6:25 | MyAppli ... mespace |
+| type tracker without call steps with property connection | tst.js:62:3:62:25 | MyAppli ... mespace |
 dataCallback
 | tst.js:10:11:10:12 | cb |
 | tst.js:21:1:23:1 | functio ... ata);\\n} |
@@ -23,9 +38,11 @@ dataCallback
 | tst.js:45:19:45:20 | cb |
 | tst.js:48:32:48:60 | identit ... llback) |
 | tst.js:58:16:58:22 | x => {} |
+| tst.js:68:16:70:3 | data => ... a);\\n  } |
 dataValue
 | tst.js:21:18:21:21 | data |
 | tst.js:25:19:25:22 | data |
 | tst.js:33:17:33:20 | data |
 | tst.js:38:10:38:13 | data |
 | tst.js:58:16:58:16 | x |
+| tst.js:68:16:68:19 | data |

javascript/ql/test/library-tests/TypeTracking/tst.js

@@ -57,3 +57,20 @@ function getFromConfigFramework() {
   let conn = config.getConfigValue('connection');
   conn.getData(x => {});
 }
+
+function initConnection() {
+  MyApplication.namespace.connection = api.chain1().chain2().createConnection();
+  MyApplication.namespace.conflict = api.chain1().chain2().createConnection();
+}
+
+function useConnection() {
+  let conn = MyApplication.namespace.connection;
+  conn.getData(data => {
+    useData(data);
+  });
+
+  let conflict = MyApplication.namespace.conflict;
+  conflict.getData(data => {
+    useData(data);
+  });
+}

javascript/ql/test/library-tests/TypeTracking/tst_conflict.js

@@ -0,0 +1,7 @@
+import myapi from "@test/myapi";
+
+let api = new myapi();
+
+function initConnection() {
+  MyApplication.namespace.conflict = api.chain1().chain2().createConnection();
+}