Skip to content

Commit 36d5fda

Browse files
hmachmac
authored
Merge pull request #260 from github/hmac-url-redirect
Add URLRedirect query
2 parents 799c0ff + 8725303 commit 36d5fda

File tree

10 files changed

+346
-0
lines changed

10 files changed

+346
-0
lines changed

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,25 @@ class RedirectToCall extends ActionControllerContextCall {
154154
}
155155
}
156156

157+
/**
158+
* A call to the `redirect_to` method, as an `HttpRedirectResponse`.
159+
*/
160+
class ActionControllerRedirectResponse extends HTTP::Server::HttpRedirectResponse::Range {
161+
RedirectToCall redirectToCall;
162+
163+
ActionControllerRedirectResponse() { this.asExpr().getExpr() = redirectToCall }
164+
165+
override DataFlow::Node getBody() { none() }
166+
167+
override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
168+
169+
override string getMimetypeDefault() { none() }
170+
171+
override DataFlow::Node getRedirectLocation() {
172+
result.asExpr().getExpr() = redirectToCall.getRedirectUrl()
173+
}
174+
}
175+
157176
/**
158177
* A method in an `ActionController` class that is accessible from within a
159178
* Rails view as a helper method. For instance, in:

‎ql/lib/codeql/ruby/security/UrlRedirectCustomizations.qll‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for detecting "URL
3+
* redirection" vulnerabilities, as well as extension points for adding your
4+
* own.
5+
*/
6+
7+
private import ruby
8+
private import codeql.ruby.DataFlow
9+
private import codeql.ruby.Concepts
10+
private import codeql.ruby.dataflow.RemoteFlowSources
11+
private import codeql.ruby.dataflow.BarrierGuards
12+
13+
/**
14+
* Provides default sources, sinks and sanitizers for detecting
15+
* "URL redirection" vulnerabilities, as well as extension points for
16+
* adding your own.
17+
*/
18+
module UrlRedirect {
19+
/**
20+
* A data flow source for "URL redirection" vulnerabilities.
21+
*/
22+
abstract class Source extends DataFlow::Node { }
23+
24+
/**
25+
* A data flow sink for "URL redirection" vulnerabilities.
26+
*/
27+
abstract class Sink extends DataFlow::Node { }
28+
29+
/**
30+
* A sanitizer for "URL redirection" vulnerabilities.
31+
*/
32+
abstract class Sanitizer extends DataFlow::Node { }
33+
34+
/**
35+
* A sanitizer guard for "URL redirection" vulnerabilities.
36+
*/
37+
abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
38+
39+
/**
40+
* Additional taint steps for "URL redirection" vulnerabilities.
41+
*/
42+
predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
43+
taintStepViaMethodCallReturnValue(node1, node2)
44+
}
45+
46+
/**
47+
* A source of remote user input, considered as a flow source.
48+
*/
49+
class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
50+
51+
/**
52+
* A HTTP redirect response, considered as a flow sink.
53+
*/
54+
class RedirectLocationAsSink extends Sink {
55+
RedirectLocationAsSink() {
56+
exists(HTTP::Server::HttpRedirectResponse e |
57+
this = e.getRedirectLocation() and
58+
// As a rough heuristic, assume that methods with these names are handlers for POST/PUT/PATCH/DELETE requests,
59+
// which are not as vulnerable to URL redirection because browsers will not initiate them from clicking a link.
60+
not this.getEnclosingCallable()
61+
.(Method)
62+
.getName()
63+
.regexpMatch(".*(create|update|destroy).*")
64+
)
65+
}
66+
}
67+
68+
/**
69+
* A comparison with a constant string, considered as a sanitizer-guard.
70+
*/
71+
class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
72+
73+
/**
74+
* Some methods will propagate taint to their return values.
75+
* Here we cover a few common ones related to `ActionController::Parameters`.
76+
* TODO: use ApiGraphs or something to restrict these method calls to the correct receiver, rather
77+
* than matching on method name alone.
78+
*/
79+
predicate taintStepViaMethodCallReturnValue(DataFlow::Node node1, DataFlow::Node node2) {
80+
exists(MethodCall m | m = node2.asExpr().getExpr() |
81+
m.getReceiver() = node1.asExpr().getExpr() and
82+
(actionControllerTaintedMethod(m) or hashTaintedMethod(m))
83+
)
84+
}
85+
86+
/**
87+
* String interpolation is considered safe, provided the string is prefixed by a non-tainted value.
88+
* In most cases this will prevent the tainted value from controlling e.g. the host of the URL.
89+
*
90+
* For example:
91+
*
92+
* ```ruby
93+
* redirect_to "/users/#{params[:key]}" # safe
94+
* redirect_to "#{params[:key]}/users" # unsafe
95+
* ```
96+
*
97+
* There are prefixed interpolations that are not safe, e.g.
98+
*
99+
* ```ruby
100+
* redirect_to "foo#{params[:key]}/users" # => "foo-malicious-site.com/users"
101+
* ```
102+
*
103+
* We currently don't catch these cases.
104+
*/
105+
class StringInterpolationAsSanitizer extends Sanitizer {
106+
StringInterpolationAsSanitizer() {
107+
exists(StringlikeLiteral str, int n | str.getComponent(n) = this.asExpr().getExpr() and n > 0)
108+
}
109+
}
110+
111+
/**
112+
* These methods return a new `ActionController::Parameters` or a `Hash` containing a subset of
113+
* the original values. This may still contain user input, so the results are tainted.
114+
* TODO: flesh this out to cover the whole API.
115+
*/
116+
predicate actionControllerTaintedMethod(MethodCall m) {
117+
m.getMethodName() in ["to_unsafe_hash", "to_unsafe_h", "permit", "require"]
118+
}
119+
120+
/**
121+
* These `Hash` methods preserve taint because they return a new hash which may still contain keys
122+
* with user input.
123+
* TODO: flesh this out to cover the whole API.
124+
*/
125+
predicate hashTaintedMethod(MethodCall m) { m.getMethodName() in ["merge", "fetch"] }
126+
}

