Merge pull request #5498 from asgerf/js/flow-through-accessors

Approved by erik-krogh, max-schaefer

javascript/change-notes/2021-03-23-accessor-calls.md

@@ -0,0 +1,3 @@
+lgtm,codescanning
+* Calls to property accessors are now analyzed on par with regular function calls,
+  leading to more results from queries that rely on data flow.

javascript/ql/src/meta/alerts/CallGraph.ql

@@ -10,6 +10,9 @@
 
 import javascript
 
-from DataFlow::InvokeNode invoke, Function f
-where invoke.getACallee() = f
-select invoke, "Call to $@", f, f.describe()
+from DataFlow::Node invoke, Function f, string kind
+where
+  invoke.(DataFlow::InvokeNode).getACallee() = f and kind = "Call"
+  or
+  invoke.(DataFlow::PropRef).getAnAccessorCallee().getFunction() = f and kind = "Accessor call"
+select invoke, kind + " to $@", f, f.describe()

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

@@ -1461,19 +1461,20 @@ private predicate summarizedHigherOrderCall(
   DataFlow::Node arg, DataFlow::Node cb, int i, DataFlow::Configuration cfg, PathSummary summary
 ) {
   exists(
-    Function f, DataFlow::InvokeNode outer, DataFlow::InvokeNode inner, int j,
-    DataFlow::Node innerArg, DataFlow::SourceNode cbParm, PathSummary oldSummary
+    Function f, DataFlow::InvokeNode inner, int j, DataFlow::Node innerArg,
+    DataFlow::SourceNode cbParm, PathSummary oldSummary
   |
     // Captured flow does not need to be summarized - it is handled by the local case in `higherOrderCall`.
-    not arg = DataFlow::capturedVariableNode(_) and
-    summarizedHigherOrderCallAux(f, outer, arg, innerArg, cfg, oldSummary, cbParm, inner, j, cb)
+    not arg = DataFlow::capturedVariableNode(_)
   |
     // direct higher-order call
-    cbParm.flowsTo(inner.getCalleeNode()) and
+    summarizedHigherOrderCallAux(f, arg, innerArg, cfg, oldSummary, cbParm, inner, j, cb) and
+    inner = cbParm.getAnInvocation() and
     i = j and
     summary = oldSummary
     or
     // indirect higher-order call
+    summarizedHigherOrderCallAux(f, arg, innerArg, cfg, oldSummary, cbParm, inner, j, cb) and
     exists(DataFlow::Node cbArg, PathSummary newSummary |
       cbParm.flowsTo(cbArg) and
       summarizedHigherOrderCall(innerArg, cbArg, i, cfg, newSummary) and
@@ -1487,14 +1488,17 @@ private predicate summarizedHigherOrderCall(
  */
 pragma[noinline]
 private predicate summarizedHigherOrderCallAux(
-  Function f, DataFlow::InvokeNode outer, DataFlow::Node arg, DataFlow::Node innerArg,
-  DataFlow::Configuration cfg, PathSummary oldSummary, DataFlow::SourceNode cbParm,
-  DataFlow::InvokeNode inner, int j, DataFlow::Node cb
+  Function f, DataFlow::Node arg, DataFlow::Node innerArg, DataFlow::Configuration cfg,
+  PathSummary oldSummary, DataFlow::SourceNode cbParm, DataFlow::InvokeNode inner, int j,
+  DataFlow::Node cb
 ) {
-  reachableFromInput(f, outer, arg, innerArg, cfg, oldSummary) and
-  // Only track actual parameter flow.
-  argumentPassing(outer, cb, f, cbParm) and
-  innerArg = inner.getArgument(j)
+  exists(DataFlow::Node outer1, DataFlow::Node outer2 |
+    reachableFromInput(f, outer1, arg, innerArg, cfg, oldSummary) and
+    outer1 = pragma[only_bind_into](outer2) and
+    // Only track actual parameter flow.
+    argumentPassing(outer2, cb, f, cbParm) and
+    innerArg = inner.getArgument(j)
+  )
 }
 
 /**

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/Nodes.qll

@@ -546,6 +546,16 @@ class ObjectLiteralNode extends DataFlow::ValueNode, DataFlow::SourceNode {
   DataFlow::Node getASpreadProperty() {
     result = astNode.getAProperty().(SpreadProperty).getInit().(SpreadElement).getOperand().flow()
   }
+
+  /** Gets the property getter of the given name, installed on this object literal. */
+  DataFlow::FunctionNode getPropertyGetter(string name) {
+    result = astNode.getPropertyByName(name).(PropertyGetter).getInit().flow()
+  }
+
+  /** Gets the property setter of the given name, installed on this object literal. */
+  DataFlow::FunctionNode getPropertySetter(string name) {
+    result = astNode.getPropertyByName(name).(PropertySetter).getInit().flow()
+  }
 }
 
 /**

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

@@ -162,4 +162,46 @@ module CallGraph {
       )
     )
   }
+
+  /** Holds if a property setter named `name` exists in a class. */
+  private predicate isSetterName(string name) {
+    exists(any(DataFlow::ClassNode cls).getInstanceMember(name, DataFlow::MemberKind::setter()))
+  }
+
+  /**
+   * Gets a property write that assigns to the property `name` on an instance of this class,
+   * and `name` is the name of a property setter.
+   */
+  private DataFlow::PropWrite getAnInstanceMemberAssignment(DataFlow::ClassNode cls, string name) {
+    isSetterName(name) and // restrict size of predicate
+    result = cls.getAnInstanceReference().getAPropertyWrite(name)
+    or
+    exists(DataFlow::ClassNode subclass |
+      result = getAnInstanceMemberAssignment(subclass, name) and
+      not exists(subclass.getInstanceMember(name, DataFlow::MemberKind::setter())) and
+      cls = subclass.getADirectSuperClass()
+    )
+  }
+
+  /**
+   * 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.getAnInstanceMemberAccess(name) and
+      result = cls.getInstanceMember(name, DataFlow::MemberKind::getter())
+      or
+      ref = getAnInstanceMemberAssignment(cls, name) and
+      result = cls.getInstanceMember(name, DataFlow::MemberKind::setter())
+    )
+    or
+    exists(DataFlow::ObjectLiteralNode object, string name |
+      ref = object.getAPropertyRead(name) and
+      result = object.getPropertyGetter(name)
+      or
+      ref = object.getAPropertyWrite(name) and
+      result = object.getPropertySetter(name)
+    )
+  }
 }

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

@@ -25,7 +25,9 @@ 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 +122,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 +183,11 @@ private module CachedSteps {
    */
   cached
   predicate argumentPassing(
-    DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Function f, DataFlow::SourceNode parm
+    DataFlow::SourceNode invk, DataFlow::Node 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,12 +199,19 @@ 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 |
+    exists(DataFlow::Node callback, int i, Parameter p, Function target |
       invk.(DataFlow::PartialInvokeNode).isPartialArgument(callback, arg, i) and
       partiallyCalls(invk, callback, f) and
-      f.getParameter(i) = p and
+      f = pragma[only_bind_into](target) and
+      target.getParameter(i) = p and
       not p.isRestParameter() and
       parm = DataFlow::parameterNode(p)
     )
@@ -213,7 +226,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,16 @@ 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 |
+| getters-and-setters.js:60:20:60:27 | source() | getters-and-setters.js:66:10:66:14 | obj.x |
+| getters-and-setters.js:67:13:67:20 | source() | getters-and-setters.js:63:18:63:22 | value |
+| getters-and-setters.js:79:20:79:27 | source() | getters-and-setters.js:88:10:88:18 | new C().x |
+| getters-and-setters.js:79:20:79:27 | source() | getters-and-setters.js:92:14:92:16 | c.x |
+| getters-and-setters.js:79:20:79:27 | source() | getters-and-setters.js:100:10:100:22 | getX(new C()) |
+| getters-and-setters.js:89:17:89:24 | source() | getters-and-setters.js:82:18:82:22 | value |
 | 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,16 @@
 | 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 |
+| getters-and-setters.js:60:20:60:27 | source() | getters-and-setters.js:66:10:66:14 | obj.x |
+| getters-and-setters.js:67:13:67:20 | source() | getters-and-setters.js:63:18:63:22 | value |
+| getters-and-setters.js:79:20:79:27 | source() | getters-and-setters.js:88:10:88:18 | new C().x |
+| getters-and-setters.js:79:20:79:27 | source() | getters-and-setters.js:92:14:92:16 | c.x |
+| getters-and-setters.js:79:20:79:27 | source() | getters-and-setters.js:100:10:100:22 | getX(new C()) |
+| getters-and-setters.js:89:17:89:24 | source() | getters-and-setters.js:82:18:82:22 | value |
 | 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

@@ -0,0 +1,102 @@
+import * as dummy from 'dummy';
+
+function testGetterSource() {
+    class C {
+        get x() {
+            return source();
+        }
+    };
+    sink(new C().x); // NOT OK
+
+    function indirection(c) {
+        if (c) {
+            sink(c.x); // NOT OK
+        }
+    }
+    indirection(new C());
+    indirection(null);
+}
+
+function testSetterSink() {
+    class C {
+        set x(v) {
+            sink(v); // NOT OK
+        }
+    };
+    function indirection(c) {
+        c.x = source();
+    }
+    indirection(new C());
+    indirection(null);
+}
+
+function testFlowThroughGetter() {
+    class C {
+        constructor(x) {
+            this._x = x;
+        }
+
+        get x() {
+            return this._x;
+        }
+    };
+
+    function indirection(c) {
+        sink(c.x); // NOT OK
+    }
+    indirection(new C(source()));
+    indirection(null);
+
+    function getX(c) {
+        return c.x;
+    }
+    sink(getX(new C(source()))); // NOT OK - but not flagged
+    getX(null);
+}
+
+function testFlowThroughObjectLiteralAccessors() {
+    let obj = {
+        get x() {
+            return source();
+        },
+        set y(value) {
+            sink(value); // NOT OK
+        }
+    };
+    sink(obj.x); // NOT OK
+    obj.y = source();
+
+    function indirection(c) {
+        sink(c.x); // NOT OK - but not currently flagged
+    }
+    indirection(obj);
+    indirection(null);
+}
+
+function testFlowThroughSubclass() {
+    class Base {
+        get x() {
+            return source();
+        }
+        set y(value) {
+            sink(value); // NOT OK
+        }
+    };
+    class C extends Base {
+    }
+
+    sink(new C().x); // NOT OK
+    new C().y = source();
+
+    function indirection(c) {
+        sink(c.x); // NOT OK
+    }
+    indirection(new C());
+    indirection(null);
+
+    function getX(c) {
+        return c.x;
+    }
+    sink(getX(new C())); // NOT OK - but not flagged
+    getX(null);
+}