JS: Track flow into ES accessors

Author: asger-semmle

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

@@ -531,6 +531,13 @@ module DataFlow {
     predicate isPrivateField() {
       getPropertyName().charAt(0) = "#" and getPropertyNameExpr() instanceof Label
     }
+
+    /**
+     * Gets an accessor (`get` or `set` method) that may be invoked by this property reference.
+     */
+    final DataFlow::FunctionNode getAnAccessorCallee() {
+      result = CallGraph::getAnAccessorCallee(this)
+    }
   }
 
   /**

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

@@ -162,4 +162,18 @@ module CallGraph {
       )
     )
   }
+
+  /**
+   * Gets a getter or setter invoked as a result of the given property access.
+   */
+  cached
+  DataFlow::FunctionNode getAnAccessorCallee(DataFlow::PropRef ref) {
+    exists(DataFlow::ClassNode cls, string name |
+      ref = cls.getAnInstanceReference().getAPropertyRead(name) and
+      result = cls.getInstanceMember(name, DataFlow::MemberKind::getter())
+      or
+      ref = cls.getAnInstanceReference().getAPropertyWrite(name) and
+      result = cls.getInstanceMember(name, DataFlow::MemberKind::setter())
+    )
+  }
 }

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

@@ -25,7 +25,8 @@ predicate shouldTrackProperties(AbstractValue obj) {
  */
 pragma[noinline]
 predicate returnExpr(Function f, DataFlow::Node source, DataFlow::Node sink) {
-  sink.asExpr() = f.getAReturnedExpr() and source = sink
+  sink.asExpr() = f.getAReturnedExpr() and source = sink and
+  not f = any(SetterMethodDeclaration decl).getBody()
 }
 
 /**
@@ -120,7 +121,11 @@ private module CachedSteps {
    * Holds if `invk` may invoke `f`.
    */
   cached
-  predicate calls(DataFlow::InvokeNode invk, Function f) { f = invk.getACallee(0) }
+  predicate calls(DataFlow::SourceNode invk, Function f) {
+    f = invk.(DataFlow::InvokeNode).getACallee(0)
+    or
+    f = invk.(DataFlow::PropRef).getAnAccessorCallee().getFunction()
+  }
 
   private predicate callsBoundInternal(
     DataFlow::InvokeNode invk, Function f, int boundArgs, boolean contextDependent
@@ -177,11 +182,11 @@ private module CachedSteps {
    */
   cached
   predicate argumentPassing(
-    DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Function f, DataFlow::SourceNode parm
+    DataFlow::SourceNode invk, DataFlow::ValueNode arg, Function f, DataFlow::SourceNode parm
   ) {
     calls(invk, f) and
     (
-      exists(int i | arg = invk.getArgument(i) |
+      exists(int i | arg = invk.(DataFlow::InvokeNode).getArgument(i) |
         exists(Parameter p |
           f.getParameter(i) = p and
           not p.isRestParameter() and
@@ -193,6 +198,12 @@ private module CachedSteps {
       or
       arg = invk.(DataFlow::CallNode).getReceiver() and
       parm = DataFlow::thisNode(f)
+      or
+      arg = invk.(DataFlow::PropRef).getBase() and
+      parm = DataFlow::thisNode(f)
+      or
+      arg = invk.(DataFlow::PropWrite).getRhs() and
+      parm = DataFlow::parameterNode(f.getParameter(0))
     )
     or
     exists(DataFlow::Node callback, int i, Parameter p |
@@ -213,7 +224,7 @@ private module CachedSteps {
       callsBound(invk, f, boundArgs) and
       f.getParameter(boundArgs + i) = p and
       not p.isRestParameter() and
-      arg = invk.getArgument(i) and
+      arg = invk.(DataFlow::InvokeNode).getArgument(i) and
       parm = DataFlow::parameterNode(p)
     )
   }

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

@@ -66,6 +66,10 @@ typeInferenceMismatch
 | exceptions.js:144:9:144:16 | source() | exceptions.js:132:8:132:27 | returnThrownSource() |
 | exceptions.js:150:13:150:20 | source() | exceptions.js:153:10:153:10 | e |
 | exceptions.js:158:13:158:20 | source() | exceptions.js:161:10:161:10 | e |
+| getters-and-setters.js:6:20:6:27 | source() | getters-and-setters.js:9:10:9:18 | new C().x |
+| getters-and-setters.js:6:20:6:27 | source() | getters-and-setters.js:13:18:13:20 | c.x |
+| getters-and-setters.js:27:15:27:22 | source() | getters-and-setters.js:23:18:23:18 | v |
+| getters-and-setters.js:47:23:47:30 | source() | getters-and-setters.js:45:14:45:16 | c.x |
 | importedReactComponent.jsx:4:40:4:47 | source() | exportedReactComponent.jsx:2:10:2:19 | props.text |
 | indexOf.js:4:11:4:18 | source() | indexOf.js:9:10:9:10 | x |
 | json-stringify.js:2:16:2:23 | source() | json-stringify.js:5:8:5:29 | JSON.st ... source) |

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

@@ -41,6 +41,10 @@
 | exceptions.js:144:9:144:16 | source() | exceptions.js:132:8:132:27 | returnThrownSource() |
 | exceptions.js:150:13:150:20 | source() | exceptions.js:153:10:153:10 | e |
 | exceptions.js:158:13:158:20 | source() | exceptions.js:161:10:161:10 | e |
+| getters-and-setters.js:6:20:6:27 | source() | getters-and-setters.js:9:10:9:18 | new C().x |
+| getters-and-setters.js:6:20:6:27 | source() | getters-and-setters.js:13:18:13:20 | c.x |
+| getters-and-setters.js:27:15:27:22 | source() | getters-and-setters.js:23:18:23:18 | v |
+| getters-and-setters.js:47:23:47:30 | source() | getters-and-setters.js:45:14:45:16 | c.x |
 | indexOf.js:4:11:4:18 | source() | indexOf.js:9:10:9:10 | x |
 | indexOf.js:4:11:4:18 | source() | indexOf.js:13:10:13:10 | x |
 | nested-props.js:4:13:4:20 | source() | nested-props.js:5:10:5:14 | obj.x |

javascript/ql/test/library-tests/TaintTracking/getters-and-setters.js

@@ -50,6 +50,6 @@ function testFlowThroughGetter() {
     function getX(c) {
         return c.x;
     }
-    sink(getX(new C(source()))); // NOT OK
-    sink(getX(new C(source()))); // NOT OK
+    sink(getX(new C(source()))); // NOT OK - but not flagged
+    getX(null);
 }