Skip to content

Commit 24e8316

Browse files
alexrfordalexrford
authored
Merge pull request #13289 from alexrford/rb/rack-redirect
Ruby: rack - model redirect responses
2 parents 7efbd88 + 8ef8a0d commit 24e8316

File tree

9 files changed

+189
-47
lines changed

9 files changed

+189
-47
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* 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‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1284,13 +1284,16 @@ class HashLiteralNode extends LocalSourceNode, ExprNode {
12841284
* into calls to `Array.[]`, so this includes both desugared calls as well as
12851285
* explicit calls.
12861286
*/
1287-
class ArrayLiteralNode extends LocalSourceNode, ExprNode {
1287+
class ArrayLiteralNode extends LocalSourceNode, CallNode {
12881288
ArrayLiteralNode() { super.getExprNode() instanceof CfgNodes::ExprNodes::ArrayLiteralCfgNode }
12891289

12901290
/**
12911291
* Gets an element of the array.
12921292
*/
1293-
Node getAnElement() { result = this.(CallNode).getPositionalArgument(_) }
1293+
Node getAnElement() { result = this.getElement(_) }
1294+
1295+
/** Gets the `i`th element of the array. */
1296+
Node getElement(int i) { result = this.getPositionalArgument(i) }
12941297
}
12951298

12961299
/**

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

Lines changed: 4 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -2,47 +2,13 @@
22
* Provides modeling for the Rack library.
33
*/
44

5-
private import codeql.ruby.controlflow.CfgNodes::ExprNodes
6-
private import codeql.ruby.DataFlow
7-
private import codeql.ruby.typetracking.TypeTracker
8-
95
/**
106
* Provides modeling for the Rack library.
117
*/
128
module Rack {
13-
/**
14-
* A class that may be a rack application.
15-
* This is a class that has a `call` method that takes a single argument
16-
* (traditionally called `env`) and returns a rack-compatible response.
17-
*/
18-
class AppCandidate extends DataFlow::ClassNode {
19-
private DataFlow::MethodNode call;
20-
21-
AppCandidate() {
22-
call = this.getInstanceMethod("call") and
23-
call.getNumberOfParameters() = 1 and
24-
call.getAReturnNode() = trackRackResponse()
25-
}
26-
27-
/**
28-
* Gets the environment of the request, which is the lone parameter to the `call` method.
29-
*/
30-
DataFlow::ParameterNode getEnv() { result = call.getParameter(0) }
31-
}
32-
33-
private predicate isRackResponse(DataFlow::Node r) {
34-
// [status, headers, body]
35-
r.asExpr().(ArrayLiteralCfgNode).getNumberOfArguments() = 3
36-
}
37-
38-
private DataFlow::LocalSourceNode trackRackResponse(TypeTracker t) {
39-
t.start() and
40-
isRackResponse(result)
41-
or
42-
exists(TypeTracker t2 | result = trackRackResponse(t2).track(t2, t))
43-
}
9+
import rack.internal.App
10+
import rack.internal.Response::Public as Response
4411

45-
private DataFlow::Node trackRackResponse() {
46-
trackRackResponse(TypeTracker::end()).flowsTo(result)
47-
}
12+
/** DEPRECATED: Alias for App::AppCandidate */
13+
deprecated class AppCandidate = App::AppCandidate;
4814
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ module Request {
128128
private import codeql.ruby.frameworks.Rack
129129

130130
private class RackEnv extends Env {
131-
RackEnv() { this = any(Rack::AppCandidate app).getEnv().getALocalUse() }
131+
RackEnv() { this = any(Rack::App::AppCandidate app).getEnv().getALocalUse() }
132132
}
133133

134134
/**

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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
/**
2+
* Provides modeling for Rack applications.
3+
*/
4+
5+
private import codeql.ruby.ApiGraphs
6+
private import codeql.ruby.DataFlow
7+
private import codeql.ruby.typetracking.TypeTracker
8+
private import Response::Private as RP
9+
10+
/** A method node for a method named `call`. */
11+
private class CallMethodNode extends DataFlow::MethodNode {
12+
CallMethodNode() { this.getMethodName() = "call" }
13+
}
14+
15+
private DataFlow::LocalSourceNode trackRackResponse(TypeBackTracker t, CallMethodNode call) {
16+
t.start() and
17+
result = call.getAReturnNode().getALocalSource()
18+
or
19+
exists(TypeBackTracker t2 | result = trackRackResponse(t2, call).backtrack(t2, t))
20+
}
21+
22+
private RP::PotentialResponseNode trackRackResponse(CallMethodNode call) {
23+
result = trackRackResponse(TypeBackTracker::end(), call)
24+
}
25+
26+
/**
27+
* Provides modeling for Rack applications.
28+
*/
29+
module App {
30+
/**
31+
* A class that may be a rack application.
32+
* This is a class that has a `call` method that takes a single argument
33+
* (traditionally called `env`) and returns a rack-compatible response.
34+
*/
35+
class AppCandidate extends DataFlow::ClassNode {
36+
private CallMethodNode call;
37+
private RP::PotentialResponseNode resp;
38+
39+
AppCandidate() {
40+
call = this.getInstanceMethod("call") and
41+
call.getNumberOfParameters() = 1 and
42+
resp = trackRackResponse(call)
43+
}
44+
45+
/**
46+
* Gets the environment of the request, which is the lone parameter to the `call` method.
47+
*/
48+
DataFlow::ParameterNode getEnv() { result = call.getParameter(0) }
49+
50+
/** Gets the response returned from a request to this application. */
51+
RP::PotentialResponseNode getResponse() { result = resp }
52+
}
53+
}

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

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
/**
2+
* Provides modeling for the `Response` component of the `Rack` library.
3+
*/
4+
5+
private import codeql.ruby.AST
6+
private import codeql.ruby.ApiGraphs
7+
private import codeql.ruby.Concepts
8+
private import codeql.ruby.controlflow.CfgNodes::ExprNodes
9+
private import codeql.ruby.DataFlow
10+
private import codeql.ruby.typetracking.TypeTracker
11+
private import App as A
12+
13+
/** Contains implementation details for modeling `Rack::Response`. */
14+
module Private {
15+
/** 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. */
16+
class PotentialResponseNode extends DataFlow::ArrayLiteralNode {
17+
// [status, headers, body]
18+
PotentialResponseNode() { this.getNumberOfArguments() = 3 }
19+
20+
/** Gets the headers returned with this response. */
21+
DataFlow::Node getHeaders() { result = this.getElement(1) }
22+
23+
/** Gets the body of this response. */
24+
DataFlow::Node getBody() { result = this.getElement(2) }
25+
}
26+
}
27+
28+
/**
29+
* Provides modeling for the `Response` component of the `Rack` library.
30+
*/
31+
module Public {
32+
bindingset[headerName]
33+
private DataFlow::Node getHeaderValue(ResponseNode resp, string headerName) {
34+
exists(DataFlow::Node headers | headers = resp.getHeaders() |
35+
// set via `headers.<header_name>=`
36+
exists(
37+
DataFlow::CallNode contentTypeAssignment, Assignment assignment,
38+
DataFlow::PostUpdateNode postUpdateHeaders
39+
|
40+
contentTypeAssignment.getMethodName() = headerName.replaceAll("-", "_").toLowerCase() + "=" and
41+
assignment =
42+
contentTypeAssignment.getArgument(0).(DataFlow::OperationNode).asOperationAstNode() and
43+
postUpdateHeaders.(DataFlow::LocalSourceNode).flowsTo(headers) and
44+
postUpdateHeaders.getPreUpdateNode() = contentTypeAssignment.getReceiver()
45+
|
46+
result.asExpr().getExpr() = assignment.getRightOperand()
47+
)
48+
or
49+
// set within a hash
50+
exists(DataFlow::HashLiteralNode headersHash | headersHash.flowsTo(headers) |
51+
result =
52+
headersHash
53+
.getElementFromKey(any(ConstantValue v |
54+
v.getStringlikeValue().toLowerCase() = headerName.toLowerCase()
55+
))
56+
)
57+
)
58+
}
59+
60+
/** A `DataFlow::Node` returned from a rack request. */
61+
class ResponseNode extends Private::PotentialResponseNode, Http::Server::HttpResponse::Range {
62+
ResponseNode() { this = any(A::App::AppCandidate app).getResponse() }
63+
64+
override DataFlow::Node getBody() { result = this.getElement(2) }
65+
66+
override DataFlow::Node getMimetypeOrContentTypeArg() {
67+
result = getHeaderValue(this, "content-type")
68+
}
69+
70+
// TODO: is there a sensible value for this?
71+
override string getMimetypeDefault() { none() }
72+
}
73+
74+
/** A `DataFlow::Node` returned from a rack request that has a redirect HTTP status code. */
75+
class RedirectResponse extends ResponseNode, Http::Server::HttpRedirectResponse::Range {
76+
private DataFlow::Node redirectLocation;
77+
78+
RedirectResponse() { redirectLocation = getHeaderValue(this, "location") }
79+
80+
override DataFlow::Node getRedirectLocation() { result = redirectLocation }
81+
}
82+
}

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

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,11 @@
1-
| rack.rb:1:1:5:3 | HelloWorld | rack.rb:2:12:2:14 | env |
2-
| rack.rb:7:1:16:3 | Proxy | rack.rb:12:12:12:18 | the_env |
3-
| rack.rb:18:1:31:3 | Logger | rack.rb:24:12:24:14 | env |
4-
| rack.rb:45:1:61:3 | Baz | rack.rb:46:12:46:14 | env |
1+
rackApps
2+
| rack.rb:1:1:10:3 | HelloWorld | rack.rb:2:12:2:14 | env |
3+
| rack.rb:12:1:22:3 | Proxy | rack.rb:17:12:17:18 | the_env |
4+
| rack.rb:24:1:37:3 | Logger | rack.rb:30:12:30:14 | env |
5+
| rack.rb:39:1:45:3 | Redirector | rack.rb:40:12:40:14 | env |
6+
| rack.rb:59:1:75:3 | Baz | rack.rb:60:12:60:14 | env |
7+
rackResponseContentTypes
8+
| rack.rb:8:5:8:38 | call to [] | rack.rb:7:34:7:45 | "text/plain" |
9+
| rack.rb:20:5:20:27 | call to [] | rack.rb:19:28:19:38 | "text/html" |
10+
redirectResponses
11+
| 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‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,17 @@
1+
private import codeql.ruby.AST
12
private import codeql.ruby.frameworks.Rack
23
private import codeql.ruby.DataFlow
34

4-
query predicate rackApps(Rack::AppCandidate c, DataFlow::ParameterNode env) { env = c.getEnv() }
5+
query predicate rackApps(Rack::App::AppCandidate c, DataFlow::ParameterNode env) {
6+
env = c.getEnv()
7+
}
8+
9+
query predicate rackResponseContentTypes(
10+
Rack::Response::ResponseNode resp, DataFlow::Node contentType
11+
) {
12+
contentType = resp.getMimetypeOrContentTypeArg()
13+
}
14+
15+
query predicate redirectResponses(Rack::Response::RedirectResponse resp, DataFlow::Node location) {
16+
location = resp.getRedirectLocation()
17+
}

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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
class HelloWorld
22
def call(env)
3-
[200, {'Content-Type' => 'text/plain'}, ['Hello World']]
3+
status = 200
4+
if something_goes_wrong(env)
5+
status = 500
6+
end
7+
headers = {'Content-Type' => 'text/plain'}
8+
[status, headers, ['Hello World']]
49
end
510
end
611

@@ -11,6 +16,7 @@ def initialize(app)
1116

1217
def call(the_env)
1318
status, headers, body = @app.call(the_env)
19+
headers.content_type = "text/html"
1420
[status, headers, body]
1521
end
1622
end
@@ -30,6 +36,14 @@ def call(env)
3036
end
3137
end
3238

39+
class Redirector
40+
def call(env)
41+
status = 302
42+
headers = {'location' => '/foo.html'}
43+
[status, headers, ['this is a redirect']]
44+
end
45+
end
46+
3347
class Foo
3448
def not_call(env)
3549
[1, 2, 3]

0 commit comments

Comments
 (0)