Merge pull request #10369 from alexrford/rb/sensitive-get-query

Ruby: add `rb/sensitive-get-query` query

ruby/ql/lib/codeql/ruby/Concepts.qll

@@ -250,6 +250,11 @@ module Http {
 
       /** Gets a string that identifies the framework used for this route setup. */
       string getFramework() { result = super.getFramework() }
+
+      /**
+       * Gets the HTTP method name, in lowercase, that this handler will respond to.
+       */
+      string getHttpMethod() { result = super.getHttpMethod() }
     }
 
     /** Provides a class for modeling new HTTP routing APIs. */
@@ -287,6 +292,11 @@ module Http {
 
         /** Gets a string that identifies the framework used for this route setup. */
         abstract string getFramework();
+
+        /**
+         * Gets the HTTP method name, in all caps, that this handler will respond to.
+         */
+        abstract string getHttpMethod();
       }
     }
 
@@ -395,6 +405,12 @@ module Http {
 
       /** Gets a string that identifies the framework used for this route setup. */
       string getFramework() { result = super.getFramework() }
+
+      /**
+       * Gets an HTTP method name, in all caps, that this handler will respond to.
+       * Handlers can potentially respond to multiple HTTP methods.
+       */
+      string getAnHttpMethod() { result = super.getAnHttpMethod() }
     }
 
     /** Provides a class for modeling new HTTP request handlers. */
@@ -416,6 +432,12 @@ module Http {
 
         /** Gets a string that identifies the framework used for this request handler. */
         abstract string getFramework();
+
+        /**
+         * Gets an HTTP method name, in all caps, that this handler will respond to.
+         * Handlers can potentially respond to multiple HTTP methods.
+         */
+        abstract string getAnHttpMethod();
       }
     }
 
@@ -430,6 +452,8 @@ module Http {
       }
 
       override string getFramework() { result = rs.getFramework() }
+
+      override string getAnHttpMethod() { result = rs.getHttpMethod() }
     }
 
     /** A parameter that will receive parts of the url when handling an incoming request. */

ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll

@@ -687,6 +687,15 @@ module ExprNodes {
     final ExprCfgNode getValue() { e.hasCfgChild(e.getValue(), this, result) }
   }
 
+  /** A control-flow node that wraps a `VariableAccess` AST expression. */
+  class VariableAccessCfgNode extends ExprCfgNode {
+    override string getAPrimaryQlClass() { result = "VariableAccessCfgNode" }
+
+    override VariableAccess e;
+
+    final override VariableAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+  }
+
   /** A control-flow node that wraps a `VariableReadAccess` AST expression. */
   class VariableReadAccessCfgNode extends ExprCfgNode {
     override string getAPrimaryQlClass() { result = "VariableReadAccessCfgNode" }

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

@@ -83,6 +83,8 @@ class ActionControllerActionMethod extends Method, Http::Server::RequestHandler:
 
   override string getFramework() { result = "ActionController" }
 
+  override string getAnHttpMethod() { result = this.getARoute().getHttpMethod() }
+
   /** Gets a call to render from within this method. */
   Rails::RenderCall getARenderCall() { result.getParent+() = this }
 

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

@@ -84,6 +84,9 @@ private class GraphqlSchemaResolverClass extends ClassDeclaration {
   }
 }
 
+/** Gets an HTTP method that is supported for querying a GraphQL server. */
+private string getASupportedHttpMethod() { result = ["get", "post"] }
+
 /**
  * A `ClassDeclaration` for a class that extends `GraphQL::Schema::Object`.
  * For example,
@@ -172,6 +175,8 @@ class GraphqlResolveMethod extends Method, Http::Server::RequestHandler::Range {
 
   override string getFramework() { result = "GraphQL" }
 
+  override string getAnHttpMethod() { result = getASupportedHttpMethod() }
+
   /** Gets the mutation class containing this method. */
   GraphqlResolvableClass getMutationClass() { result = resolvableClass }
 }
@@ -219,6 +224,8 @@ class GraphqlLoadMethod extends Method, Http::Server::RequestHandler::Range {
 
   override string getFramework() { result = "GraphQL" }
 
+  override string getAnHttpMethod() { result = getASupportedHttpMethod() }
+
   /** Gets the mutation class containing this method. */
   GraphqlResolvableClass getMutationClass() { result = resolvableClass }
 }
