Merge pull request #68 from esben-semmle/determinate-1-cfa-type-inference

Approved by xiemaisi

Author: semmle-qlci

change-notes/1.18/analysis-javascript.md

@@ -16,6 +16,8 @@
 
 * The taint tracking library now recognizes additional sanitization patterns. This may give fewer false-positive results for the security queries.
 
+* Type inference for simple function calls has been improved. This may give additional results for queries that rely on type inference.
+
 * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following libraries:
   - [bluebird](http://bluebirdjs.com)
   - [browserid-crypto](https://github.com/mozilla/browserid-crypto)
@@ -92,6 +94,7 @@
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Arguments redefined | Fewer results | This rule previously also flagged redefinitions of `eval`. This was an oversight that is now fixed. |
+| Comparison between inconvertible types | Fewer results | This rule now flags fewer comparisons involving parameters. |
 | Comparison between inconvertible types | Lower severity | The severity of this rule has been revised to "warning". |
 | CORS misconfiguration for credentials transfer | More true-positive results | This rule now treats header names case-insensitively. |
 | Hard-coded credentials | More true-positive results | This rule now recognizes secret cryptographic keys. |
@@ -106,6 +109,8 @@
 | Unused variable | Fewer results | This rule no longer flags class expressions that could be made anonymous. While technically true, these results are not interesting. |
 | Unused variable | Renamed | This rule has been renamed to "Unused variable, import, function or class" to reflect the fact that it flags different kinds of unused program elements. |
 | Use of incompletely initialized object| Fewer results | This rule now flags the constructor instead its errorneous `this` or `super` expressions. |
+| Useless conditional | Fewer results | This rule no longer flags uses of boolean return values. | 
+| Useless conditional | Fewer results | This rule now flags fewer comparisons involving parameters. | 
 
 ## Changes to QL libraries
 

javascript/ql/src/Expressions/HeterogeneousComparison.ql

@@ -170,13 +170,36 @@ string getTypeDescription(string message1, string message2, int complexity1, int
     result = message1
 }
 
+/**
+ * Holds if `e` directly uses a parameter's initial value as passed in from the caller.
+ */
+predicate isInitialParameterUse(Expr e) {
+  // unlike `SimpleParameter.getAnInitialUse` this will not include uses we have refinement information for
+  exists (SimpleParameter p, SsaExplicitDefinition ssa |
+    ssa.getDef() = p and
+    ssa.getVariable().getAUse() = e and
+    not p.isRestParameter()
+  )
+}
+
+/**
+ * Holds if `e` is an expression that should not be considered in a heterogeneous comparison.
+ *
+ * We currently whitelist expressions that rely on inter-procedural parameter information.
+ */
+predicate whitelist(Expr e) {
+  isInitialParameterUse(e)
+}
+
 from ASTNode cmp,
      DataFlow::AnalyzedNode left, DataFlow::AnalyzedNode right,
      string leftTypes, string rightTypes,
      string leftExprDescription, string rightExprDescription,
      int leftTypeCount, int rightTypeCount ,
      string leftTypeDescription, string rightTypeDescription
 where isHeterogeneousComparison(cmp, left, right, leftTypes, rightTypes) and
+      not whitelist(left.asExpr()) and
+      not whitelist(right.asExpr()) and
       leftExprDescription = capitalize(getDescription(left.asExpr(), "this expression")) and
       rightExprDescription = getDescription(right.asExpr(), "an expression") and
       leftTypeCount = strictcount(left.getAType()) and

javascript/ql/src/Statements/UselessConditional.ql

@@ -14,6 +14,7 @@
 
 import javascript
 import semmle.javascript.RestrictedLocations
+import semmle.javascript.dataflow.Refinements
 
 /**
  * Holds if `va` is a defensive truthiness check that may be worth keeping, even if it
@@ -62,19 +63,55 @@ predicate isConstant(Expr e) {
   isSymbolicConstant(e.(VarAccess).getVariable())
 }
 
+/**
+ * Holds if `e` directly uses a parameter's negated or initial value as passed in from the caller.
+ */
+predicate isInitialParameterUse(Expr e) {
+  // unlike `SimpleParameter.getAnInitialUse` this will not include uses we have refinement information for
+  exists (SimpleParameter p, SsaExplicitDefinition ssa |
+    ssa.getDef() = p and
+    ssa.getVariable().getAUse() = e and
+    not p.isRestParameter()
+  )
+  or
+  isInitialParameterUse(e.(LogNotExpr).getOperand())
+}
+
+/**
+ * Holds if `e` directly uses the returned value from a function call that returns a constant boolean value.
+ */
+predicate isConstantBooleanReturnValue(Expr e) {
+  // unlike `SourceNode.flowsTo` this will not include uses we have refinement information for
+  exists (DataFlow::CallNode call |
+    exists (call.analyze().getTheBooleanValue()) |
+    e = call.asExpr() or
+    // also support return values that are assigned to variables
+    exists (SsaExplicitDefinition ssa |
+      ssa.getDef().getSource() = call.asExpr() and
+      ssa.getVariable().getAUse() = e
+    )
+  )
+  or
+  isConstantBooleanReturnValue(e.(LogNotExpr).getOperand())
+}
+
 /**
  * Holds if `e` is an expression that should not be flagged as a useless condition.
  *
- * We currently whitelist three kinds of expressions:
+ * We currently whitelist these kinds of expressions:
  *
  *   - constants (including references to literal constants);
  *   - negations of constants;
- *   - defensive checks.
+ *   - defensive checks;
+ *   - expressions that rely on inter-procedural parameter information;
+ *   - constant boolean returned values.
  */
 predicate whitelist(Expr e) {
   isConstant(e) or
   isConstant(e.(LogNotExpr).getOperand()) or
-  isDefensiveInit(e)
+  isDefensiveInit(e) or
+  isInitialParameterUse(e) or
+  isConstantBooleanReturnValue(e)
 }
 
 /**
@@ -107,4 +144,4 @@ where isConditional(cond, op.asExpr()) and
           msg = "This expression always evaluates to " + cv + "."
       )
 
-select sel, msg
+select sel, msg

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

@@ -239,3 +239,52 @@ class AnalyzedModule extends TopLevel {
     )
   }
 }
+
+/**
+ * Flow analysis for functions.
+ */
+class AnalyzedFunction extends DataFlow::AnalyzedValueNode {
+  override Function astNode;
+
+  override AbstractValue getALocalValue() { result = TAbstractFunction(astNode) }
+
+  /**
+   * Gets a return value for a call to this function.
+   */
+  AbstractValue getAReturnValue() {
+    if astNode.isGenerator() or astNode.isAsync() then
+      result = TAbstractOtherObject()
+    else (
+      // explicit return value
+      result = astNode.getAReturnedExpr().analyze().getALocalValue()
+      or
+      // implicit return value
+      (
+        // either because execution of the function may terminate normally
+        mayReturnImplicitly()
+        or
+        // or because there is a bare `return;` statement
+        exists (ReturnStmt ret | ret = astNode.getAReturnStmt() | not exists(ret.getExpr()))
+      ) and
+      result = TAbstractUndefined()
+    )
+  }
+
+  /**
+   * Holds if the execution of this function may complete normally without
+   * encountering a `return` or `throw` statement.
+   *
+   * Note that this is an overapproximation, that is, the predicate may hold
+   * of functions that cannot actually complete normally, since it does not
+   * account for `finally` blocks and does not check reachability.
+   */
+  private predicate mayReturnImplicitly() {
+    exists (ConcreteControlFlowNode final |
+      final.getContainer() = astNode and
+      final.isAFinalNode() and
+      not final instanceof ReturnStmt and
+      not final instanceof ThrowStmt
+    )
+  }
+
+}

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

@@ -89,15 +89,6 @@ private class AnalyzedArrayComprehensionExpr extends DataFlow::AnalyzedValueNode
   override AbstractValue getALocalValue() { result = TAbstractOtherObject() }
 }
 
-/**
- * Flow analysis for functions.
- */
-private class AnalyzedFunction extends DataFlow::AnalyzedValueNode {
-  override Function astNode;
-
-  override AbstractValue getALocalValue() { result = TAbstractFunction(astNode) }
-}
-
 /**
  * Flow analysis for class declarations.
  */

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

@@ -7,69 +7,6 @@
 import javascript
 import AbstractValuesImpl
 
-/**
- * Flow analysis for immediately-invoked function expressions (IIFEs).
- */
-private class IifeReturnFlow extends DataFlow::AnalyzedValueNode {
-  ImmediatelyInvokedFunctionExpr iife;
-
-  IifeReturnFlow() {
-    astNode = (CallExpr)iife.getInvocation()
-  }
-
-  override AbstractValue getALocalValue() {
-    result = getAReturnValue(iife)
-  }
-}
-
-/**
- * Gets a return value for the immediately-invoked function expression `f`.
- */
-private AbstractValue getAReturnValue(ImmediatelyInvokedFunctionExpr f) {
-  // explicit return value
-  result = f.getAReturnedExpr().analyze().getALocalValue()
-  or
-  // implicit return value
-  (
-    // either because execution of the function may terminate normally
-    mayReturnImplicitly(f)
-    or
-    // or because there is a bare `return;` statement
-    exists (ReturnStmt ret | ret = f.getAReturnStmt() | not exists(ret.getExpr()))
-  ) and
-  result = getDefaultReturnValue(f)
-}
-
-
-/**
- * Holds if the execution of function `f` may complete normally without
- * encountering a `return` or `throw` statement.
- *
- * Note that this is an overapproximation, that is, the predicate may hold
- * of functions that cannot actually complete normally, since it does not
- * account for `finally` blocks and does not check reachability.
- */
-private predicate mayReturnImplicitly(Function f) {
-  exists (ConcreteControlFlowNode final |
-    final.getContainer() = f and
-    final.isAFinalNode() and
-    not final instanceof ReturnStmt and
-    not final instanceof ThrowStmt
-  )
-}
-
-/**
- * Gets the default return value for immediately-invoked function expression `f`,
- * that is, the value that `f` returns if its execution terminates without
- * encountering an explicit `return` statement.
- */
-private AbstractValue getDefaultReturnValue(ImmediatelyInvokedFunctionExpr f) {
-  if f.isGenerator() or f.isAsync() then
-    result = TAbstractOtherObject()
-  else
-    result = TAbstractUndefined()
-}
-
 /**
  * Flow analysis for `this` expressions inside functions.
  */
@@ -190,3 +127,79 @@ private class AnalyzedThisInPropertyFunction extends AnalyzedThisExpr {
   }
 }
 
