Merge pull request github#10803 from hmac/actiondispatch-response

Ruby: Model ActionDispatch::Response

Author: aibaars

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

@@ -467,6 +467,37 @@ module Http {
       override RequestInputKind getKind() { result = parameterInputKind() }
     }
 
+    /**
+     * A data flow node that writes data to a header in an HTTP response.
+     *
+     * Extend this class to refine existing API models. If you want to model new APIs,
+     * extend `HeaderWriteAccess::Range` instead.
+     */
+    class HeaderWriteAccess extends DataFlow::Node instanceof HeaderWriteAccess::Range {
+      /** Gets the (lower case) name of the header that is written to. */
+      string getName() { result = super.getName().toLowerCase() }
+
+      /** Gets the value that is written to the header. */
+      DataFlow::Node getValue() { result = super.getValue() }
+    }
+
+    /** Provides a class for modeling new HTTP header writes. */
+    module HeaderWriteAccess {
+      /**
+       * A data flow node that writes data to the header in an HTTP response.
+       *
+       * Extend this class to model new APIs. If you want to refine existing API models,
+       * extend `HeaderWriteAccess` instead.
+       */
+      abstract class Range extends DataFlow::Node {
+        /** Gets the name of the header that is written to. */
+        abstract string getName();
+
+        /** Gets the value that is written to the header. */
+        abstract DataFlow::Node getValue();
+      }
+    }
+
     /**
      * A data-flow node that creates a HTTP response on a server.
      *

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

@@ -516,8 +516,11 @@ private class ActionControllerProtectFromForgeryCall extends CsrfProtectionSetti
  */
 private class SendFile extends FileSystemAccess::Range, DataFlow::CallNode {
   SendFile() {
-    this.asExpr().getExpr() instanceof ActionControllerContextCall and
-    this.getMethodName() = "send_file"
+    this.getMethodName() = "send_file" and
+    (
+      this.asExpr().getExpr() instanceof ActionControllerContextCall or
+      this.getReceiver().asExpr().getExpr() instanceof Response::ResponseCall
+    )
   }
 
   override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
@@ -642,3 +645,94 @@ private module ParamsSummaries {
     }
   }
 }
+
+/**
+ * Provides modeling for `ActionDispatch::Response`, which represents an HTTP
+ * response.
+ */
+private module Response {
+  class ResponseCall extends ActionControllerContextCall {
+    ResponseCall() { this.getMethodName() = "response" }
+  }
+
+  class BodyWrite extends DataFlow::CallNode, Http::Server::HttpResponse::Range {
+    BodyWrite() {
+      this.getReceiver().asExpr().getExpr() instanceof ResponseCall and
+      this.getMethodName() = "body="
+    }
+
+    override DataFlow::Node getBody() { result = this.getArgument(0) }
+
+    override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
+
+    override string getMimetypeDefault() { result = "text/http" }
+  }
+
+  class SendFileCall extends DataFlow::CallNode, Http::Server::HttpResponse::Range {
+    SendFileCall() {
+      this.getReceiver().asExpr().getExpr() instanceof ResponseCall and
+      this.getMethodName() = "send_file"
+    }
+
+    override DataFlow::Node getBody() { result = this.getArgument(0) }
+
+    override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
+
+    override string getMimetypeDefault() { result = "application/octet-stream" }
+  }
+
+  class HeaderWrite extends DataFlow::CallNode, Http::Server::HeaderWriteAccess::Range {
+    HeaderWrite() {
+      // response.header[key] = val
+      // response.headers[key] = val
+      exists(MethodCall headerCall |
+        headerCall.getMethodName() = ["header", "headers"] and
+        headerCall.getReceiver() instanceof ResponseCall
+      |
+        this.getReceiver().asExpr().getExpr() = headerCall and
+        this.getMethodName() = "[]="
+      )
+      or
+      // response.set_header(key) = val
+      // response[header] = val
+      // response.add_header(key, val)
+      this.getReceiver().asExpr().getExpr() instanceof ResponseCall and
+      this.getMethodName() = ["set_header", "[]=", "add_header"]
+    }
+
+    override string getName() {
+      result = this.getArgument(0).asExpr().getConstantValue().getString()
+    }
+
+    override DataFlow::Node getValue() { result = this.getArgument(1) }
+  }
+
+  class SpecificHeaderWrite extends DataFlow::CallNode, Http::Server::HeaderWriteAccess::Range {
+    SpecificHeaderWrite() {
+      // response.<method> = val
+      this.getReceiver().asExpr().getExpr() instanceof ResponseCall and
+      this.getMethodName() =
+        [
+          "location=", "cache_control=", "_cache_control=", "etag=", "charset=", "content_type=",
+          "date=", "last_modified=", "weak_etag=", "strong_etag="
+        ]
+    }
+
+    override string getName() {
+      this.getMethodName() = "location=" and result = "location"
+      or
+      this.getMethodName() = ["_cache_control=", "cache_control="] and result = "cache-control"
+      or
+      this.getMethodName() = ["etag=", "weak_etag=", "strong_etag="] and result = "etag"
+      or
+      // sets the charset part of the content-type header
+      this.getMethodName() = ["charset=", "content_type="] and result = "content-type"
+      or
+      this.getMethodName() = "date=" and result = "date"
+      or
+      this.getMethodName() = "last_modified=" and result = "last-modified"
+    }
+
+    override DataFlow::Node getValue() { result = this.getArgument(0) }
+  }
+}

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