@@ -388,6 +395,8 @@ class GraphqlFieldResolutionMethod extends Method, Http::Server::RequestHandler:
 
   override string getFramework() { result = "GraphQL" }
 
+  override string getAnHttpMethod() { result = getASupportedHttpMethod() }
+
   /** Gets the class containing this method. */
   GraphqlSchemaObjectClass getGraphqlClass() { result = schemaObjectClass }
 }

ruby/ql/lib/codeql/ruby/security/SensitiveActions.qll

@@ -11,6 +11,144 @@
 
 private import codeql.ruby.AST
 private import codeql.ruby.DataFlow
+import codeql.ruby.security.internal.SensitiveDataHeuristics
+private import HeuristicNames
+private import codeql.ruby.CFG
+
+/** An expression that might contain sensitive data. */
+cached
+abstract class SensitiveNode extends DataFlow::Node {
+  /** Gets a human-readable description of this expression for use in alert messages. */
+  cached
+  abstract string describe();
+
+  /** Gets a classification of the kind of sensitive data this expression might contain. */
+  cached
+  abstract SensitiveDataClassification getClassification();
+}
+
+/** A method call that might produce sensitive data. */
+class SensitiveCall extends SensitiveNode instanceof DataFlow::CallNode {
+  SensitiveDataClassification classification;
+
+  SensitiveCall() {
+    classification = this.getMethodName().(SensitiveDataMethodName).getClassification()
+    or
+    // This is particularly to pick up methods with an argument like "password", which
+    // may indicate a lookup.
+    exists(string s | super.getArgument(_).asExpr().getConstantValue().isStringlikeValue(s) |
+      nameIndicatesSensitiveData(s, classification)
+    )
+  }
+
+  override string describe() { result = "a call to " + super.getMethodName() }
+
+  override SensitiveDataClassification getClassification() { result = classification }
+}
+
+/** An access to a variable or hash value that might contain sensitive data. */
+abstract class SensitiveVariableAccess extends SensitiveNode {
+  string name;
+
+  SensitiveVariableAccess() {
+    this.asExpr().(CfgNodes::ExprNodes::VariableAccessCfgNode).getExpr().getVariable().hasName(name)
+    or
+    this.asExpr()
+        .(CfgNodes::ExprNodes::ElementReferenceCfgNode)
+        .getAnArgument()
+        .getConstantValue()
+        .isStringlikeValue(name)
+  }
+
+  override string describe() { result = "an access to " + name }
+}
+
+/** A write to a location that might contain sensitive data. */
+abstract class SensitiveWrite extends DataFlow::Node { }
+
+/**
+ * Holds if `node` is a write to a variable or hash value named `name`.
+ *
+ * Helper predicate factored out for performance,
+ * to filter `name` as much as possible before using it in
+ * regex matching.
+ */
+pragma[nomagic]
+private predicate writesProperty(DataFlow::Node node, string name) {
+  exists(VariableWriteAccess vwa | vwa.getVariable().getName() = name |
+    node.asExpr().getExpr() = vwa
+  )
+  or
+  // hash value assignment
+  node.(DataFlow::CallNode).getMethodName() = "[]=" and
+  node.(DataFlow::CallNode).getArgument(0).asExpr().getConstantValue().isStringlikeValue(name)
+}
+
+/**
+ * Instance and class variable names are reported with their respective `@`
+ * and `@@` prefixes. This predicate strips these prefixes.
+ */
+bindingset[name]
+private string unprefixedVariableName(string name) { result = name.regexpReplaceAll("^@*", "") }
+
+/** A write to a variable or property that might contain sensitive data. */
+private class BasicSensitiveWrite extends SensitiveWrite {
+  SensitiveDataClassification classification;
+
+  BasicSensitiveWrite() {
+    exists(string name |
+      /*
+       * PERFORMANCE OPTIMISATION:
+       * `nameIndicatesSensitiveData` performs a `regexpMatch` on `name`.
+       * To carry out a regex match, we must first compute the Cartesian product
+       * of all possible `name`s and regexes, then match.
+       * To keep this product as small as possible,
+       * we want to filter `name` as much as possible before the product.
+       *
+       * Do this by factoring out a helper predicate containing the filtering
+       * logic that restricts `name`. This helper predicate will get picked first
+       * in the join order, since it is the only call here that binds `name`.
+       */
+
+      writesProperty(this, name) and
+      nameIndicatesSensitiveData(unprefixedVariableName(name), classification)
+    )
+  }
+
+  /** Gets a classification of the kind of sensitive data the write might handle. */
+  SensitiveDataClassification getClassification() { result = classification }
+}
+
+/** An access to a variable or hash value that might contain sensitive data. */
+private class BasicSensitiveVariableAccess extends SensitiveVariableAccess {
+  SensitiveDataClassification classification;
+
+  BasicSensitiveVariableAccess() {
+    nameIndicatesSensitiveData(unprefixedVariableName(name), classification)
+  }
+
+  override SensitiveDataClassification getClassification() { result = classification }
+}
+
+/** A method name that suggests it may be sensitive. */
+abstract class SensitiveMethodName extends string {
+  SensitiveMethodName() { this = any(MethodBase m).getName() }
+}
+
+/** A method name that suggests it may produce sensitive data. */
+abstract class SensitiveDataMethodName extends SensitiveMethodName {
+  /** Gets a classification of the kind of sensitive data this method may produce. */
+  abstract SensitiveDataClassification getClassification();
+}
+
+/** A method name that might return sensitive credential data. */
+class CredentialsMethodName extends SensitiveDataMethodName {
+  SensitiveDataClassification classification;
+
+  CredentialsMethodName() { nameIndicatesSensitiveData(this, classification) }
+
+  override SensitiveDataClassification getClassification() { result = classification }
+}
 
 /**
  * A sensitive action, such as transfer of sensitive data.

ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryCustomizations.qll

@@ -0,0 +1,54 @@
+/**
+ * Provides default sources and sinks for reasoning about sensitive data sourced
+ * from the query string of a GET request, as well as extension points for
+ * adding your own.
+ */
+
+private import codeql.ruby.security.SensitiveActions
+private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
+
+/**
+ * Provides default sources and sinks for reasoning about sensitive data sourced
+ * from the query string of a GET request, as well as extension points for
+ * adding your own.
+ */
+module SensitiveGetQuery {
+  /**
+   * A data flow source representing data sourced from the query string in a
+   * GET request handler.
+   */
+  abstract class Source extends DataFlow::Node {
+    /** Gets the request handler corresponding to this data source. */
+    abstract Http::Server::RequestHandler getHandler();
+  }
+
+  /**
+   * An access to data from the query string of a GET request as a data flow
+   * source.
+   */
+  private class RequestInputAccessSource extends Source instanceof Http::Server::RequestInputAccess {
+    private Http::Server::RequestHandler handler;
+
+    RequestInputAccessSource() {
+      handler = this.asExpr().getExpr().getEnclosingMethod() and
+      handler.getAnHttpMethod() = "get" and
+      this.getKind() = "parameter"
+    }
+
+    override Http::Server::RequestHandler getHandler() { result = handler }
+  }
+
+  /**
+   * A data flow sink suggesting a use of sensitive data.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /** A sensitive data node as a data flow sink. */
+  private class SensitiveNodeSink extends Sink instanceof SensitiveNode {
+    SensitiveNodeSink() {
+      // User names and other similar information is not sensitive in this context.
+      not this.getClassification() = SensitiveDataClassification::id()
+    }
+  }
+}

ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryQuery.qll

