JS: Canonicalize ThisNode

Author: asger-semmle

javascript/ql/src/semmle/javascript/Expr.qll

@@ -303,6 +303,16 @@ class ThisExpr extends @thisexpr, Expr {
   Function getBinder() {
     result = getEnclosingFunction().getThisBinder()
   }
+
+  /**
+   * Gets the function or top-level whose `this` binding this expression refers to,
+   * which is the nearest enclosing non-arrow function or top-level.
+   */
+  StmtContainer getBindingContainer() {
+    result = getContainer().(Function).getThisBindingContainer()
+    or
+    result = getContainer().(TopLevel)
+  }
 }
 
 /** An array literal. */

javascript/ql/src/semmle/javascript/Functions.qll

@@ -206,6 +206,17 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
     result = this
   }
 
+  /**
+   * Gets the function or top-level whose `this` binding a `this` expression in this function refers to,
+   * which is the nearest enclosing non-arrow function or top-level.
+   */
+  StmtContainer getThisBindingContainer() {
+    result = getThisBinder()
+    or
+    not exists(getThisBinder()) and
+    result = getTopLevel()
+  }
+
   /**
    * Holds if this function has a mapped `arguments` variable whose indices are aliased
    * with the function's parameters.

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

@@ -32,6 +32,7 @@ module DataFlow {
   or TReflectiveCallNode(MethodCallExpr ce, string kind) {
        ce.getMethodName() = kind and (kind = "call" or kind = "apply")
      }
+  or TThisNode(StmtContainer f) { f.(Function).getThisBinder() = f or f instanceof TopLevel }
 
   /**
    * A node in the data flow graph.
@@ -867,6 +868,13 @@ module DataFlow {
     nd = TDestructuringPatternNode(p)
   }
 
+  /**
+   * INTERNAL: Use `thisNode(StmtContainer container)` instead.
+   */
+  predicate thisNode(DataFlow::Node node, StmtContainer container) {
+    node = TThisNode(container)
+  }
+
   /**
    * A classification of flows that are not modeled, or only modeled incompletely, by
    * `DataFlowNode`:
@@ -970,6 +978,11 @@ module DataFlow {
       pred = valueNode(defSourceNode(def)) and
       succ = TDestructuringPatternNode(def.getTarget())
     )
+    or
+    // flow from 'this' parameter into 'this' expressions
+    exists (ThisExpr thiz |
+      pred = TThisNode(thiz.getBindingContainer()) and
+      succ = valueNode(thiz))
   }
 
   /**

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

@@ -198,16 +198,36 @@ class NewNode extends InvokeNode {
   override DataFlow::Impl::NewNodeDef impl;
 }
 
-/** A data flow node corresponding to a `this` expression. */
-class ThisNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
-  override ThisExpr astNode;
+/** A data flow node corresponding to the `this` parameter in a function or `this` at the top-level. */
+class ThisNode extends DataFlow::Node, DataFlow::DefaultSourceNode {
+  ThisNode() {
+    DataFlow::thisNode(this, _)
+  }
 
   /**
    * Gets the function whose `this` binding this expression refers to,
    * which is the nearest enclosing non-arrow function.
    */
   FunctionNode getBinder() {
-    result = DataFlow::valueNode(astNode.getBinder())
+    exists (Function binder |
+      DataFlow::thisNode(this, binder) and
+      result = DataFlow::valueNode(binder))
+  }
+
+  /**
+   * Gets the function or top-level whose `this` binding this expression refers to,
+   * which is the nearest enclosing non-arrow function or top-level.
+   */
+  StmtContainer getBindingContainer() {
+    DataFlow::thisNode(this, result)
+  }
+
+  override string toString() { result = "this" }
+
+  override predicate hasLocationInfo(string filepath, int startline, int startcolumn,
+                                     int endline, int endcolumn) {
+    // Use the function entry as the location
+    getBindingContainer().getEntry().getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
   }
 }
 

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

@@ -185,7 +185,6 @@ class DefaultSourceNode extends SourceNode {
       astNode instanceof ObjectExpr or
       astNode instanceof ArrayExpr or
       astNode instanceof JSXNode or
-      astNode instanceof ThisExpr or
       astNode instanceof GlobalVarAccess or
       astNode instanceof ExternalModuleReference
     )
@@ -198,5 +197,7 @@ class DefaultSourceNode extends SourceNode {
     DataFlow::parameterNode(this, _)
     or
     this instanceof DataFlow::Impl::InvokeNodeDef
+    or
+    DataFlow::thisNode(this, _)
   }
 }

javascript/ql/src/semmle/javascript/frameworks/React.qll

@@ -52,10 +52,10 @@ abstract class ReactComponent extends ASTNode {
   }
 
   /**
-   * Gets a `this` access in an instance method of this component.
+   * Gets the `this` node in an instance method of this component.
    */
   DataFlow::SourceNode getAThisAccess() {
-    result.asExpr().(ThisExpr).getBinder() = getInstanceMethod(_)
+    result.(DataFlow::ThisNode).getBinder().getFunction() = getInstanceMethod(_)
   }
 
   /**

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -3,6 +3,8 @@
 | partialCalls.js:4:17:4:24 | source() | partialCalls.js:30:14:30:20 | x.value |
 | partialCalls.js:4:17:4:24 | source() | partialCalls.js:41:10:41:18 | id(taint) |
 | partialCalls.js:4:17:4:24 | source() | partialCalls.js:51:14:51:14 | x |
+| thisAssignments.js:4:17:4:24 | source() | thisAssignments.js:5:10:5:18 | obj.field |
+| thisAssignments.js:7:19:7:26 | source() | thisAssignments.js:8:10:8:20 | this.field2 |
 | tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x |
 | tst.js:2:13:2:20 | source() | tst.js:5:10:5:22 | "/" + x + "!" |
 | tst.js:2:13:2:20 | source() | tst.js:14:10:14:17 | x.sort() |

javascript/ql/test/library-tests/TaintTracking/thisAssignments.js

@@ -0,0 +1,10 @@
+class C {
+  foo() {
+    let obj = {};
+    obj.field = source();
+    sink(obj.field); // NOT OK - tainted
+    
+    this.field2 = source();
+    sink(this.field2); // NOT OK - tainted
+  }
+}

javascript/ql/test/query-tests/Security/CWE-079/Xss.expected

@@ -1,3 +1,4 @@
+| addEventListener.js:2:20:2:29 | event.data | Cross-site scripting vulnerability due to $@. | addEventListener.js:1:43:1:47 | event | user-provided value |
 | jquery.js:4:5:4:11 | tainted | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:33 | document.location | user-provided value |
 | jquery.js:7:5:7:34 | "<div i ... + "\\">" | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:33 | document.location | user-provided value |
 | jquery.js:8:18:8:34 | "XSS: " + tainted | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:33 | document.location | user-provided value |

javascript/ql/test/query-tests/Security/CWE-079/addEventListener.js

@@ -0,0 +1,3 @@
+this.addEventListener('message', function(event) {
+    document.write(event.data); // NOT OK
+})