@@ -105,6 +105,17 @@ private module Shared {
     }
   }
 
+  /** A write to an HTTP response header, considered as a flow sink. */
+  class HeaderWriteAsSink extends Sink {
+    HeaderWriteAsSink() {
+      exists(Http::Server::HeaderWriteAccess a |
+        a.getName() = ["content-type", "access-control-allow-origin"]
+      |
+        this = a.getValue()
+      )
+    }
+  }
+
   /**
    * An HTML escaping, considered as a sanitizer.
    */

ruby/ql/src/change-notes/2022-10-14-actiondispatch-response.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* HTTP response header and body writes via `ActionDispatch::Response` are now
+  recognized.

ruby/ql/test/library-tests/frameworks/ActionController.expected

@@ -6,7 +6,7 @@ actionControllerControllerClasses
 | active_record/ActiveRecord.rb:66:1:98:3 | BazController |
 | active_record/ActiveRecord.rb:100:1:108:3 | AnnotatedController |
 | active_storage/active_storage.rb:39:1:45:3 | PostsController |
-| app/controllers/comments_controller.rb:1:1:14:3 | CommentsController |
+| app/controllers/comments_controller.rb:1:1:40:3 | CommentsController |
 | app/controllers/foo/bars_controller.rb:3:1:46:3 | BarsController |
 | app/controllers/photos_controller.rb:1:1:4:3 | PhotosController |
 | app/controllers/posts_controller.rb:1:1:10:3 | PostsController |
@@ -61,8 +61,8 @@ actionControllerActionMethods
 | active_record/ActiveRecord.rb:101:3:103:5 | index |
 | active_record/ActiveRecord.rb:105:3:107:5 | unsafe_action |
 | active_storage/active_storage.rb:40:3:44:5 | create |
-| app/controllers/comments_controller.rb:2:3:10:5 | index |
-| app/controllers/comments_controller.rb:12:3:13:5 | show |
+| app/controllers/comments_controller.rb:2:3:36:5 | index |
+| app/controllers/comments_controller.rb:38:3:39:5 | show |
 | app/controllers/foo/bars_controller.rb:5:3:7:5 | index |
 | app/controllers/foo/bars_controller.rb:9:3:18:5 | show_debug |
 | app/controllers/foo/bars_controller.rb:20:3:24:5 | show |
@@ -370,3 +370,19 @@ getAssociatedControllerClasses
 controllerTemplateFiles
 | app/controllers/foo/bars_controller.rb:3:1:46:3 | BarsController | app/views/foo/bars/_widget.html.erb:0:0:0:0 | app/views/foo/bars/_widget.html.erb |
 | app/controllers/foo/bars_controller.rb:3:1:46:3 | BarsController | app/views/foo/bars/show.html.erb:0:0:0:0 | app/views/foo/bars/show.html.erb |