+/**
+ * A call with inter-procedural type inference for the return value.
+ */
+abstract class CallWithAnalyzedReturnFlow extends DataFlow::AnalyzedValueNode {
+
+  /**
+   * Gets a called function.
+   */
+  abstract AnalyzedFunction getACallee();
+
+  override AbstractValue getALocalValue() {
+    result = getACallee().getAReturnValue() and
+    not this instanceof DataFlow::NewNode
+  }
+}
+
+/**
+ * Flow analysis for the return value of IIFEs.
+ */
+private class IIFEWithAnalyzedReturnFlow extends CallWithAnalyzedReturnFlow {
+  
+  ImmediatelyInvokedFunctionExpr iife;
+  
+  IIFEWithAnalyzedReturnFlow() {
+    astNode = iife.getInvocation()
+  }
+  
+  override AnalyzedFunction getACallee() {
+    result = iife.analyze()
+  }
+  
+}
+
+/** A function that only is used locally, making it amenable to type inference. */
+class LocalFunction extends Function {
+
+  DataFlow::Impl::ExplicitInvokeNode invk;
+
+  LocalFunction() {
+    this instanceof FunctionDeclStmt and
+    exists (LocalVariable v, Expr callee |
+      callee = invk.getCalleeNode().asExpr() and
+      v = getVariable() and
+      v.getAnAccess() = callee and
+      forall(VarAccess o | o = v.getAnAccess() | o = callee) and
+      not exists(v.getAnAssignedExpr()) and
+      not exists(ExportDeclaration export | export.exportsAs(v, _))
+    ) and
+    // if the function is non-strict and its `arguments` object is accessed, we
+    // also assume that there may be other calls (through `arguments.callee`)
+    (isStrict() or not usesArgumentsObject())
+  }
+
+  /** Gets an invocation of this function. */
+  DataFlow::InvokeNode getAnInvocation() {
+    result = invk
+  }
+
+}
+
+/**
+ * Enables inter-procedural type inference for a call to a `LocalFunction`.
+ */
+private class LocalFunctionCallWithAnalyzedReturnFlow extends CallWithAnalyzedReturnFlow {
+
+  LocalFunction f;
+
+  LocalFunctionCallWithAnalyzedReturnFlow() {
+    this = f.getAnInvocation()
+  }
+
+  override AnalyzedFunction getACallee() {
+    result = f.analyze()
+  }
+
+}