Merge pull request #341 from github/rc/3.3

Rc/3.3 mergeback

Author: aibaars

ql/lib/codeql/ruby/ApiGraphs.qll

@@ -188,7 +188,7 @@ module API {
   /** A node corresponding to the use of an API component. */
   class Use extends Node, Impl::MkUse {
     override string toString() {
-      exists(string type | this = Impl::MkUse(_) and type = "Use " |
+      exists(string type | type = "Use " |
         result = type + getPath()
         or
         not exists(this.getPath()) and result = type + "with no path"
@@ -239,20 +239,19 @@ module API {
       /** The root of the API graph. */
       MkRoot() or
       /** A use of an API member at the node `nd`. */
-      MkUse(DataFlow::Node nd) { use(_, _, nd) }
+      MkUse(DataFlow::Node nd) { isUse(nd) }
 
     private string resolveTopLevel(ConstantReadAccess read) {
       TResolved(result) = resolveScopeExpr(read) and
       not result.matches("%::%")
     }
 
     /**
-     * Holds if `ref` is a use of a node that should have an incoming edge from `base` labeled
-     * `lbl` in the API graph.
+     * Holds if `ref` is a use of a node that should have an incoming edge from the root
+     * node labeled `lbl` in the API graph.
      */
     cached
-    predicate use(TApiNode base, string lbl, DataFlow::Node ref) {
-      base = MkRoot() and
+    predicate useRoot(string lbl, DataFlow::Node ref) {
       exists(string name, ExprNodes::ConstantAccessCfgNode access, ConstantReadAccess read |
         access = ref.asExpr() and
         lbl = Label::member(read.getName()) and
@@ -264,7 +263,14 @@ module API {
         not exists(resolveTopLevel(read)) and
         not exists(read.getScopeExpr())
       )
-      or
+    }
+
+    /**
+     * Holds if `ref` is a use of a node that should have an incoming edge from use node
+     * `base` labeled `lbl` in the API graph.
+     */
+    cached
+    predicate useUse(DataFlow::LocalSourceNode base, string lbl, DataFlow::Node ref) {
       exists(ExprCfgNode node |
         // First, we find a predecessor of the node `ref` that we want to determine. The predecessor
         // is any node that is a type-tracked use of a data flow node (`src`), which is itself a
@@ -307,9 +313,15 @@ module API {
     }
 
     pragma[nomagic]
-    private predicate useExpr(ExprCfgNode node, TApiNode base) {
-      exists(DataFlow::LocalSourceNode src, DataFlow::LocalSourceNode pred |
-        use(base, src) and
+    private predicate isUse(DataFlow::Node nd) {
+      useRoot(_, nd)
+      or
+      useUse(_, _, nd)
+    }
+
+    pragma[nomagic]
+    private predicate useExpr(ExprCfgNode node, DataFlow::LocalSourceNode src) {
+      exists(DataFlow::LocalSourceNode pred |
         pred = trackUseNode(src) and
         pred.flowsTo(any(DataFlow::ExprNode n | n.getExprNode() = node))
       )
@@ -331,7 +343,7 @@ module API {
       // recursive case, so instead we check it explicitly here.
       src instanceof DataFlow::LocalSourceNode and
       t.start() and
-      use(_, src) and
+      isUse(src) and
       result = src
       or
       exists(TypeTracker t2 | result = trackUseNode(src, t2).track(t2, t))
@@ -353,9 +365,14 @@ module API {
     cached
     predicate edge(TApiNode pred, string lbl, TApiNode succ) {
       /* Every node that is a use of an API component is itself added to the API graph. */
-      exists(DataFlow::LocalSourceNode ref |
-        use(pred, lbl, ref) and
-        succ = MkUse(ref)
+      exists(DataFlow::LocalSourceNode ref | succ = MkUse(ref) |
+        pred = MkRoot() and
+        useRoot(lbl, ref)
+        or
+        exists(DataFlow::Node nd |
+          pred = MkUse(nd) and
+          useUse(nd, lbl, ref)
+        )
       )
     }
 

ql/lib/codeql/ruby/Concepts.qll

@@ -419,6 +419,15 @@ module HTTP {
 
       /** Gets a string that identifies the framework used for this request. */
       string getFramework() { result = super.getFramework() }
+
+      /**
+       * Holds if this request is made using a mode that disables SSL/TLS
+       * certificate validation, where `disablingNode` represents the point at
+       * which the validation was disabled.
+       */
+      predicate disablesCertificateValidation(DataFlow::Node disablingNode) {
+        super.disablesCertificateValidation(disablingNode)
+      }
     }
 
     /** Provides a class for modeling new HTTP requests. */
@@ -435,6 +444,13 @@ module HTTP {
 
         /** Gets a string that identifies the framework used for this request. */
         abstract string getFramework();
+
+        /**
+         * Holds if this request is made using a mode that disables SSL/TLS
+         * certificate validation, where `disablingNode` represents the point at
+         * which the validation was disabled.
+         */
+        abstract predicate disablesCertificateValidation(DataFlow::Node disablingNode);
       }
     }
 

ql/lib/codeql/ruby/ast/Erb.qll

@@ -260,8 +260,28 @@ class ErbFile extends File {
    */
   predicate isPartial() { this.getStem().charAt(0) = "_" }
 
+  /**
+   * Gets the base template name associated with this ERB file.
+   * For instance, a file named `foo.html.erb` has a template name of `foo`.
+   * A partial template file named `_item.html.erb` has a template name of `item`.
+   */
+  string getTemplateName() { none() }
+
   /**
    * Gets the erb template contained within this file.
    */
   ErbTemplate getTemplate() { result = template }
 }
+
+private class PartialErbFile extends ErbFile {
+  PartialErbFile() { this.isPartial() }
+
+  // Drop the leading underscore
+  override string getTemplateName() { result = this.getStem().splitAt(".", 0).suffix(1) }
+}
+
+private class FullErbFile extends ErbFile {
+  FullErbFile() { not this.isPartial() }
+
+  override string getTemplateName() { result = this.getStem().splitAt(".", 0) }
+}

ql/lib/codeql/ruby/ast/Literal.qll

@@ -194,6 +194,13 @@ class BooleanLiteral extends Literal, TBooleanLiteral {
 
   /** Holds if the Boolean literal is `false` or `FALSE`. */
   predicate isFalse() { none() }
+
+  /** Gets the value of this Boolean literal. */
+  boolean getValue() {
+    this.isTrue() and result = true
+    or
+    this.isFalse() and result = false
+  }
 }
 
 private class TrueLiteral extends BooleanLiteral, TTrueLiteral {
@@ -750,7 +757,7 @@ class HashLiteral extends Literal, THashLiteral {
   final override string getAPrimaryQlClass() { result = "HashLiteral" }
 
   /**
-   * Gets the `n`th element in this array literal.
+   * Gets the `n`th element in this hash literal.
    *
    * In the following example, the 0th element is a `Pair`, and the 1st element
    * is a `HashSplatExpr`.
@@ -761,7 +768,7 @@ class HashLiteral extends Literal, THashLiteral {
    */
   final Expr getElement(int n) { toGenerated(result) = g.getChild(n) }
 
-  /** Gets an element in this array literal. */
+  /** Gets an element in this hash literal. */
   final Expr getAnElement() { result = this.getElement(_) }
 
   /** Gets a key-value `Pair` in this hash literal. */

ql/lib/codeql/ruby/frameworks/ActionView.qll

@@ -73,48 +73,30 @@ private class ActionViewParamsCall extends ActionViewContextCall, ParamsCall { }
 abstract class RenderCall extends MethodCall {
   RenderCall() { this.getMethodName() = "render" }
 
-  private string getWorkingDirectory() {
-    result = this.getLocation().getFile().getParentContainer().getAbsolutePath()
+  private Expr getTemplatePathArgument() {
+    // TODO: support other ways of specifying paths (e.g. `file`)
+    result = [this.getKeywordArgument(["partial", "template", "action"]), this.getArgument(0)]
   }
 
-  bindingset[templatePath]
-  private string templatePathPattern(string templatePath) {
-    exists(string basename, string relativeRoot |
-      // everything after the final slash, or the whole string if there is no slash
-      basename = templatePath.regexpCapture("^(?:.*/)?([^/]*)$", 1) and
-      // everything up to and including the final slash
-      relativeRoot = templatePath.regexpCapture("^(.*/)?(?:[^/]*?)$", 1)
-    |
-      (
-        // path relative to <source_prefix>/app/views/
-        result = "%/app/views/" + relativeRoot + "%" + basename + "%"
-        or
-        // relative to file containing call
-        result = this.getWorkingDirectory() + "%" + templatePath + "%"
-      )
-    )
+  private string getTemplatePathValue() { result = this.getTemplatePathArgument().getValueText() }
+
+  // everything up to and including the final slash, but ignoring any leading slash
+  private string getSubPath() {
+    result = this.getTemplatePathValue().regexpCapture("^/?(.*/)?(?:[^/]*?)$", 1)
   }
 
-  private string getTemplatePathPatterns() {
-    exists(string templatePath |
-      exists(Expr arg |
-        // TODO: support other ways of specifying paths (e.g. `file`)
-        arg = this.getKeywordArgument("partial") or
-        arg = this.getKeywordArgument("template") or
-        arg = this.getKeywordArgument("action") or
-        arg = this.getArgument(0)
-      |
-        templatePath = arg.(StringlikeLiteral).getValueText()
-      )
-    |
-      result = this.templatePathPattern(templatePath)
-    )
+  // everything after the final slash, or the whole string if there is no slash
+  private string getBaseName() {
+    result = this.getTemplatePathValue().regexpCapture("^/?(?:.*/)?([^/]*?)$", 1)
   }
 
   /**
-   * Get the template file to be rendered by this call, if any.
+   * Gets the template file to be rendered by this call, if any.
    */
-  ErbFile getTemplateFile() { result.getAbsolutePath().matches(this.getTemplatePathPatterns()) }
+  ErbFile getTemplateFile() {
+    result.getTemplateName() = this.getBaseName() and
+    result.getRelativePath().matches("%app/views/" + this.getSubPath() + "%")
+  }
 
   /**
    * Get the local variables passed as context to the renderer

ql/lib/codeql/ruby/frameworks/http_clients/Excon.qll

@@ -18,29 +18,113 @@ private import codeql.ruby.ApiGraphs
  * https://github.com/excon/excon/blob/master/README.md
  */
 class ExconHttpRequest extends HTTP::Client::Request::Range {
-  DataFlow::Node request;
-  DataFlow::CallNode responseBody;
+  DataFlow::Node requestUse;
+  API::Node requestNode;
+  API::Node connectionNode;
 
   ExconHttpRequest() {
-    exists(API::Node requestNode | request = requestNode.getAnImmediateUse() |
-      requestNode =
-        [
-          // one-off requests
-          API::getTopLevelMember("Excon"),
-          // connection re-use
-          API::getTopLevelMember("Excon").getInstance()
-        ]
-            .getReturn([
-                // Excon#request exists but Excon.request doesn't.
-                // This shouldn't be a problem - in real code the latter would raise NoMethodError anyway.
-                "get", "head", "delete", "options", "post", "put", "patch", "trace", "request"
-              ]) and
-      responseBody = requestNode.getAMethodCall("body") and
-      this = request.asExpr().getExpr()
-    )
+    requestUse = requestNode.getAnImmediateUse() and
+    connectionNode =
+      [
+        // one-off requests
+        API::getTopLevelMember("Excon"),
+        // connection re-use
+        API::getTopLevelMember("Excon").getInstance(),
+        API::getTopLevelMember("Excon").getMember("Connection").getInstance()
+      ] and
+    requestNode =
+      connectionNode
+          .getReturn([
+              // Excon#request exists but Excon.request doesn't.
+              // This shouldn't be a problem - in real code the latter would raise NoMethodError anyway.
+              "get", "head", "delete", "options", "post", "put", "patch", "trace", "request"
+            ]) and
+    this = requestUse.asExpr().getExpr()
   }
 
-  override DataFlow::Node getResponseBody() { result = responseBody }
+  override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") }
+
+  override predicate disablesCertificateValidation(DataFlow::Node disablingNode) {
+    // Check for `ssl_verify_peer: false` in the options hash.
+    exists(DataFlow::Node arg, int i |
+      i > 0 and arg = connectionNode.getAUse().(DataFlow::CallNode).getArgument(i)
+    |
+      argSetsVerifyPeer(arg, false, disablingNode)
+    )
+    or
+    // Or we see a call to `Excon.defaults[:ssl_verify_peer] = false` before the
+    // request, and no `ssl_verify_peer: true` in the explicit options hash for
+    // the request call.
+    exists(DataFlow::CallNode disableCall |
+      setsDefaultVerification(disableCall, false) and
+      disableCall.asExpr().getASuccessor+() = requestUse.asExpr() and
+      disablingNode = disableCall and
+      not exists(DataFlow::Node arg, int i |
+        i > 0 and arg = connectionNode.getAUse().(DataFlow::CallNode).getArgument(i)
+      |
+        argSetsVerifyPeer(arg, true, _)
+      )
+    )
+  }
 
   override string getFramework() { result = "Excon" }
 }
+
+/**
+ * Holds if `arg` represents an options hash that contains the key
+ * `:ssl_verify_peer` with `value`, where `kvNode` is the data-flow node for
+ * this key-value pair.
+ */
+predicate argSetsVerifyPeer(DataFlow::Node arg, boolean value, DataFlow::Node kvNode) {
+  // Either passed as an individual key:value argument, e.g.:
+  // Excon.get(..., ssl_verify_peer: false)
+  isSslVerifyPeerPair(arg.asExpr().getExpr(), value) and
+  kvNode = arg
+  or
+  // Or as a single hash argument, e.g.:
+  // Excon.get(..., { ssl_verify_peer: false, ... })
+  exists(DataFlow::LocalSourceNode optionsNode, Pair p |
+    p = optionsNode.asExpr().getExpr().(HashLiteral).getAKeyValuePair() and
+    isSslVerifyPeerPair(p, value) and
+    optionsNode.flowsTo(arg) and
+    kvNode.asExpr().getExpr() = p
+  )
+}
+
+/**
+ * Holds if `callNode` sets `Excon.defaults[:ssl_verify_peer]` or
+ * `Excon.ssl_verify_peer` to `value`.
+ */
+private predicate setsDefaultVerification(DataFlow::CallNode callNode, boolean value) {
+  callNode = API::getTopLevelMember("Excon").getReturn("defaults").getAMethodCall("[]=") and
+  isSslVerifyPeerLiteral(callNode.getArgument(0)) and
+  hasBooleanValue(callNode.getArgument(1), value)
+  or
+  callNode = API::getTopLevelMember("Excon").getAMethodCall("ssl_verify_peer=") and
+  hasBooleanValue(callNode.getArgument(0), value)
+}
+
+private predicate isSslVerifyPeerLiteral(DataFlow::Node node) {
+  exists(DataFlow::LocalSourceNode literal |
+    literal.asExpr().getExpr().(SymbolLiteral).getValueText() = "ssl_verify_peer" and
+    literal.flowsTo(node)
+  )
+}
+
+/** Holds if `node` can contain `value`. */
+private predicate hasBooleanValue(DataFlow::Node node, boolean value) {
+  exists(DataFlow::LocalSourceNode literal |
+    literal.asExpr().getExpr().(BooleanLiteral).getValue() = value and
+    literal.flowsTo(node)
+  )
+}
+
+/** Holds if `p` is the pair `ssl_verify_peer: <value>`. */
+private predicate isSslVerifyPeerPair(Pair p, boolean value) {
+  exists(DataFlow::Node key, DataFlow::Node valueNode |
+    key.asExpr().getExpr() = p.getKey() and valueNode.asExpr().getExpr() = p.getValue()
+  |
+    isSslVerifyPeerLiteral(key) and
+    hasBooleanValue(valueNode, value)
+  )
+}