Merge rc/1.19 into next.

javascript/ql/src/semmle/javascript/NodeJS.qll

@@ -132,7 +132,13 @@ predicate findNodeModulesFolder(Folder f, Folder nodeModules, int distance) {
  */
 private class RequireVariable extends Variable {
   RequireVariable() {
-    exists (ModuleScope m | this = m.getVariable("require"))
+    this = any(ModuleScope m).getVariable("require")
+    or
+    // cover cases where we failed to detect Node.js code
+    this.(GlobalVariable).getName() = "require"
+    or
+    // track through assignments to other variables
+    this.getAnAssignedExpr().(VarAccess).getVariable() instanceof RequireVariable
   }
 }
 
@@ -149,7 +155,9 @@ private predicate moduleInFile(Module m, File f) {
 class Require extends CallExpr, Import {
   Require() {
     exists (RequireVariable req |
-      this.getCallee() = req.getAnAccess()
+      this.getCallee() = req.getAnAccess() and
+      // `mjs` files explicitly disallow `require`
+      getFile().getExtension() != "mjs"
     )
   }
 

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

@@ -297,7 +297,22 @@ module NodeJSLib {
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
       pred = tainted and succ = this
     }
+  }
 
+  /**
+   * A model of taint propagation through `new Buffer` and `Buffer.from`.
+   */
+  private class BufferTaintStep extends TaintTracking::AdditionalTaintStep, DataFlow::InvokeNode {
+    BufferTaintStep() {
+      this = DataFlow::globalVarRef("Buffer").getAnInstantiation()
+      or
+      this = DataFlow::globalVarRef("Buffer").getAMemberInvocation("from")
+    }
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      pred = getArgument(0) and
+      succ = this
+    }
   }
 
   /**

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

@@ -0,0 +1,97 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about hard-coded data
+ * being interpreted as code.
+ */
+
+import javascript
+private import semmle.javascript.security.dataflow.CodeInjection
+
+module HardcodedDataInterpretedAsCode {
+  /**
+   * A data flow source for hard-coded data.
+   */
+  abstract class Source extends DataFlow::Node {
+    /** Gets a flow label for which this is a source. */
+    DataFlow::FlowLabel getLabel() {
+      result = DataFlow::FlowLabel::data()
+    }
+  }
+
+  /**
+   * A data flow sink for code injection.
+   */
+  abstract class Sink extends DataFlow::Node {
+    /** Gets a flow label for which this is a sink. */
+    abstract DataFlow::FlowLabel getLabel();
+
+    /** Gets a description of what kind of sink this is. */
+    abstract string getKind();
+  }
+
+  /**
+   * A sanitizer for hard-coded data.
+   */
+  abstract class Sanitizer extends DataFlow::Node {}
+
+  /**
+   * A taint-tracking configuration for reasoning about hard-coded data
+   * being interpreted as code
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() {
+      this = "HardcodedDataInterpretedAsCode"
+    }
+  
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel lbl) {
+      source.(Source).getLabel() = lbl
+    }
+  
+    override predicate isSink(DataFlow::Node nd, DataFlow::FlowLabel lbl) {
+      nd.(Sink).getLabel() = lbl
+    }
+
+    override predicate isSanitizer(DataFlow::Node node) {
+      node instanceof Sanitizer
+    }
+  }
+
+  /**
+   * A constant string consisting of eight or more hexadecimal characters (including at
+   * least one digit), viewed as a source of hard-coded data that should not be
+   * interpreted as code.
+   */
+  private class DefaultSource extends Source, DataFlow::ValueNode {
+    DefaultSource() {
+      exists (string val | val = astNode.(Expr).getStringValue() |
+        val.regexpMatch("[0-9a-fA-F]{8,}") and
+        val.regexpMatch(".*[0-9].*")
+      )
+    }
+  }
+
+  /**
+   * A code injection sink; hard-coded data should not flow here.
+   */
+  private class DefaultCodeInjectionSink extends Sink {
+    DefaultCodeInjectionSink() { this instanceof CodeInjection::Sink }
+    override DataFlow::FlowLabel getLabel() { result = DataFlow::FlowLabel::taint() }
+    override string getKind() { result = "code" }
+  }
+
+  /**
+   * An argument to `require` path; hard-coded data should not flow here.
+   */
+  private class RequireArgumentSink extends Sink {
+    RequireArgumentSink() {
+      this = any(Require r).getAnArgument().flow()
+    }
+
+    override DataFlow::FlowLabel getLabel() {
+      result = DataFlow::FlowLabel::data()
+      or
+      result = DataFlow::FlowLabel::taint()
+    }
+
+    override string getKind() { result = "an import path" }
+  }
+}

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

@@ -36,4 +36,12 @@ module PropertyInjection {
     // Assume that a value that is invoked can refer to a function.
     exists (node.getAnInvocation())
   }
+
+  /**
+   * Holds if the `node` is of form `Object.create(null)` and so it has no prototype.
+   */
+  predicate isPrototypeLessObject(DataFlow::MethodCallNode node) {
+    node = DataFlow::globalVarRef("Object").getAMethodCall("create") and
+    node.getArgument(0).asExpr() instanceof NullLiteral
+  }
 }

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

