JavaScript: Fix IllegalInvocation.

This fixes false positives that arise when a call such as `f.apply` can either be interpreted as a reflective invocation of `f`, or a normal call to method `apply` of `f`.

Author: Max Schaefer

change-notes/1.23/analysis-javascript.md

@@ -25,6 +25,7 @@
 | Client-side cross-site scripting (`js/xss`) | More results | More potential vulnerabilities involving functions that manipulate DOM attributes are now recognized. |
 | Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving functions that manipulate DOM event handler attributes are now recognized. |
 | Hard-coded credentials (`js/hardcoded-credentials`) | Fewer false-positive results | This rule now flags fewer password examples. |
+| Illegal invocation (`js/illegal-invocation`) | Fewer false-positive results | This rule now correctly handles methods named `call` and `apply`. |
 | Incorrect suffix check (`js/incorrect-suffix-check`) | Fewer false-positive results | The query recognizes valid checks in more cases. 
 | Network data written to file (`js/http-to-file-access`) | Fewer false-positive results | This query has been renamed to better match its intended purpose, and now only considers network data untrusted. | 
 | Password in configuration file (`js/password-in-configuration-file`) | Fewer false-positive results | This rule now flags fewer password examples. |

javascript/ql/src/LanguageFeatures/IllegalInvocation.ql

@@ -58,7 +58,8 @@ where
   // filter out some easy cases
   not isCallToFunction(cs) and
   // conservatively only flag call sites where _all_ callees are illegal
-  forex(Function otherCallee | otherCallee = cs.getACallee() |
+  forex(DataFlow::InvokeNode cs2, Function otherCallee |
+    cs2.getInvokeExpr() = cs.getInvokeExpr() and otherCallee = cs2.getACallee() |
     illegalInvocation(cs, otherCallee, _, _)
   )
 select cs, "Illegal invocation of $@ " + how + ".", callee, calleeDesc

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

@@ -935,6 +935,9 @@ module DataFlow {
      * and either with or without `new`.
      */
     abstract class InvokeNodeDef extends DataFlow::Node {
+      /** Gets the syntactic invoke expression underlying this function invocation. */
+      abstract InvokeExpr getInvokeExpr();
+
       /** Gets the name of the function or method being invoked, if it can be determined. */
       abstract string getCalleeName();
 
@@ -985,6 +988,8 @@ module DataFlow {
     class ExplicitInvokeNode extends InvokeNodeDef, DataFlow::ValueNode {
       override InvokeExpr astNode;
 
+      override InvokeExpr getInvokeExpr() { result = astNode }
+
       override string getCalleeName() { result = astNode.getCalleeName() }
 
       override DataFlow::Node getCalleeNode() { result = DataFlow::valueNode(astNode.getCallee()) }
@@ -1046,6 +1051,8 @@ module DataFlow {
 
       ReflectiveCallNodeDef() { this = TReflectiveCallNode(originalCall.asExpr(), kind) }
 
+      override InvokeExpr getInvokeExpr() { result = originalCall.getInvokeExpr() }
+
       override string getCalleeName() {
         result = originalCall.getReceiver().asExpr().(PropAccess).getPropertyName()
       }

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

@@ -34,6 +34,9 @@ class InvokeNode extends DataFlow::SourceNode {
 
   InvokeNode() { this = impl }
 
+  /** Gets the syntactic invoke expression underlying this function invocation. */
+  InvokeExpr getInvokeExpr() { result = impl.getInvokeExpr() }
+
   /** Gets the name of the function or method being invoked, if it can be determined. */
   string getCalleeName() { result = impl.getCalleeName() }
 

javascript/ql/test/query-tests/LanguageFeatures/IllegalInvocation/tst.js

@@ -45,4 +45,12 @@ new h()          // NOT OK
 C.call();        // NOT OK
 C.apply();       // NOT OK
 
+class E {
+  static call() {}
+  static apply() {}
+}
+
+E.call();        // OK
+E.apply();       // OK
+
 //semmle-extractor-options: --experimental