‎ql/lib/codeql/ruby/security/UrlRedirectQuery.qll‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
/**
2+
* Provides a taint-tracking configuration for detecting "URL redirection" vulnerabilities.
3+
*
4+
* Note, for performance reasons: only import this file if `Configuration` is needed,
5+
* otherwise `UrlRedirectCustomizations` should be imported instead.
6+
*/
7+
8+
private import ruby
9+
import codeql.ruby.DataFlow::DataFlow::PathGraph
10+
import codeql.ruby.DataFlow
11+
import codeql.ruby.TaintTracking
12+
import UrlRedirectCustomizations
13+
import UrlRedirectCustomizations::UrlRedirect
14+
15+
/**
16+
* A taint-tracking configuration for detecting "URL redirection" vulnerabilities.
17+
*/
18+
class Configuration extends TaintTracking::Configuration {
19+
Configuration() { this = "UrlRedirect" }
20+
21+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
22+
23+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
24+
25+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
26+
27+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
28+
guard instanceof SanitizerGuard
29+
}
30+
31+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
32+
UrlRedirect::isAdditionalTaintStep(node1, node2)
33+
}
34+
}

‎ql/src/queries/security/cwe-601/UrlRedirect.qhelp‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>
6+
Directly incorporating user input into a URL redirect request without validating the input
7+
can facilitate phishing attacks. In these attacks, unsuspecting users can be redirected to a
8+
malicious site that looks very similar to the real site they intend to visit, but which is
9+
controlled by the attacker.
10+
</p>
11+
</overview>
12+
13+
<recommendation>
14+
<p>
15+
To guard against untrusted URL redirection, it is advisable to avoid putting user input
16+
directly into a redirect URL. Instead, maintain a list of authorized
17+
redirects on the server; then choose from that list based on the user input provided.
18+
</p>
19+
</recommendation>
20+
21+
<example>
22+
<p>
23+
The following example shows an HTTP request parameter being used directly in a URL redirect
24+
without validating the input, which facilitates phishing attacks:
25+
</p>
26+
27+
<sample src="examples/redirect_bad.rb"/>
28+
29+
<p>
30+
One way to remedy the problem is to validate the user input against a known fixed string
31+
before doing the redirection:
32+
</p>
33+
34+
<sample src="examples/redirect_good.rb"/>
35+
</example>
36+
37+
<references>
38+
<li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html">
39+
XSS Unvalidated Redirects and Forwards Cheat Sheet</a>.</li>
40+
<li>Rails Guides: <a href="https://guides.rubyonrails.org/security.html#redirection-and-files">
41+
Redirection and Files</a>.</li>
42+
</references>
43+
44+
</qhelp>

