Merge pull request github#13289 from alexrford/rb/rack-redirect

Ruby: rack - model redirect responses

Author: alexrford

ruby/ql/lib/change-notes/2023-06-08-rack-redirect.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* HTTP redirect responses from Rack applications are now recognized as a potential sink for open redirect alerts.

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll

@@ -1284,13 +1284,16 @@ class HashLiteralNode extends LocalSourceNode, ExprNode {
  * into calls to `Array.[]`, so this includes both desugared calls as well as
  * explicit calls.
  */
-class ArrayLiteralNode extends LocalSourceNode, ExprNode {
+class ArrayLiteralNode extends LocalSourceNode, CallNode {
   ArrayLiteralNode() { super.getExprNode() instanceof CfgNodes::ExprNodes::ArrayLiteralCfgNode }
 
   /**
    * Gets an element of the array.
    */
-  Node getAnElement() { result = this.(CallNode).getPositionalArgument(_) }
+  Node getAnElement() { result = this.getElement(_) }
+
+  /** Gets the `i`th element of the array. */
+  Node getElement(int i) { result = this.getPositionalArgument(i) }
 }
 
 /**

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

@@ -2,47 +2,13 @@
  * Provides modeling for the Rack library.
  */
 
-private import codeql.ruby.controlflow.CfgNodes::ExprNodes
-private import codeql.ruby.DataFlow
-private import codeql.ruby.typetracking.TypeTracker
-
 /**
  * Provides modeling for the Rack library.
  */
 module Rack {
-  /**
-   * A class that may be a rack application.
-   * This is a class that has a `call` method that takes a single argument
-   * (traditionally called `env`) and returns a rack-compatible response.
-   */
-  class AppCandidate extends DataFlow::ClassNode {
-    private DataFlow::MethodNode call;
-
-    AppCandidate() {
-      call = this.getInstanceMethod("call") and
-      call.getNumberOfParameters() = 1 and
-      call.getAReturnNode() = trackRackResponse()
-    }
-
-    /**
-     * Gets the environment of the request, which is the lone parameter to the `call` method.
-     */
-    DataFlow::ParameterNode getEnv() { result = call.getParameter(0) }
-  }
-
-  private predicate isRackResponse(DataFlow::Node r) {
-    // [status, headers, body]
-    r.asExpr().(ArrayLiteralCfgNode).getNumberOfArguments() = 3
-  }
-
-  private DataFlow::LocalSourceNode trackRackResponse(TypeTracker t) {
-    t.start() and
-    isRackResponse(result)
-    or
-    exists(TypeTracker t2 | result = trackRackResponse(t2).track(t2, t))
-  }
+  import rack.internal.App
+  import rack.internal.Response::Public as Response
 
-  private DataFlow::Node trackRackResponse() {
-    trackRackResponse(TypeTracker::end()).flowsTo(result)
-  }
+  /** DEPRECATED: Alias for App::AppCandidate */
+  deprecated class AppCandidate = App::AppCandidate;
 }

ruby/ql/lib/codeql/ruby/frameworks/actiondispatch/internal/Request.qll

@@ -128,7 +128,7 @@ module Request {
     private import codeql.ruby.frameworks.Rack
 
     private class RackEnv extends Env {
-      RackEnv() { this = any(Rack::AppCandidate app).getEnv().getALocalUse() }
+      RackEnv() { this = any(Rack::App::AppCandidate app).getEnv().getALocalUse() }
     }
 
     /**

ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll

@@ -0,0 +1,53 @@
+/**
+ * Provides modeling for Rack applications.
+ */
+
+private import codeql.ruby.ApiGraphs
+private import codeql.ruby.DataFlow
+private import codeql.ruby.typetracking.TypeTracker
+private import Response::Private as RP
+
+/** A method node for a method named `call`. */
+private class CallMethodNode extends DataFlow::MethodNode {
+  CallMethodNode() { this.getMethodName() = "call" }
+}
+
+private DataFlow::LocalSourceNode trackRackResponse(TypeBackTracker t, CallMethodNode call) {
+  t.start() and
+  result = call.getAReturnNode().getALocalSource()
+  or
+  exists(TypeBackTracker t2 | result = trackRackResponse(t2, call).backtrack(t2, t))
+}
+
+private RP::PotentialResponseNode trackRackResponse(CallMethodNode call) {
+  result = trackRackResponse(TypeBackTracker::end(), call)
+}
+
+/**
+ * Provides modeling for Rack applications.
+ */
+module App {
+  /**
+   * A class that may be a rack application.
+   * This is a class that has a `call` method that takes a single argument
+   * (traditionally called `env`) and returns a rack-compatible response.
+   */
+  class AppCandidate extends DataFlow::ClassNode {
+    private CallMethodNode call;
+    private RP::PotentialResponseNode resp;
+
+    AppCandidate() {
+      call = this.getInstanceMethod("call") and
+      call.getNumberOfParameters() = 1 and
+      resp = trackRackResponse(call)
+    }
+
+    /**
+     * Gets the environment of the request, which is the lone parameter to the `call` method.
+     */
+    DataFlow::ParameterNode getEnv() { result = call.getParameter(0) }
+
+    /** Gets the response returned from a request to this application. */
+    RP::PotentialResponseNode getResponse() { result = resp }
+  }
+}

ruby/ql/lib/codeql/ruby/frameworks/rack/internal/Response.qll

@@ -0,0 +1,82 @@
+/**
+ * Provides modeling for the `Response` component of the `Rack` library.
+ */
+
+private import codeql.ruby.AST
+private import codeql.ruby.ApiGraphs
+private import codeql.ruby.Concepts
+private import codeql.ruby.controlflow.CfgNodes::ExprNodes
+private import codeql.ruby.DataFlow
+private import codeql.ruby.typetracking.TypeTracker
+private import App as A
+
+/** Contains implementation details for modeling `Rack::Response`. */
+module Private {
+  /** A `DataFlow::Node` that may be a rack response. This is detected heuristically, if something "looks like" a rack response syntactically then we consider it to be a potential response node. */
+  class PotentialResponseNode extends DataFlow::ArrayLiteralNode {
+    // [status, headers, body]
+    PotentialResponseNode() { this.getNumberOfArguments() = 3 }
+
+    /** Gets the headers returned with this response. */
+    DataFlow::Node getHeaders() { result = this.getElement(1) }
+
+    /** Gets the body of this response. */
+    DataFlow::Node getBody() { result = this.getElement(2) }
+  }
+}
+
+/**
+ * Provides modeling for the `Response` component of the `Rack` library.
+ */
+module Public {
+  bindingset[headerName]
+  private DataFlow::Node getHeaderValue(ResponseNode resp, string headerName) {
+    exists(DataFlow::Node headers | headers = resp.getHeaders() |
+      // set via `headers.<header_name>=`
+      exists(
+        DataFlow::CallNode contentTypeAssignment, Assignment assignment,
+        DataFlow::PostUpdateNode postUpdateHeaders
+      |
+        contentTypeAssignment.getMethodName() = headerName.replaceAll("-", "_").toLowerCase() + "=" and
+        assignment =
+          contentTypeAssignment.getArgument(0).(DataFlow::OperationNode).asOperationAstNode() and
+        postUpdateHeaders.(DataFlow::LocalSourceNode).flowsTo(headers) and
+        postUpdateHeaders.getPreUpdateNode() = contentTypeAssignment.getReceiver()
+      |
+        result.asExpr().getExpr() = assignment.getRightOperand()
+      )
+      or
+      // set within a hash
+      exists(DataFlow::HashLiteralNode headersHash | headersHash.flowsTo(headers) |
+        result =
+          headersHash
+              .getElementFromKey(any(ConstantValue v |
+                  v.getStringlikeValue().toLowerCase() = headerName.toLowerCase()
+                ))
+      )
+    )
+  }
+
+  /** A `DataFlow::Node` returned from a rack request. */
+  class ResponseNode extends Private::PotentialResponseNode, Http::Server::HttpResponse::Range {
+    ResponseNode() { this = any(A::App::AppCandidate app).getResponse() }
+
+    override DataFlow::Node getBody() { result = this.getElement(2) }
+
+    override DataFlow::Node getMimetypeOrContentTypeArg() {
+      result = getHeaderValue(this, "content-type")
+    }
+
+    // TODO: is there a sensible value for this?
+    override string getMimetypeDefault() { none() }
+  }
+
+  /** A `DataFlow::Node` returned from a rack request that has a redirect HTTP status code. */
+  class RedirectResponse extends ResponseNode, Http::Server::HttpRedirectResponse::Range {
+    private DataFlow::Node redirectLocation;
+
+    RedirectResponse() { redirectLocation = getHeaderValue(this, "location") }
+
+    override DataFlow::Node getRedirectLocation() { result = redirectLocation }
+  }
+}

ruby/ql/test/library-tests/frameworks/rack/Rack.expected

@@ -1,4 +1,11 @@
-| rack.rb:1:1:5:3 | HelloWorld | rack.rb:2:12:2:14 | env |
-| rack.rb:7:1:16:3 | Proxy | rack.rb:12:12:12:18 | the_env |
-| rack.rb:18:1:31:3 | Logger | rack.rb:24:12:24:14 | env |
-| rack.rb:45:1:61:3 | Baz | rack.rb:46:12:46:14 | env |
+rackApps
+| rack.rb:1:1:10:3 | HelloWorld | rack.rb:2:12:2:14 | env |
+| rack.rb:12:1:22:3 | Proxy | rack.rb:17:12:17:18 | the_env |
+| rack.rb:24:1:37:3 | Logger | rack.rb:30:12:30:14 | env |
+| rack.rb:39:1:45:3 | Redirector | rack.rb:40:12:40:14 | env |
+| rack.rb:59:1:75:3 | Baz | rack.rb:60:12:60:14 | env |
+rackResponseContentTypes
+| rack.rb:8:5:8:38 | call to [] | rack.rb:7:34:7:45 | "text/plain" |
+| rack.rb:20:5:20:27 | call to [] | rack.rb:19:28:19:38 | "text/html" |
+redirectResponses
+| rack.rb:43:5:43:45 | call to [] | rack.rb:42:30:42:40 | "/foo.html" |

ruby/ql/test/library-tests/frameworks/rack/Rack.ql

@@ -1,4 +1,17 @@
+private import codeql.ruby.AST
 private import codeql.ruby.frameworks.Rack
 private import codeql.ruby.DataFlow
 
-query predicate rackApps(Rack::AppCandidate c, DataFlow::ParameterNode env) { env = c.getEnv() }
+query predicate rackApps(Rack::App::AppCandidate c, DataFlow::ParameterNode env) {
+  env = c.getEnv()
+}
+
+query predicate rackResponseContentTypes(
+  Rack::Response::ResponseNode resp, DataFlow::Node contentType
+) {
+  contentType = resp.getMimetypeOrContentTypeArg()
+}
+
+query predicate redirectResponses(Rack::Response::RedirectResponse resp, DataFlow::Node location) {
+  location = resp.getRedirectLocation()
+}

ruby/ql/test/library-tests/frameworks/rack/rack.rb

@@ -1,6 +1,11 @@
 class HelloWorld
   def call(env)
-    [200, {'Content-Type' => 'text/plain'}, ['Hello World']]
+    status = 200
+    if something_goes_wrong(env)
+      status = 500
+    end
+    headers = {'Content-Type' => 'text/plain'}
+    [status, headers, ['Hello World']]
   end
 end
 
@@ -11,6 +16,7 @@ def initialize(app)
 
   def call(the_env)
     status, headers, body = @app.call(the_env)
+    headers.content_type = "text/html"
     [status, headers, body]
   end
 end
@@ -30,6 +36,14 @@ def call(env)
   end
 end
 
+class Redirector
+  def call(env)
+    status = 302
+    headers = {'location' => '/foo.html'}
+    [status, headers, ['this is a redirect']]
+  end
+end
+
 class Foo
   def not_call(env)
     [1, 2, 3]