@@ -1,6 +1,6 @@
 /**
- * Provides a taint tracking configuration for reasoning about injections in 
- * property names, used either for writing into a property, into a header or 
+ * Provides a taint tracking configuration for reasoning about injections in
+ * property names, used either for writing into a property, into a header or
  * for calling an object's method.
  */
 
@@ -18,11 +18,11 @@ module RemotePropertyInjection {
    * A data flow sink for remote property injection.
    */
   abstract class Sink extends DataFlow::Node {
-  
+
     /**
      * Gets a string to identify the different types of sinks.
      */
-    abstract string getMessage();   
+    abstract string getMessage();
   }
 
   /**
@@ -52,58 +52,40 @@ module RemotePropertyInjection {
   }
 
   /**
-   * A source of remote user input, considered as a flow source for remote property 
-   * injection. 
+   * A source of remote user input, considered as a flow source for remote property
+   * injection.
    */
   class RemoteFlowSourceAsSource extends Source {
     RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
   }
 
   /**
-   * A sink for property writes with dynamically computed property name.  
+   * A sink for property writes with dynamically computed property name.
    */
   class PropertyWriteSink extends Sink, DataFlow::ValueNode  {
     PropertyWriteSink() {
-      exists (DataFlow::PropWrite pw | astNode = pw.getPropertyNameExpr()) or 
-      exists (DeleteExpr expr | expr.getOperand().(PropAccess).getPropertyNameExpr() = astNode)               
+      exists (DataFlow::PropWrite pw | astNode = pw.getPropertyNameExpr()) or
+      exists (DeleteExpr expr | expr.getOperand().(PropAccess).getPropertyNameExpr() = astNode)
     }
 
     override string getMessage() {
       result = " a property name to write to."
-    }     
+    }
   }