@@ -0,0 +1,31 @@
+/**
+ * Provides a taint-tracking configuration for detecting flow of query string
+ * data to sensitive actions in GET query request handlers.
+ *
+ * Note, for performance reasons: only import this file if `Configuration` is
+ * needed, otherwise `SensitiveGetQueryCustomizations` should be imported
+ * instead.
+ */
+
+private import ruby
+private import codeql.ruby.TaintTracking
+
+/**
+ * Provides a taint-tracking configuration for detecting flow of query string
+ * data to sensitive actions in GET query request handlers.
+ */
+module SensitiveGetQuery {
+  import SensitiveGetQueryCustomizations::SensitiveGetQuery
+
+  /**
+   * A taint-tracking configuration for reasoning about use of sensitive data
+   * from a GET request query string.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "SensitiveGetQuery" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+  }
+}

ruby/ql/src/change-notes/2022-09-10-sensitive-get-query.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `rb/sensitive-get-query`, to detect cases where sensitive data is read from the query parameters of an HTTP `GET` request.

ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.qhelp

@@ -0,0 +1,43 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Sensitive information such as passwords should not be transmitted within the query string of the requested URL.
+Sensitive information within URLs may be logged in various locations, including the user's browser, the web server,
+and any proxy servers between the two endpoints. URLs may also be displayed on-screen, bookmarked
+or emailed around by users. They may be disclosed to third parties via the Referer header when any off-site links are
+followed. Placing sensitive information into the URL therefore increases the risk that it will be captured by an attacker.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Use HTTP POST to send sensitive information as part of the request body; for example, as form data.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows two route handlers that both receive a username and a password.
+The first receives this sensitive information from the query parameters of a GET request, which is
+transmitted in the URL. The second receives this sensitive information from the request body of a POST request.
+</p>
+<sample src="examples/routes.rb" />
+<sample src="examples/users_controller.rb" />
+</example>
+
+<references>
+  <li>
+    CWE:
+    <a href="https://cwe.mitre.org/data/definitions/598.html">CWE-598: Use of GET Request Method with Sensitive Query Strings</a>
+  </li>
+  <li>
+    PortSwigger (Burp):
+    <a href="https://portswigger.net/kb/issues/00400300_password-submitted-using-get-method">Password Submitted using GET Method</a>
+  </li>
+  <li>
+    OWASP:
+    <a href="https://owasp.org/www-community/vulnerabilities/Information_exposure_through_query_strings_in_url">Information Exposure through Query Strings in URL</a>
+  </li>
+</references>
+</qhelp>

ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.ql

@@ -0,0 +1,23 @@
+/**
+ * @name Sensitive data read from GET request
+ * @description Placing sensitive data in a GET request increases the risk of
+ *              the data being exposed to an attacker.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 6.5
+ * @precision high
+ * @id rb/sensitive-get-query
+ * @tags security
+ *       external/cwe/cwe-598
+ */
+
+import ruby
+import DataFlow::PathGraph
+import codeql.ruby.security.SensitiveGetQueryQuery
+import codeql.ruby.security.SensitiveActions
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, SensitiveGetQuery::Configuration config
+where config.hasFlowPath(source, sink)
+select source.getNode(), source, sink,
+  "$@ for GET requests uses query parameter as sensitive data.",
+  source.getNode().(SensitiveGetQuery::Source).getHandler(), "Route handler"

ruby/ql/src/queries/security/cwe-598/examples/routes.rb

@@ -0,0 +1,4 @@
+Rails.application.routes.draw do
+  get "users/login", to: "#login_get" # BAD: sensitive data transmitted through query parameters
+  post "users/login", to: "users#login_post" # GOOD: sensitive data transmitted in the request body
+end

ruby/ql/src/queries/security/cwe-598/examples/users_controller.rb

@@ -0,0 +1,16 @@
+class UsersController < ActionController::Base
+  def login_get
+    password = params[:password]
+    authenticate_user(params[:username], password)
+  end
+
+  def login_post
+    password = params[:password]
+    authenticate_user(params[:username], password)
+  end
+
+  private
+  def authenticate_user(username, password)
+    # ... authenticate the user here
+  end
+end

ruby/ql/test/query-tests/security/cwe-598/SensitiveGetQuery.expected

@@ -0,0 +1,24 @@
+edges
+| app/controllers/users_controller.rb:4:11:4:16 | call to params :  | app/controllers/users_controller.rb:4:11:4:27 | ...[...] |
+| app/controllers/users_controller.rb:9:16:9:21 | call to params :  | app/controllers/users_controller.rb:9:16:9:27 | ...[...] :  |
+| app/controllers/users_controller.rb:9:16:9:27 | ...[...] :  | app/controllers/users_controller.rb:10:42:10:49 | password |
+| app/controllers/users_controller.rb:14:5:14:13 | [post] self [@password] :  | app/controllers/users_controller.rb:15:42:15:50 | self [@password] :  |
+| app/controllers/users_controller.rb:14:17:14:22 | call to params :  | app/controllers/users_controller.rb:14:17:14:28 | ...[...] :  |
+| app/controllers/users_controller.rb:14:17:14:28 | ...[...] :  | app/controllers/users_controller.rb:14:5:14:13 | [post] self [@password] :  |
+| app/controllers/users_controller.rb:15:42:15:50 | self [@password] :  | app/controllers/users_controller.rb:15:42:15:50 | @password |
+nodes
+| app/controllers/users_controller.rb:4:11:4:16 | call to params :  | semmle.label | call to params :  |
+| app/controllers/users_controller.rb:4:11:4:27 | ...[...] | semmle.label | ...[...] |
+| app/controllers/users_controller.rb:9:16:9:21 | call to params :  | semmle.label | call to params :  |
+| app/controllers/users_controller.rb:9:16:9:27 | ...[...] :  | semmle.label | ...[...] :  |
+| app/controllers/users_controller.rb:10:42:10:49 | password | semmle.label | password |
+| app/controllers/users_controller.rb:14:5:14:13 | [post] self [@password] :  | semmle.label | [post] self [@password] :  |
+| app/controllers/users_controller.rb:14:17:14:22 | call to params :  | semmle.label | call to params :  |
+| app/controllers/users_controller.rb:14:17:14:28 | ...[...] :  | semmle.label | ...[...] :  |
+| app/controllers/users_controller.rb:15:42:15:50 | @password | semmle.label | @password |
+| app/controllers/users_controller.rb:15:42:15:50 | self [@password] :  | semmle.label | self [@password] :  |
+subpaths
+#select
+| app/controllers/users_controller.rb:4:11:4:16 | call to params | app/controllers/users_controller.rb:4:11:4:16 | call to params :  | app/controllers/users_controller.rb:4:11:4:27 | ...[...] | $@ for GET requests uses query parameter as sensitive data. | app/controllers/users_controller.rb:3:3:6:5 | login_get_1 | Route handler |
+| app/controllers/users_controller.rb:9:16:9:21 | call to params | app/controllers/users_controller.rb:9:16:9:21 | call to params :  | app/controllers/users_controller.rb:10:42:10:49 | password | $@ for GET requests uses query parameter as sensitive data. | app/controllers/users_controller.rb:8:3:11:5 | login_get_2 | Route handler |
+| app/controllers/users_controller.rb:14:17:14:22 | call to params | app/controllers/users_controller.rb:14:17:14:22 | call to params :  | app/controllers/users_controller.rb:15:42:15:50 | @password | $@ for GET requests uses query parameter as sensitive data. | app/controllers/users_controller.rb:13:3:16:5 | login_get_3 | Route handler |

ruby/ql/test/query-tests/security/cwe-598/SensitiveGetQuery.qlref

@@ -0,0 +1 @@
+queries/security/cwe-598/SensitiveGetQuery.ql

ruby/ql/test/query-tests/security/cwe-598/app/controllers/users_controller.rb

@@ -0,0 +1,32 @@
+class UsersController < ApplicationController
+
+  def login_get_1
+    foo = params[:password] # BAD: route handler uses GET query parameters to receive sensitive data
+    authenticate_user(params[:username], foo)
+  end
+
+  def login_get_2
+    password = params[:foo] # BAD: route handler uses GET query parameters to receive sensitive data
+    authenticate_user(params[:username], password)
+  end
+
+  def login_get_3
+    @password = params[:foo] # BAD: route handler uses GET query parameters to receive sensitive data
+    authenticate_user(params[:username], @password)
+  end
+
+  def login_post
+    foo = params[:password] # GOOD: handler uses POST form parameters to receive sensitive data
+    authenticate_user(params[:username], foo)
+  end
+
+  def login_get_cookies
+    foo = cookies[:password] # GOOD: data sourced from cookies rather than (plaintext) query params
+    authenticate_user(params[:username], foo)
+  end
+
+  private
+  def authenticate_user(username, password)
+    # ... authenticate the user here
+  end
+end

ruby/ql/test/query-tests/security/cwe-598/config/routes.rb

@@ -0,0 +1,7 @@
+Rails.application.routes.draw do
+  match "users/login1", to: "users#login_get_1", via: :get
+  get "users/login2", to: "users#login_get_2"
+  get "users/login3", to: "users#login_get_3"
+  post "users/login4", to: "users#login_post"
+  get "users/login5", to: "users#login_get_cookies"
+end