diff --git a/ruby/ql/lib/change-notes/2023-06-08-rack-redirect.md b/ruby/ql/lib/change-notes/2023-06-08-rack-redirect.md new file mode 100644 index 000000000000..09687fa95be2 --- /dev/null +++ b/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. diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll index 501cf70593e5..6fda1adb3321 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll +++ b/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) } } /** diff --git a/ruby/ql/lib/codeql/ruby/frameworks/Rack.qll b/ruby/ql/lib/codeql/ruby/frameworks/Rack.qll index 49281c609bd7..6963f37a81c7 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/Rack.qll +++ b/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; } diff --git a/ruby/ql/lib/codeql/ruby/frameworks/actiondispatch/internal/Request.qll b/ruby/ql/lib/codeql/ruby/frameworks/actiondispatch/internal/Request.qll index d749b87f2737..8c6a11b455b0 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/actiondispatch/internal/Request.qll +++ b/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() } } /** diff --git a/ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll b/ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll new file mode 100644 index 000000000000..bfdd988ac191 --- /dev/null +++ b/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 } + } +} diff --git a/ruby/ql/lib/codeql/ruby/frameworks/rack/internal/Response.qll b/ruby/ql/lib/codeql/ruby/frameworks/rack/internal/Response.qll new file mode 100644 index 000000000000..9d998c780aef --- /dev/null +++ b/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.=` + 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 } + } +} diff --git a/ruby/ql/test/library-tests/frameworks/rack/Rack.expected b/ruby/ql/test/library-tests/frameworks/rack/Rack.expected index 5613aabd7a48..c55afeb78010 100644 --- a/ruby/ql/test/library-tests/frameworks/rack/Rack.expected +++ b/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" | diff --git a/ruby/ql/test/library-tests/frameworks/rack/Rack.ql b/ruby/ql/test/library-tests/frameworks/rack/Rack.ql index 560b81c3839b..9b5d0629a9f0 100644 --- a/ruby/ql/test/library-tests/frameworks/rack/Rack.ql +++ b/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() +} diff --git a/ruby/ql/test/library-tests/frameworks/rack/rack.rb b/ruby/ql/test/library-tests/frameworks/rack/rack.rb index 03955455787a..9f743496ad26 100644 --- a/ruby/ql/test/library-tests/frameworks/rack/rack.rb +++ b/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]