+headerWriteAccesses
+| app/controllers/comments_controller.rb:15:5:15:35 | call to []= | content-type | app/controllers/comments_controller.rb:15:39:15:49 | ... = ... |
+| app/controllers/comments_controller.rb:16:5:16:46 | call to set_header | content-length | app/controllers/comments_controller.rb:16:43:16:45 | 100 |
+| app/controllers/comments_controller.rb:17:5:17:39 | call to []= | x-custom-header | app/controllers/comments_controller.rb:17:43:17:46 | ... = ... |
+| app/controllers/comments_controller.rb:18:5:18:39 | call to []= | x-another-custom-header | app/controllers/comments_controller.rb:18:43:18:47 | ... = ... |
+| app/controllers/comments_controller.rb:19:5:19:49 | call to add_header | x-yet-another | app/controllers/comments_controller.rb:19:42:19:49 | "indeed" |
+| app/controllers/comments_controller.rb:25:5:25:21 | call to location= | location | app/controllers/comments_controller.rb:25:25:25:36 | ... = ... |
+| app/controllers/comments_controller.rb:26:5:26:26 | call to cache_control= | cache-control | app/controllers/comments_controller.rb:26:30:26:36 | ... = ... |
+| app/controllers/comments_controller.rb:27:5:27:27 | call to _cache_control= | cache-control | app/controllers/comments_controller.rb:27:31:27:37 | ... = ... |
+| app/controllers/comments_controller.rb:28:5:28:17 | call to etag= | etag | app/controllers/comments_controller.rb:28:21:28:27 | ... = ... |
+| app/controllers/comments_controller.rb:29:5:29:20 | call to charset= | content-type | app/controllers/comments_controller.rb:29:24:29:30 | ... = ... |
+| app/controllers/comments_controller.rb:30:5:30:25 | call to content_type= | content-type | app/controllers/comments_controller.rb:30:29:30:35 | ... = ... |
+| app/controllers/comments_controller.rb:32:5:32:17 | call to date= | date | app/controllers/comments_controller.rb:32:21:32:30 | ... = ... |
+| app/controllers/comments_controller.rb:33:5:33:26 | call to last_modified= | last-modified | app/controllers/comments_controller.rb:33:30:33:43 | ... = ... |
+| app/controllers/comments_controller.rb:34:5:34:22 | call to weak_etag= | etag | app/controllers/comments_controller.rb:34:26:34:32 | ... = ... |
+| app/controllers/comments_controller.rb:35:5:35:24 | call to strong_etag= | etag | app/controllers/comments_controller.rb:35:28:35:34 | ... = ... |

ruby/ql/test/library-tests/frameworks/ActionController.ql

@@ -3,6 +3,7 @@ private import codeql.ruby.frameworks.ActionController
 private import codeql.ruby.frameworks.Rails
 private import codeql.ruby.frameworks.ActionView
 private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
 
 query predicate actionControllerControllerClasses(ActionControllerControllerClass cls) { any() }
 