‎ql/src/queries/security/cwe-601/UrlRedirect.ql‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* @name URL redirection from remote source
3+
* @description URL redirection based on unvalidated user input
4+
* may cause redirection to malicious web sites.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 6.1
8+
* @sub-severity low
9+
* @id rb/url-redirection
10+
* @tags security
11+
* external/cwe/cwe-601
12+
* @precision high
13+
*/
14+
15+
import ruby
16+
import codeql.ruby.security.UrlRedirectQuery
17+
import codeql.ruby.DataFlow::DataFlow::PathGraph
18+
19+
from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
20+
where config.hasFlowPath(source, sink)
21+
select sink.getNode(), source, sink, "Untrusted URL redirection due to $@.", source.getNode(),
22+
"a user-provided value"

‎ql/src/queries/security/cwe-601/examples/redirect_bad.rb‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
class HelloController < ActionController::Base
2+
def hello
3+
redirect_to params[:url]
4+
end
5+
end

‎ql/src/queries/security/cwe-601/examples/redirect_good.rb‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
class HelloController < ActionController::Base
2+
VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html"
3+
4+
def hello
5+
if params[:url] == VALID_REDIRECT
6+
redirect_to params[:url]
7+
else
8+
# error
9+
end
10+
end
11+
end

‎ql/test/query-tests/security/cwe-601/UrlRedirect.expected‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
edges
2+
| UrlRedirect.rb:9:17:9:22 | call to params : | UrlRedirect.rb:9:17:9:28 | ...[...] |
3+
| UrlRedirect.rb:14:17:14:22 | call to params : | UrlRedirect.rb:14:17:14:43 | call to fetch |
4+
| UrlRedirect.rb:19:17:19:22 | call to params : | UrlRedirect.rb:19:17:19:37 | call to to_unsafe_hash |
5+
| UrlRedirect.rb:24:31:24:36 | call to params : | UrlRedirect.rb:24:17:24:37 | call to filter_params |
6+
| UrlRedirect.rb:34:20:34:25 | call to params : | UrlRedirect.rb:34:17:34:37 | "#{...}/foo" |
7+
nodes
8+
| UrlRedirect.rb:4:17:4:22 | call to params | semmle.label | call to params |
9+
| UrlRedirect.rb:9:17:9:22 | call to params : | semmle.label | call to params : |
10+
| UrlRedirect.rb:9:17:9:28 | ...[...] | semmle.label | ...[...] |
11+
| UrlRedirect.rb:14:17:14:22 | call to params : | semmle.label | call to params : |
12+
| UrlRedirect.rb:14:17:14:43 | call to fetch | semmle.label | call to fetch |
13+
| UrlRedirect.rb:19:17:19:22 | call to params : | semmle.label | call to params : |
14+
| UrlRedirect.rb:19:17:19:37 | call to to_unsafe_hash | semmle.label | call to to_unsafe_hash |
15+
| UrlRedirect.rb:24:17:24:37 | call to filter_params | semmle.label | call to filter_params |
16+
| UrlRedirect.rb:24:31:24:36 | call to params : | semmle.label | call to params : |
17+
| UrlRedirect.rb:34:17:34:37 | "#{...}/foo" | semmle.label | "#{...}/foo" |
18+
| UrlRedirect.rb:34:20:34:25 | call to params : | semmle.label | call to params : |
19+
#select
20+
| UrlRedirect.rb:4:17:4:22 | call to params | UrlRedirect.rb:4:17:4:22 | call to params | UrlRedirect.rb:4:17:4:22 | call to params | Untrusted URL redirection due to $@. | UrlRedirect.rb:4:17:4:22 | call to params | a user-provided value |
21+
| UrlRedirect.rb:9:17:9:28 | ...[...] | UrlRedirect.rb:9:17:9:22 | call to params : | UrlRedirect.rb:9:17:9:28 | ...[...] | Untrusted URL redirection due to $@. | UrlRedirect.rb:9:17:9:22 | call to params | a user-provided value |
22+
| UrlRedirect.rb:14:17:14:43 | call to fetch | UrlRedirect.rb:14:17:14:22 | call to params : | UrlRedirect.rb:14:17:14:43 | call to fetch | Untrusted URL redirection due to $@. | UrlRedirect.rb:14:17:14:22 | call to params | a user-provided value |
23+
| UrlRedirect.rb:19:17:19:37 | call to to_unsafe_hash | UrlRedirect.rb:19:17:19:22 | call to params : | UrlRedirect.rb:19:17:19:37 | call to to_unsafe_hash | Untrusted URL redirection due to $@. | UrlRedirect.rb:19:17:19:22 | call to params | a user-provided value |
24+
| UrlRedirect.rb:24:17:24:37 | call to filter_params | UrlRedirect.rb:24:31:24:36 | call to params : | UrlRedirect.rb:24:17:24:37 | call to filter_params | Untrusted URL redirection due to $@. | UrlRedirect.rb:24:31:24:36 | call to params | a user-provided value |
25+
| UrlRedirect.rb:34:17:34:37 | "#{...}/foo" | UrlRedirect.rb:34:20:34:25 | call to params : | UrlRedirect.rb:34:17:34:37 | "#{...}/foo" | Untrusted URL redirection due to $@. | UrlRedirect.rb:34:20:34:25 | call to params | a user-provided value |

