Merge pull request github#10602 from hmac/hmac/actiondispatch-request

Ruby: Model ActionDispatch::Request

Author: hmac

ruby/ql/lib/change-notes/2022-10-13-actiondispatch-request.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* More sources of remote input arising from methods on `ActionDispatch::Request`
+  are now recognised.

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

@@ -290,6 +290,26 @@ module Http {
       }
     }
 
+    /** A kind of request input. */
+    class RequestInputKind extends string {
+      RequestInputKind() { this = ["parameter", "header", "body", "url", "cookie"] }
+    }
+
+    /** Input from the parameters of a request. */
+    RequestInputKind parameterInputKind() { result = "parameter" }
+
+    /** Input from the headers of a request. */
+    RequestInputKind headerInputKind() { result = "header" }
+
+    /** Input from the body of a request. */
+    RequestInputKind bodyInputKind() { result = "body" }
+
+    /** Input from the URL of a request. */
+    RequestInputKind urlInputKind() { result = "url" }
+
+    /** Input from the cookies of a request. */
+    RequestInputKind cookieInputKind() { result = "cookie" }
+
     /**
      * An access to a user-controlled HTTP request input. For example, the URL or body of a request.
      * Instances of this class automatically become `RemoteFlowSource`s.
@@ -304,6 +324,32 @@ module Http {
        * This is typically the name of the method that gives rise to this input.
        */
       string getSourceType() { result = super.getSourceType() }
+
+      /**
+       * Gets the kind of the accessed input,
+       * Can be one of "parameter", "header", "body", "url", "cookie".
+       */
+      RequestInputKind getKind() { result = super.getKind() }
+
+      /**
+       * Holds if this part of the request may be controlled by a third party,
+       * that is, an agent other than the one who sent the request.
+       *
+       * This is true for the URL, query parameters, and request body.
+       * These can be controlled by a malicious third party in the following scenarios:
+       *
+       * - The user clicks a malicious link or is otherwise redirected to a malicious URL.
+       * - The user visits a web site that initiates a form submission or AJAX request on their behalf.
+       *
+       * In these cases, the request is technically sent from the user's browser, but
+       * the user is not in direct control of the URL or POST body.
+       *
+       * Headers are never considered third-party controllable by this predicate, although the
+       * third party does have some control over the the Referer and Origin headers.
+       */
+      predicate isThirdPartyControllable() {
+        this.getKind() = [parameterInputKind(), urlInputKind(), bodyInputKind()]
+      }
     }
 
     /** Provides a class for modeling new HTTP request inputs. */
@@ -321,6 +367,12 @@ module Http {
          * This is typically the name of the method that gives rise to this input.
          */
         abstract string getSourceType();
+
+        /**
+         * Gets the kind of the accessed input,
+         * Can be one of "parameter", "header", "body", "url", "cookie".
+         */
+        abstract RequestInputKind getKind();
       }
     }
 
@@ -387,6 +439,8 @@ module Http {
       RoutedParameter() { this.getParameter() = handler.getARoutedParameter() }
 
       override string getSourceType() { result = handler.getFramework() + " RoutedParameter" }
+
+      override RequestInputKind getKind() { result = parameterInputKind() }
     }
 
     /**

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

@@ -139,6 +139,8 @@ class ParamsSource extends Http::Server::RequestInputAccess::Range {
   ParamsSource() { this.asExpr().getExpr() instanceof Rails::ParamsCall }
 
   override string getSourceType() { result = "ActionController::Metal#params" }
+
+  override Http::Server::RequestInputKind getKind() { result = Http::Server::parameterInputKind() }
 }
 
 /**
@@ -149,6 +151,8 @@ class CookiesSource extends Http::Server::RequestInputAccess::Range {
   CookiesSource() { this.asExpr().getExpr() instanceof Rails::CookiesCall }
 
   override string getSourceType() { result = "ActionController::Metal#cookies" }
+
+  override Http::Server::RequestInputKind getKind() { result = Http::Server::cookieInputKind() }
 }
 
 /** A call to `cookies` from within a controller. */
@@ -161,6 +165,140 @@ private class ActionControllerParamsCall extends ActionControllerContextCall, Pa
   ActionControllerParamsCall() { this.getMethodName() = "params" }
 }
 