@@ -31,3 +32,9 @@ query predicate getAssociatedControllerClasses(ActionControllerControllerClass c
 query predicate controllerTemplateFiles(ActionControllerControllerClass cls, ErbFile templateFile) {
   controllerTemplateFile(cls, templateFile)
 }
+
+query predicate headerWriteAccesses(
+  Http::Server::HeaderWriteAccess a, string name, DataFlow::Node value
+) {
+  name = a.getName() and value = a.getValue()
+}

ruby/ql/test/library-tests/frameworks/ActionDispatch.expected

@@ -36,8 +36,8 @@ actionDispatchRoutes
 actionDispatchControllerMethods
 | app/config/routes.rb:2:3:8:5 | call to resources | app/controllers/posts_controller.rb:2:3:3:5 | index |
 | app/config/routes.rb:2:3:8:5 | call to resources | app/controllers/posts_controller.rb:5:3:6:5 | show |
-| app/config/routes.rb:3:5:6:7 | call to resources | app/controllers/comments_controller.rb:2:3:10:5 | index |
-| app/config/routes.rb:3:5:6:7 | call to resources | app/controllers/comments_controller.rb:12:3:13:5 | show |
+| app/config/routes.rb:3:5:6:7 | call to resources | app/controllers/comments_controller.rb:2:3:36:5 | index |
+| app/config/routes.rb:3:5:6:7 | call to resources | app/controllers/comments_controller.rb:38:3:39:5 | show |
 | app/config/routes.rb:7:5:7:37 | call to post | app/controllers/posts_controller.rb:8:3:9:5 | upvote |
 | app/config/routes.rb:27:3:27:48 | call to match | app/controllers/photos_controller.rb:2:3:3:5 | show |
 | app/config/routes.rb:28:3:28:50 | call to match | app/controllers/photos_controller.rb:2:3:3:5 | show |

ruby/ql/test/library-tests/frameworks/ActionView.expected

@@ -24,6 +24,8 @@ renderToCalls
 linkToCalls
 | app/views/foo/bars/show.html.erb:33:5:33:41 | call to link_to |
 httpResponses
+| app/controllers/comments_controller.rb:11:5:11:17 | call to body= | app/controllers/comments_controller.rb:11:21:11:34 | ... = ... | text/http |
+| app/controllers/comments_controller.rb:21:5:21:37 | call to send_file | app/controllers/comments_controller.rb:21:24:21:36 | "my-file.ext" | application/octet-stream |
 | app/controllers/foo/bars_controller.rb:15:16:15:97 | call to render_to_string | app/controllers/foo/bars_controller.rb:15:33:15:47 | "foo/bars/show" | text/html |
 | app/controllers/foo/bars_controller.rb:23:5:23:76 | call to render | app/controllers/foo/bars_controller.rb:23:12:23:26 | "foo/bars/show" | text/html |
 | app/controllers/foo/bars_controller.rb:35:5:35:33 | call to render | app/controllers/foo/bars_controller.rb:35:18:35:33 | call to [] | application/json |

ruby/ql/test/library-tests/frameworks/app/controllers/comments_controller.rb

@@ -7,6 +7,32 @@ def index
     request.query_parameters
     request.request_parameters
     request.filtered_parameters
+
+    response.body = "some content"
+
+    response.status = 200
+
+    response.header["Content-Type"] = "text/html"
+    response.set_header("Content-Length", 100)
+    response.headers["X-Custom-Header"] = "hi"
+    response["X-Another-Custom-Header"] = "yes"
+    response.add_header "X-Yet-Another", "indeed"
+
+    response.send_file("my-file.ext")
+
+    response.request
+
+    response.location = "http://..." # relevant for url redirect query
+    response.cache_control = "value"
+    response._cache_control = "value"
+    response.etag = "value"
+    response.charset = "value" # sets the charset part of the content-type header
+    response.content_type = "value" # sets the main part of the content-type header
+
+    response.date = Date.today
+    response.last_modified = Date.yesterday
+    response.weak_etag = "value"
+    response.strong_etag = "value"
   end
 
   def show

ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected

@@ -10,13 +10,15 @@ edges
 | app/controllers/foo/bars_controller.rb:17:21:17:36 | ...[...] :  | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website |
 | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | app/controllers/foo/bars_controller.rb:18:10:18:22 | ...[...] :  |
 | app/controllers/foo/bars_controller.rb:18:10:18:22 | ...[...] :  | app/controllers/foo/bars_controller.rb:19:22:19:23 | dt :  |
-| app/controllers/foo/bars_controller.rb:18:10:18:22 | ...[...] :  | app/controllers/foo/bars_controller.rb:24:53:24:54 | dt :  |
+| app/controllers/foo/bars_controller.rb:18:10:18:22 | ...[...] :  | app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  |
 | app/controllers/foo/bars_controller.rb:19:22:19:23 | dt :  | app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text |
-| app/controllers/foo/bars_controller.rb:24:53:24:54 | dt :  | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text |
-| app/controllers/foo/bars_controller.rb:24:53:24:54 | dt :  | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] |
-| app/controllers/foo/bars_controller.rb:24:53:24:54 | dt :  | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] |
-| app/controllers/foo/bars_controller.rb:24:53:24:54 | dt :  | app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text |
-| app/controllers/foo/bars_controller.rb:24:53:24:54 | dt :  | app/views/foo/bars/show.html.erb:44:76:44:87 | call to display_text :  |
+| app/controllers/foo/bars_controller.rb:24:39:24:44 | call to params :  | app/controllers/foo/bars_controller.rb:24:39:24:59 | ...[...] :  |
+| app/controllers/foo/bars_controller.rb:24:39:24:59 | ...[...] :  | app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... |
+| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text |
+| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] |
+| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] |
+| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text |
+| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:44:76:44:87 | call to display_text :  |
 | app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... :  | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text |
 | app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... :  | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] |
 | app/views/foo/bars/show.html.erb:44:76:44:87 | call to display_text :  | app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... :  |
@@ -35,7 +37,10 @@ nodes
 | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | semmle.label | call to params :  |
 | app/controllers/foo/bars_controller.rb:18:10:18:22 | ...[...] :  | semmle.label | ...[...] :  |
 | app/controllers/foo/bars_controller.rb:19:22:19:23 | dt :  | semmle.label | dt :  |
-| app/controllers/foo/bars_controller.rb:24:53:24:54 | dt :  | semmle.label | dt :  |
+| app/controllers/foo/bars_controller.rb:24:39:24:44 | call to params :  | semmle.label | call to params :  |
+| app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... | semmle.label | ... = ... |
+| app/controllers/foo/bars_controller.rb:24:39:24:59 | ...[...] :  | semmle.label | ...[...] :  |
+| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | semmle.label | dt :  |
 | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | semmle.label | call to display_text |
 | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | semmle.label | ...[...] |
 | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | semmle.label | @user_website |
@@ -58,6 +63,7 @@ nodes
 | app/views/foo/bars/show.html.erb:77:28:77:39 | ...[...] | semmle.label | ...[...] |
 subpaths
 #select
+| app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... | app/controllers/foo/bars_controller.rb:24:39:24:44 | call to params :  | app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:24:39:24:44 | call to params | user-provided value |
 | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
 | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | app/controllers/foo/bars_controller.rb:17:21:17:26 | call to params :  | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:17:21:17:26 | call to params | user-provided value |