Merge master into next.

`master` up to and including cfe0b8803a.

javascript/ql/src/Declarations/UnusedVariable.ql

@@ -94,36 +94,95 @@ predicate isEnumMember(VarDecl decl) {
 }
 
 /**
- * Gets a description of the declaration `vd`, which is either of the form "function f" if
- * it is a function name, or "variable v" if it is not.
+ * Gets a description of the declaration `vd`, which is either of the form
+ * "function f", "variable v" or "class c".
  */
-string describe(VarDecl vd) {
+string describeVarDecl(VarDecl vd) {
   if vd = any(Function f).getId() then
     result = "function " + vd.getName()
   else if vd = any(ClassDefinition c).getIdentifier() then
     result = "class " + vd.getName()
-  else if (vd = any(ImportSpecifier im).getLocal() or vd = any(ImportEqualsDeclaration im).getId()) then
-    result = "import " + vd.getName()
   else
     result = "variable " + vd.getName()
 }
 
-from VarDecl vd, UnusedLocal v
-where v = vd.getVariable() and
-      // exclude variables mentioned in JSDoc comments in externs
-      not mentionedInJSDocComment(v) and
-      // exclude variables used to filter out unwanted properties
-      not isPropertyFilter(v) and
-      // exclude imports of React that are implicitly referenced by JSX
-      not isReactImportForJSX(v) and
-      // exclude names that are used as types
-      not isUsedAsType(vd) and
-      // exclude names that are used as namespaces from inside a type
-      not isUsedAsNamespace(vd) and
-      // exclude decorated functions and classes
-      not isDecorated(vd) and
-      // exclude names of enum members; they also define property names
-      not isEnumMember(vd) and
-      // ignore ambient declarations - too noisy
-      not vd.isAmbient()
-select vd, "Unused " + describe(vd) + "."
+/**
+ * An import statement that provides variable declarations.
+ */
+class ImportVarDeclProvider extends Stmt {
+
+  ImportVarDeclProvider() {
+    this instanceof ImportDeclaration or
+    this instanceof ImportEqualsDeclaration
+  }
+
+  /**
+   * Gets a variable declaration of this import.
+   */
+  VarDecl getAVarDecl() {
+    result = this.(ImportDeclaration).getASpecifier().getLocal() or
+    result = this.(ImportEqualsDeclaration).getId()
+  }
+
+  /**
+   * Gets an unacceptable unused variable declared by this import.
+   */
+  UnusedLocal getAnUnacceptableUnusedLocal() {
+    result = getAVarDecl().getVariable() and
+    not whitelisted(result)
+  }
+
+}
+
+/**
+ * Holds if it is acceptable that `v` is unused.
+ */
+predicate whitelisted(UnusedLocal v) {
+  // exclude variables mentioned in JSDoc comments in externs
+  mentionedInJSDocComment(v) or
+  // exclude variables used to filter out unwanted properties
+  isPropertyFilter(v) or
+  // exclude imports of React that are implicitly referenced by JSX
+  isReactImportForJSX(v) or
+  // exclude names that are used as types
+  exists (VarDecl vd |
+    v = vd.getVariable() |
+    isUsedAsType(vd) or
+    // exclude names that are used as namespaces from inside a type
+    isUsedAsNamespace(vd)or
+    // exclude decorated functions and classes
+    isDecorated(vd) or
+    // exclude names of enum members; they also define property names
+    isEnumMember(vd) or
+    // ignore ambient declarations - too noisy
+    vd.isAmbient()
+  )
+}
+
+/**
+ * Holds if `vd` declares an unused variable that does not come from an import statement, as explained by `msg`.
+ */
+predicate unusedNonImports(VarDecl vd, string msg) {
+  exists (UnusedLocal v |
+    v = vd.getVariable() and
+    msg = "Unused " + describeVarDecl(vd) + "." and
+    not vd = any(ImportVarDeclProvider p).getAVarDecl() and
+    not whitelisted(v)
+  )
+}
+
+/**
+ * Holds if `provider` declares one or more unused variables, as explained by `msg`.
+ */
+predicate unusedImports(ImportVarDeclProvider provider, string msg) {
+  exists (string imports, string names |
+    imports = pluralize("import", count(provider.getAnUnacceptableUnusedLocal())) and
+    names = strictconcat(provider.getAnUnacceptableUnusedLocal().getName(), ", ") and
+    msg = "Unused " + imports + " " + names + "."
+  )
+}
+
+from ASTNode sel, string msg
+where unusedNonImports(sel, msg) or
+      unusedImports(sel, msg)
+select sel, msg

javascript/ql/src/LanguageFeatures/EmptyArrayInit.ql

@@ -45,5 +45,4 @@ class OmittedArrayElement extends ArrayExpr {
 }
 
 from OmittedArrayElement ae
-where not ae.getFile().getFileType().isTypeScript() // ignore quirks in TypeScript tokenizer
 select ae, "Avoid omitted array elements."

javascript/ql/src/LanguageFeatures/SemicolonInsertion.ql

@@ -36,8 +36,7 @@ where s.hasSemicolonInserted() and
       asi = strictcount(Stmt ss | asi(sc, ss, true)) and
       nstmt = strictcount(Stmt ss | asi(sc, ss, _)) and
       perc = ((1-asi/nstmt)*100).floor() and
-      perc >= 90 and
-      not s.getFile().getFileType().isTypeScript() // ignore some quirks in the TypeScript tokenizer
+      perc >= 90
 select (LastLineOf)s, "Avoid automated semicolon insertion " +
                       "(" + perc + "% of all statements in $@ have an explicit semicolon).",
                       sc, "the enclosing " + sctype

javascript/ql/src/Statements/MisleadingIndentationAfterControlStmt.ql

@@ -39,7 +39,6 @@ where misleadingIndentationCandidate(ctrl, s1, s2) and
       f.hasIndentation(ctrlStartLine, indent, _) and
       f.hasIndentation(startLine1, indent, _) and
       f.hasIndentation(startLine2, indent, _) and
-      not s2 instanceof EmptyStmt and
-      not f.getFileType().isTypeScript() // ignore quirks in TypeScript tokenizer
+      not s2 instanceof EmptyStmt
 select (FirstLineOf)s2, "The indentation of this statement suggests that it is controlled by $@, while in fact it is not.",
             (FirstLineOf)ctrl, "this statement"

javascript/ql/src/semmle/javascript/Util.qll

@@ -11,3 +11,16 @@ bindingset[s]
 string capitalize(string s) {
   result = s.charAt(0).toUpperCase() + s.suffix(1)
 }
+
+ /**
+  * Gets the pluralization for `n` occurrences of `noun`.
+  *
+  * For example, the pluralization of `"function"` for `n = 2` is `"functions"`.
+  */
+bindingset[noun, n]
+string pluralize(string noun, int n) {
+  if n = 1 then
+    result = noun
+  else
+    result = noun + "s"
+}

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

@@ -93,7 +93,7 @@ abstract class Configuration extends string {
   }
 
   /**
-   * Holds if `source` is a source of flow labelled with `lbl` that is relevant
+   * Holds if `source` is a source of flow labeled with `lbl` that is relevant
    * for this configuration.
    */
   predicate isSource(DataFlow::Node source, FlowLabel lbl) {
@@ -108,7 +108,7 @@ abstract class Configuration extends string {
   }
 
   /**
-   * Holds if `sink` is a sink of flow labelled with `lbl` that is relevant
+   * Holds if `sink` is a sink of flow labeled with `lbl` that is relevant
    * for this configuration.
    */
   predicate isSink(DataFlow::Node sink, FlowLabel lbl) {
@@ -146,6 +146,7 @@ abstract class Configuration extends string {
    */
   predicate isBarrier(DataFlow::Node node) {
     exists (BarrierGuardNode guard |
+      not guard instanceof LabeledBarrierGuardNode and
       isBarrierGuard(guard) and
       guard.blocks(node)
     )
@@ -161,6 +162,17 @@ abstract class Configuration extends string {
    */
   predicate isBarrier(DataFlow::Node src, DataFlow::Node trg, FlowLabel lbl) { none() }
 
+  /**
+   * Holds if flow with label `lbl` cannot flow into `node`.
+   */
+  predicate isLabeledBarrier(DataFlow::Node node, FlowLabel lbl) {
+    exists (LabeledBarrierGuardNode guard |
+      lbl = guard.getALabel() and
+      isBarrierGuard(guard) and
+      guard.blocks(node)
+    )
+  }
+
   /**
    * Holds if data flow node `guard` can act as a barrier when appearing
    * in a condition.
@@ -297,7 +309,16 @@ abstract class BarrierGuardNode extends DataFlow::Node {
    * Holds if this node blocks expression `e` provided it evaluates to `outcome`.
    */
   abstract predicate blocks(boolean outcome, Expr e);
+}
 
+/**
+ * A guard node that only blocks specific labels.
+ */
+abstract class LabeledBarrierGuardNode extends BarrierGuardNode {
+  /**
+   * Get a flow label blocked by this guard node.
+   */
+  abstract FlowLabel getALabel();
 }
 
 /**
@@ -570,7 +591,8 @@ private predicate flowThroughCall(DataFlow::Node input, DataFlow::Node invk,
     ret.asExpr() = f.getAReturnedExpr() and
     calls(invk, f) and // Do not consider partial calls
     reachableFromInput(f, invk, input, ret, cfg, summary) and
-    not cfg.isBarrier(ret, invk)
+    not cfg.isBarrier(ret, invk) and
+    not cfg.isLabeledBarrier(invk, summary.getEndLabel())
   )
 }
 
@@ -641,7 +663,8 @@ private predicate flowStep(DataFlow::Node pred, DataFlow::Configuration cfg,
    flowThroughProperty(pred, succ, cfg, summary)
   ) and
   not cfg.isBarrier(succ) and
-  not cfg.isBarrier(pred, succ)
+  not cfg.isBarrier(pred, succ) and
+  not cfg.isLabeledBarrier(succ, summary.getEndLabel())
 }
 
 /**
@@ -666,6 +689,7 @@ private predicate reachableFromSource(DataFlow::Node nd, DataFlow::Configuration
   exists (FlowLabel lbl |
     isSource(nd, cfg, lbl) and
     not cfg.isBarrier(nd) and
+    not cfg.isLabeledBarrier(nd, lbl) and
     summary = MkPathSummary(false, false, lbl, lbl)
   )
   or
@@ -684,7 +708,8 @@ private predicate onPath(DataFlow::Node nd, DataFlow::Configuration cfg,
                          PathSummary summary) {
   reachableFromSource(nd, cfg, summary) and
   isSink(nd, cfg, summary.getEndLabel()) and
-  not cfg.isBarrier(nd)
+  not cfg.isBarrier(nd) and
+  not cfg.isLabeledBarrier(nd, summary.getEndLabel())
   or
   exists (DataFlow::Node mid, PathSummary stepSummary |
     reachableFromSource(nd, cfg, summary) and

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

@@ -130,7 +130,7 @@ module TaintTracking {
    * configurations it is used in.
    *
    * Note: For performance reasons, all subclasses of this class should be part
-   * of the standard library. Override `Configuration::isTaintSanitizerGuard`
+   * of the standard library. Override `Configuration::isSanitizer`
    * for analysis-specific taint steps.
    */
   abstract class AdditionalSanitizerGuardNode extends SanitizerGuardNode {
@@ -159,6 +159,12 @@ module TaintTracking {
 
   }
 
+  /**
+   * A sanitizer guard node that only blocks specific flow labels.
+   */
+  abstract class LabeledSanitizerGuardNode extends SanitizerGuardNode, DataFlow::LabeledBarrierGuardNode {
+  }
+
   /**
    * DEPRECATED: Override `Configuration::isAdditionalTaintStep` or use
    * `AdditionalTaintStep` instead.

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

@@ -74,10 +74,12 @@ predicate localFlowStep(DataFlow::Node pred, DataFlow::Node succ,
   any(DataFlow::AdditionalFlowStep afs).step(pred, succ) and predlbl = succlbl
   or
   exists (boolean vp | configuration.isAdditionalFlowStep(pred, succ, vp) |
-    if vp = false and (predlbl = FlowLabel::data() or predlbl = FlowLabel::taint()) then
-      succlbl = FlowLabel::taint()
-    else
-      predlbl = succlbl
+    vp = true and
+    predlbl = succlbl
+    or
+    vp = false and
+    (predlbl = FlowLabel::data() or predlbl = FlowLabel::taint()) and
+    succlbl = FlowLabel::taint()
   )
   or
   configuration.isAdditionalFlowStep(pred, succ, predlbl, succlbl)

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

@@ -488,6 +488,31 @@ module Express {
     override string getKind() {
       result = kind
     }
+
+    override predicate isUserControlledObject() {
+      kind = "body" and
+      exists (ExpressLibraries::BodyParser bodyParser, RouteHandlerExpr expr |
+        expr.getBody() = rh and
+        bodyParser.producesUserControlledObjects() and
+        bodyParser.flowsToExpr(expr.getAMatchingAncestor())
+      )
+      or
+      // If we can't find the middlewares for the route handler,
+      // but all known body parsers are deep, assume req.body is a deep object.
+      kind = "body" and
+      forall(ExpressLibraries::BodyParser bodyParser | bodyParser.producesUserControlledObjects())
+      or
+      kind = "parameter" and
+      exists (DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |
+        this.(DataFlow::MethodCallNode).calls(request, "param")
+        or
+        exists (DataFlow::PropRead base |
+          // `req.query.name`
+          base.accesses(request, "query") and
+          this = base.getAPropertyReference(_)
+        )
+      )
+    }
   }
 
   /**

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

@@ -230,4 +230,53 @@ module ExpressLibraries {
 
   }
 
+  /**
+   * An instance of the Express `body-parser` middleware.
+   */
+  class BodyParser extends DataFlow::InvokeNode {
+    string kind;
+
+    BodyParser() {
+      this = DataFlow::moduleImport("body-parser").getACall() and kind = "json"
+      or
+      exists (string moduleName |
+        (moduleName = "body-parser" or moduleName = "express") and
+        (kind = "json" or kind = "urlencoded") and
+        this = DataFlow::moduleMember(moduleName, kind).getACall()
+      )
+    }
+
+    /**
+     * Holds if this is a JSON body parser.
+     */
+    predicate isJson() {
+      kind = "json"
+    }
+
+    /**
+     * Holds if this is a URL-encoded body parser.
+     */
+    predicate isUrlEncoded() {
+      kind = "urlencoded"
+    }
+
+    /**
+     * Holds if this is an extended URL-encoded body parser.
+     *
+     * The extended URL-encoding allows for nested objects, like JSON.
+     */
+    predicate isExtendedUrlEncoded() {
+      kind = "urlencoded" and
+      not getOptionArgument(0, "extended").mayHaveBooleanValue(false)
+    }
+
+    /**
+     * Holds if this parses the input as JSON or extended URL-encoding, resulting
+     * in user-controlled objects (as opposed to user-controlled strings).
+     */
+    predicate producesUserControlledObjects() {
+      isJson() or isExtendedUrlEncoded()
+    }
+  }
+
 }

javascript/ql/src/semmle/javascript/security/TaintedObject.qll

@@ -0,0 +1,119 @@
+/**
+ * Provides methods for reasoning about the flow of deeply tainted objects, such as JSON objects
+ * parsed from user-controlled data.
+ *
+ * Deeply tainted objects are arrays or objects with user-controlled property names, containing
+ * tainted values or deeply tainted objects in their properties.
+ *
+ * To track deeply tainted objects, a flow-tracking configuration should generally include the following:
+ *
+ * 1. One or more sinks associated with the label `TaintedObject::label()`.
+ * 2. The sources from `TaintedObject::isSource`.
+ * 3. The flow steps from `TaintedObject::step`.
+ * 4. The sanitizing guards `TaintedObject::SanitizerGuard`.
+ */
+import javascript
+
+module TaintedObject {
+  private import DataFlow
+
+  private class TaintedObjectLabel extends FlowLabel {
+    TaintedObjectLabel() { this = "tainted-object" }
+  }
+
+  /**
+   * Gets the flow label representing a deeply tainted object.
+   *
+   * A "tainted object" is an array or object whose property values are all assumed to be tainted as well.
+   *
+   * Note that the presence of the this label generally implies the presence of the `taint` label as well.
+   */
+  FlowLabel label() { result instanceof TaintedObjectLabel  }
+
+  /**
+   * Holds for the flows steps that are relevant for tracking user-controlled JSON objects.
+   */
+  predicate step(Node src, Node trg, FlowLabel inlbl, FlowLabel outlbl) {
+    // JSON parsers map tainted inputs to tainted JSON
+    (inlbl = FlowLabel::data() or inlbl = FlowLabel::taint()) and
+    outlbl = label() and
+    exists (JsonParserCall parse |
+      src = parse.getInput() and
+      trg = parse.getOutput())
+    or
+    // Property reads preserve deep object taint.
+    inlbl = label() and
+    outlbl = label() and
+    trg.(PropRead).getBase() = src
+    or
+    // Property projection preserves deep object taint
+    inlbl = label() and
+    outlbl = label() and
+    trg.(PropertyProjection).getObject() = src
+    or
+    // Extending objects preserves deep object taint
+    inlbl = label() and
+    outlbl = label() and
+    exists (ExtendCall call |
+      src = call.getAnOperand() and
+      trg = call
+      or
+      src = call.getASourceOperand() and
+      trg = call.getDestinationOperand().getALocalSource())
+  }
+
+  /**
+   * Holds if `node` is a source of JSON taint and label is the JSON taint label.
+   */
+  predicate isSource(Node source, FlowLabel label) {
+    source instanceof Source and label = label()
+  }
+
+  /**
+   * A source of a user-controlled deep object.
+   */
+  abstract class Source extends DataFlow::Node {}
+
+  /** Request input accesses as a JSON source. */
+  private class RequestInputAsSource extends Source {
+    RequestInputAsSource() {
+      this.(HTTP::RequestInputAccess).isUserControlledObject()
+    }
+  }
+
+  /**
+   * Sanitizer guard that blocks deep object taint.
+   */
+  abstract class SanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode {
+    override FlowLabel getALabel() {
+      result = label()
+    }
+  }
+
+  /**
+   * A test of form `typeof x === "something"`, preventing `x` from being an object in some cases.
+   */
+  private class TypeTestGuard extends SanitizerGuard, ValueNode {
+    override EqualityTest astNode;
+    TypeofExpr typeof;
+    boolean polarity;
+
+    TypeTestGuard() {
+      astNode.getAnOperand() = typeof and
+      (
+        // typeof x === "object" sanitizes `x` when it evaluates to false
+        astNode.getAnOperand().getStringValue() = "object" and
+        polarity = astNode.getPolarity().booleanNot()
+        or
+        // typeof x === "string" sanitizes `x` when it evaluates to true
+        astNode.getAnOperand().getStringValue() != "object" and
+        polarity = astNode.getPolarity()
+      )
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      polarity = outcome and
+      e = typeof.getOperand()
+    }
+  }
+}

javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll

@@ -65,6 +65,11 @@ module ClientSideUrlRedirect {
       queryAccess(pred, succ) and
       f instanceof DocumentUrl and
       g = DataFlow::FlowLabel::taint()
+      or
+      // preserve document.url label in step from `location` to `location.href`
+      f instanceof DocumentUrl and
+      g instanceof DocumentUrl and
+      succ.(DataFlow::PropRead).accesses(pred, "href")
     }
   }
 

javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll

@@ -4,6 +4,7 @@
  */
 
 import javascript
+import semmle.javascript.security.TaintedObject
 
 module NosqlInjection {
   /**
@@ -14,7 +15,16 @@ module NosqlInjection {
   /**
    * A data flow sink for SQL-injection vulnerabilities.
    */
-  abstract class Sink extends DataFlow::Node { }
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets a flow label relevant for this sink.
+     *
+     * Defaults to deeply tainted objects only.
+     */
+    DataFlow::FlowLabel getAFlowLabel() {
+      result = TaintedObject::label()
+    }
+  }
 
   /**
    * A sanitizer for SQL-injection vulnerabilities.
@@ -31,8 +41,12 @@ module NosqlInjection {
       source instanceof Source
     }
 
-    override predicate isSink(DataFlow::Node sink) {
-      sink instanceof Sink
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
+      TaintedObject::isSource(source, label)
+    }
+
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
+      sink.(Sink).getAFlowLabel() = label
     }
 
     override predicate isSanitizer(DataFlow::Node node) {
@@ -40,12 +54,20 @@ module NosqlInjection {
       node instanceof Sanitizer
     }
 
-    override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
+      guard instanceof TaintedObject::SanitizerGuard
+    }
+
+    override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl) {
+      TaintedObject::step(src, trg, inlbl, outlbl)
+      or
       // additional flow step to track taint through NoSQL query objects
+      inlbl = TaintedObject::label() and
+      outlbl = TaintedObject::label() and
       exists (NoSQL::Query query, DataFlow::SourceNode queryObj |
         queryObj.flowsToExpr(query) and
-        queryObj.flowsTo(succ) and
-        pred = queryObj.getAPropertyWrite().getRhs()
+        queryObj.flowsTo(trg) and
+        src = queryObj.getAPropertyWrite().getRhs()
       )
     }
   }

javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll

@@ -10,6 +10,11 @@ import semmle.javascript.security.dataflow.DOM
 abstract class RemoteFlowSource extends DataFlow::Node {
   /** Gets a string that describes the type of this remote flow source. */
   abstract string getSourceType();
+
+  /**
+   * Holds if this can be a user-controlled object, such as a JSON object parsed from user-controlled data.
+   */
+  predicate isUserControlledObject() { none() }
 }
 
 /**