-  
+
   /**
-   * A sink for method calls using dynamically computed method names.   
-   */
-  class MethodCallSink extends Sink, DataFlow::ValueNode  {
-    MethodCallSink() {
-      exists (DataFlow::PropRead pr | astNode = pr.getPropertyNameExpr() |
-        exists (pr.getAnInvocation()) and
-
-        // Omit sinks covered by the UnsafeDynamicMethodAccess query
-        not PropertyInjection::hasUnsafeMethods(pr.getBase().getALocalSource())
-      )
-    }
-
-    override string getMessage() {
-      result = " a method name to be called."
-    }
-  }
-  
-  /** 
-   * A sink for HTTP header writes with dynamically computed header name. 
-   * This sink avoids double-flagging by ignoring `SetMultipleHeaders` since 
-   * the multiple headers use case consists of an objects containing different 
-   * header names as properties. This case is already handled by 
-   * `PropertyWriteSink`. 
+   * A sink for HTTP header writes with dynamically computed header name.
+   * This sink avoids double-flagging by ignoring `SetMultipleHeaders` since
+   * the multiple headers use case consists of an objects containing different
+   * header names as properties. This case is already handled by
+   * `PropertyWriteSink`.
    */
   class HeaderNameSink extends Sink, DataFlow::ValueNode  {
     HeaderNameSink() {
-      exists (HTTP::ExplicitHeaderDefinition  hd | 
-        not hd instanceof Express::SetMultipleHeaders and 
-        astNode = hd.getNameExpr()        
-      )          
+      exists (HTTP::ExplicitHeaderDefinition  hd |
+        not hd instanceof Express::SetMultipleHeaders and
+        astNode = hd.getNameExpr()
+      )
     }
 
     override string getMessage() {

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

@@ -73,14 +73,6 @@ module UnsafeDynamicMethodAccess {
       PropertyInjection::hasUnsafeMethods(node) // Redefined here so custom queries can override it
     }
 
-    /**
-     * Holds if the `node` is of form `Object.create(null)` and so it has no prototype.
-     */
-    predicate isPrototypeLessObject(DataFlow::MethodCallNode node) {
-      node = DataFlow::globalVarRef("Object").getAMethodCall("create") and
-      node.getArgument(0).asExpr() instanceof NullLiteral
-    }
-
     override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel, DataFlow::FlowLabel dstlabel) {
       // Reading a property of the global object or of a function
       exists (DataFlow::PropRead read |
@@ -92,7 +84,7 @@ module UnsafeDynamicMethodAccess {
       or
       // Reading a chain of properties from any object with a prototype can lead to Function
       exists (PropertyProjection proj |
-        not isPrototypeLessObject(proj.getObject().getALocalSource()) and
+        not PropertyInjection::isPrototypeLessObject(proj.getObject().getALocalSource()) and
         src = proj.getASelector() and
         dst = proj and
         (srclabel = data() or srclabel = taint()) and

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

@@ -0,0 +1,153 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about unvalidated dynamic
+ * method calls.
+ */
+
+import javascript
+import semmle.javascript.frameworks.Express
+import PropertyInjectionShared
+private import semmle.javascript.dataflow.InferredTypes
+
+module UnvalidatedDynamicMethodCall {
+  private import DataFlow::FlowLabel
+
+  /**
+   * A data flow source for unvalidated dynamic method calls.
+   */
+  abstract class Source extends DataFlow::Node {
+    /**
+     * Gets the flow label relevant for this source.
+     */
+    DataFlow::FlowLabel getFlowLabel() {
+      result = data()
+    }
+  }
+
+  /**
+   * A data flow sink for unvalidated dynamic method calls.
+   */
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets the flow label relevant for this sink
+     */
+    abstract DataFlow::FlowLabel getFlowLabel();
+  }
+
+  /**
+   * A sanitizer for unvalidated dynamic method calls.
+   */
+  abstract class Sanitizer extends DataFlow::Node {
+    abstract predicate sanitizes(DataFlow::Node source, DataFlow::Node sink, DataFlow::FlowLabel lbl);
+  }
+
+  /**
+   * A flow label describing values read from a user-controlled property that
+   * may not be functions.
+   */
+  private class MaybeNonFunction extends DataFlow::FlowLabel {
+    MaybeNonFunction() { this = "MaybeNonFunction" }
+  }
+
+  /**
+   * A flow label describing values read from a user-controlled property that
+   * may originate from a prototype object.
+   */
+  private class MaybeFromProto extends DataFlow::FlowLabel {
+    MaybeFromProto() { this = "MaybeFromProto" }
+  }
+
+  /**
+   * A taint-tracking configuration for reasoning about unvalidated dynamic method calls.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "UnvalidatedDynamicMethodCall" }
+
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
+      source.(Source).getFlowLabel() = label
+    }
+
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
+      sink.(Sink).getFlowLabel() = label
+    }
+
+    override predicate isSanitizer(DataFlow::Node nd) {
+      super.isSanitizer(nd) or
+      nd instanceof PropertyInjection::Sanitizer
+    }
+
+    override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel, DataFlow::FlowLabel dstlabel) {
+      exists (DataFlow::PropRead read |
+        src = read.getPropertyNameExpr().flow() and
+        dst = read and
+        (srclabel = data() or srclabel = taint()) and
+        (dstlabel instanceof MaybeNonFunction
+         or
+         // a property of `Object.create(null)` cannot come from a prototype
+         not PropertyInjection::isPrototypeLessObject(read.getBase().getALocalSource()) and
+         dstlabel instanceof MaybeFromProto) and
+        // avoid overlapping results with unsafe dynamic method access query
+        not PropertyInjection::hasUnsafeMethods(read.getBase().getALocalSource())
+      )
+    }
+  }
+
+  /**
+   * A source of remote user input, considered as a source for unvalidated dynamic method calls.
+   */
+  class RemoteFlowSourceAsSource extends Source {
+    RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
+  }
+
+  /**
+   * The page URL considered as a flow source for unvalidated dynamic method calls.
+   */
+  class DocumentUrlAsSource extends Source {
+    DocumentUrlAsSource() { isDocumentURL(asExpr()) }
+  }
+
+  /**
+   * A function invocation of an unsafe function, as a sink for remote unvalidated dynamic method calls.
+   */
+  class CalleeAsSink extends Sink {
+    InvokeExpr invk;
+
+    CalleeAsSink() {
+      this = invk.getCallee().flow() and
+      // don't flag invocations inside a try-catch
+      not invk.getASuccessor() instanceof CatchClause
+    }
+
+    override DataFlow::FlowLabel getFlowLabel() {
+      result instanceof MaybeNonFunction and
+      // don't flag if the type inference can prove that it is a function;
+      // this complements the `FunctionCheck` sanitizer below: the type inference can
+      // detect more checks locally, but doesn't provide inter-procedural reasoning
+      this.analyze().getAType() != TTFunction()
+      or
+      result instanceof MaybeFromProto
+    }
+  }
+
+  /**
+   * A check of the form `typeof x === 'function'`, which sanitizes away the `MaybeNonFunction`
+   * taint kind.
+   */
+  class FunctionCheck extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode {
+    override EqualityTest astNode;
+    TypeofExpr t;
+
+    FunctionCheck() {
+      astNode.getAnOperand().getStringValue() = "function" and
+      astNode.getAnOperand().getUnderlyingValue() = t
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      outcome = astNode.getPolarity() and
+      e = t.getOperand().getUnderlyingValue()
+    }
+
+    override DataFlow::FlowLabel getALabel() {
+      result instanceof MaybeNonFunction
+    }
+  }
+}