diff --git a/ruby/ql/lib/codeql/ruby/Concepts.qll b/ruby/ql/lib/codeql/ruby/Concepts.qll index 549ca596d65..8630b2815f7 100644 --- a/ruby/ql/lib/codeql/ruby/Concepts.qll +++ b/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 name of the header that is written to. */ + string getName() { result = super.getName() } + + /** 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. * diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll index 68489b9ccb5..707f00d0425 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll +++ b/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. = 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) } + } +} diff --git a/ruby/ql/lib/codeql/ruby/security/XSS.qll b/ruby/ql/lib/codeql/ruby/security/XSS.qll index 6bab2544546..df68f9464fd 100644 --- a/ruby/ql/lib/codeql/ruby/security/XSS.qll +++ b/ruby/ql/lib/codeql/ruby/security/XSS.qll @@ -105,6 +105,11 @@ private module Shared { } } + /** A write to an HTTP response header, considered as a flow sink. */ + class HeaderWriteAsSink extends Sink { + HeaderWriteAsSink() { this = any(Http::Server::HeaderWriteAccess a).getValue() } + } + /** * An HTML escaping, considered as a sanitizer. */ diff --git a/ruby/ql/src/change-notes/2022-10-14-actiondispatch-response.md b/ruby/ql/src/change-notes/2022-10-14-actiondispatch-response.md new file mode 100644 index 00000000000..850b853eabe --- /dev/null +++ b/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. diff --git a/ruby/ql/test/library-tests/frameworks/ActionController.expected b/ruby/ql/test/library-tests/frameworks/ActionController.expected index b1351bf76c8..6bf458d4210 100644 --- a/ruby/ql/test/library-tests/frameworks/ActionController.expected +++ b/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 | ... = ... | diff --git a/ruby/ql/test/library-tests/frameworks/ActionController.ql b/ruby/ql/test/library-tests/frameworks/ActionController.ql index d55503c3493..dca683dc6ab 100644 --- a/ruby/ql/test/library-tests/frameworks/ActionController.ql +++ b/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() +} diff --git a/ruby/ql/test/library-tests/frameworks/ActionDispatch.expected b/ruby/ql/test/library-tests/frameworks/ActionDispatch.expected index ff28522251b..b3499beda45 100644 --- a/ruby/ql/test/library-tests/frameworks/ActionDispatch.expected +++ b/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 | diff --git a/ruby/ql/test/library-tests/frameworks/ActionView.expected b/ruby/ql/test/library-tests/frameworks/ActionView.expected index 2f525d2be25..4e102a3c429 100644 --- a/ruby/ql/test/library-tests/frameworks/ActionView.expected +++ b/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 | diff --git a/ruby/ql/test/library-tests/frameworks/app/controllers/comments_controller.rb b/ruby/ql/test/library-tests/frameworks/app/controllers/comments_controller.rb index c7289e472ee..57fac7797bd 100644 --- a/ruby/ql/test/library-tests/frameworks/app/controllers/comments_controller.rb +++ b/ruby/ql/test/library-tests/frameworks/app/controllers/comments_controller.rb @@ -7,6 +7,32 @@ class CommentsController < ApplicationController 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 diff --git a/ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected b/ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected index f1e965bf260..d9d84a6699b 100644 --- a/ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected +++ b/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:25:53:25: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:25:53:25:54 | dt : | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text | +| app/controllers/foo/bars_controller.rb:25:53:25:54 | dt : | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] | +| app/controllers/foo/bars_controller.rb:25:53:25:54 | dt : | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] | +| app/controllers/foo/bars_controller.rb:25:53:25:54 | dt : | app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text | +| app/controllers/foo/bars_controller.rb:25:53:25: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:25:53:25: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 | diff --git a/ruby/ql/test/query-tests/security/cwe-079/app/controllers/foo/bars_controller.rb b/ruby/ql/test/query-tests/security/cwe-079/app/controllers/foo/bars_controller.rb index a05bf3a2314..c9c4f9c88b0 100644 --- a/ruby/ql/test/query-tests/security/cwe-079/app/controllers/foo/bars_controller.rb +++ b/ruby/ql/test/query-tests/security/cwe-079/app/controllers/foo/bars_controller.rb @@ -21,6 +21,7 @@ class BarsController < ApplicationController @safe_foo = "safe_foo" @html_escaped = ERB::Util.html_escape(params[:text]) @header_escaped = ERB::Util.html_escape(cookies[:foo]) # OK - cookies not controllable by 3rd party + response.header["content-type"] = params[:content_type] render "foo/bars/show", locals: { display_text: dt, safe_text: "hello" } end end