‎ql/test/query-tests/security/cwe-601/UrlRedirect.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
queries/security/cwe-601/UrlRedirect.ql

‎ql/test/query-tests/security/cwe-601/UrlRedirect.rb‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
class UsersController < ActionController::Base
2+
# BAD
3+
def route1
4+
redirect_to params
5+
end
6+
7+
# BAD
8+
def route2
9+
redirect_to params[:key]
10+
end
11+
12+
# BAD
13+
def route3
14+
redirect_to params.fetch(:specific_arg)
15+
end
16+
17+
# BAD
18+
def route4
19+
redirect_to params.to_unsafe_hash
20+
end
21+
22+
# BAD
23+
def route5
24+
redirect_to filter_params(params)
25+
end
26+
27+
# GOOD
28+
def route6
29+
redirect_to "/foo/#{params[:key]}"
30+
end
31+
32+
# BAD
33+
def route7
34+
redirect_to "#{params[:key]}/foo"
35+
end
36+
37+
# GOOD
38+
def route8
39+
key = params[:key]
40+
if key == "foo"
41+
redirect_to key
42+
else
43+
redirect_to "/default"
44+
end
45+
end
46+
47+
# GOOD
48+
# Technically vulnerable, but we assume this is a handler for a POST request,
49+
# so can't be triggered by following a link.
50+
def create
51+
redirect_to params[:key]
52+
end
53+
54+
private
55+
56+
def filter_params(input_params)
57+
input_params.permit(:key)
58+
end
59+
end

0 commit comments

Comments
 (0)