+/** Modeling for `ActionDispatch::Request`. */
+private module Request {
+  /**
+   * A call to `request` from within a controller. This is an instance of
+   * `ActionDispatch::Request`.
+   */
+  private class RequestNode extends DataFlow::CallNode {
+    RequestNode() {
+      this.asExpr().getExpr() instanceof ActionControllerContextCall and
+      this.getMethodName() = "request"
+    }
+  }
+
+  /**
+   * A method call on `request`.
+   */
+  private class RequestMethodCall extends DataFlow::CallNode {
+    RequestMethodCall() {
+      any(RequestNode r).(DataFlow::LocalSourceNode).flowsTo(this.getReceiver())
+    }
+  }
+
+  abstract private class RequestInputAccess extends RequestMethodCall,
+    Http::Server::RequestInputAccess::Range {
+    override string getSourceType() { result = "ActionDispatch::Request#" + this.getMethodName() }
+  }
+
+  /**
+   * A method call on `request` which returns request parameters.
+   */
+  private class ParametersCall extends RequestInputAccess {
+    ParametersCall() {
+      this.getMethodName() =
+        [
+          "parameters", "params", "GET", "POST", "query_parameters", "request_parameters",
+          "filtered_parameters"
+        ]
+    }
+
+    override Http::Server::RequestInputKind getKind() {
+      result = Http::Server::parameterInputKind()
+    }
+  }
+
+  /** A method call on `request` which returns part or all of the request path. */
+  private class PathCall extends RequestInputAccess {
+    PathCall() {
+      this.getMethodName() =
+        ["path", "filtered_path", "fullpath", "original_fullpath", "original_url", "url"]
+    }
+
+    override Http::Server::RequestInputKind getKind() { result = Http::Server::urlInputKind() }
+  }
+
+  /** A method call on `request` which returns a specific request header. */
+  private class HeadersCall extends RequestInputAccess {
+    HeadersCall() {
+      this.getMethodName() =
+        [
+          "authorization", "script_name", "path_info", "user_agent", "referer", "referrer",
+          "host_authority", "content_type", "host", "hostname", "accept_encoding",
+          "accept_language", "if_none_match", "if_none_match_etags", "content_mime_type"
+        ]
+      or
+      // Request headers are prefixed with `HTTP_` to distinguish them from
+      // "headers" supplied by Rack middleware.
+      this.getMethodName() = ["get_header", "fetch_header"] and
+      this.getArgument(0).asExpr().getExpr().getConstantValue().getString().regexpMatch("^HTTP_.+")
+    }
+
+    override Http::Server::RequestInputKind getKind() { result = Http::Server::headerInputKind() }
+  }
+
+  // TODO: each_header
+  /**
+   * A method call on `request` which returns part or all of the host.
+   * This can be influenced by headers such as Host and X-Forwarded-Host.
+   */
+  private class HostCall extends RequestInputAccess {
+    HostCall() {
+      this.getMethodName() =
+        [
+          "authority", "host", "host_authority", "host_with_port", "hostname", "forwarded_for",
+          "forwarded_host", "port", "forwarded_port"
+        ]
+    }
+
+    override Http::Server::RequestInputKind getKind() { result = Http::Server::headerInputKind() }
+  }
+
+  /**
+   * A method call on `request` which is influenced by one or more request
+   * headers.
+   */
+  private class HeaderTaintedCall extends RequestInputAccess {
+    HeaderTaintedCall() {
+      this.getMethodName() = ["media_type", "media_type_params", "content_charset", "base_url"]
+    }
+
+    override Http::Server::RequestInputKind getKind() { result = Http::Server::headerInputKind() }
+  }
+
+  /** A method call on `request` which returns the request body. */
+  private class BodyCall extends RequestInputAccess {
+    BodyCall() { this.getMethodName() = ["body", "raw_post"] }
+
+    override Http::Server::RequestInputKind getKind() { result = Http::Server::bodyInputKind() }
+  }
+
+  /**
+   * A method call on `request` which returns the rack env.
+   * This is a hash containing all the information about the request. Values
+   * under keys starting with `HTTP_` are user-controlled.
+   */
+  private class EnvCall extends RequestMethodCall {
+    EnvCall() { this.getMethodName() = ["env", "filtered_env"] }
+  }
+
+  /**
+   * A read of a user-controlled parameter from the request env.
+   */
+  private class EnvHttpAccess extends DataFlow::CallNode, Http::Server::RequestInputAccess::Range {
+    EnvHttpAccess() {
+      any(EnvCall c).(DataFlow::LocalSourceNode).flowsTo(this.getReceiver()) and
+      this.getMethodName() = "[]" and
+      this.getArgument(0).asExpr().getExpr().getConstantValue().getString().regexpMatch("^HTTP_.+")
+    }
+
+    override Http::Server::RequestInputKind getKind() { result = Http::Server::headerInputKind() }
+
+    override string getSourceType() { result = "ActionDispatch::Request#env[]" }
+  }
+}
+
 /** A call to `render` from within a controller. */
 private class ActionControllerRenderCall extends ActionControllerContextCall, RenderCallImpl {
   ActionControllerRenderCall() { this.getMethodName() = "render" }

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

@@ -50,7 +50,9 @@ module UrlRedirect {
   /**
    * A source of remote user input, considered as a flow source.
    */
-  class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+  class HttpRequestInputAccessAsSource extends Source, Http::Server::RequestInputAccess {
+    HttpRequestInputAccessAsSource() { this.isThirdPartyControllable() }
+  }
 
   /**
    * A HTTP redirect response, considered as a flow sink.

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

@@ -312,9 +312,11 @@ module ReflectedXss {
   deprecated predicate isAdditionalXSSTaintStep = isAdditionalXssTaintStep/2;
 
   /**
-   * A source of remote user input, considered as a flow source.
+   * A HTTP request input, considered as a flow source.
    */
-  class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+  class HttpRequestInputAccessAsSource extends Source, Http::Server::RequestInputAccess {
+    HttpRequestInputAccessAsSource() { this.isThirdPartyControllable() }
+  }
 }
 
 /** DEPRECATED: Alias for ReflectedXss */