Skip to content

Commit c00e207

Browse files
erik-krogherik-krogh
authored
Merge pull request #16111 from erik-krogh/rb-url
RB: Improve QHelp for `rb/url-redirect`, and fix an FP.
2 parents 982765c + 844e78d commit c00e207

File tree

8 files changed

+125
-28
lines changed

8 files changed

+125
-28
lines changed

‎ruby/ql/lib/codeql/ruby/ast/internal/Constant.qll

+4
Original file line numberDiff line numberDiff line change
@@ -560,6 +560,10 @@ private predicate isArrayExpr(Expr e, ArrayLiteralCfgNode arr) {
560560
// Note(hmac): I don't think this is necessary, as `getSource` will not return
561561
// results if the source is a phi node.
562562
forex(ExprCfgNode n | n = e.getAControlFlowNode() | isArrayConstant(n, arr))
563+
or
564+
// if `e` is an array, then `e.freeze` is also an array
565+
e.(MethodCall).getMethodName() = "freeze" and
566+
isArrayExpr(e.(MethodCall).getReceiver(), arr)
563567
}
564568

565569
private class TokenConstantAccess extends ConstantAccess, TTokenConstantAccess {

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

+6
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,12 @@ module UrlRedirect {
8383
*/
8484
class StringConstCompareAsSanitizer extends Sanitizer, StringConstCompareBarrier { }
8585

86+
/**
87+
* A string concatenation against a constant list, considered as a sanitizer-guard.
88+
*/
89+
class StringConstArrayInclusionAsSanitizer extends Sanitizer, StringConstArrayInclusionCallBarrier
90+
{ }
91+
8692
/**
8793
* Some methods will propagate taint to their return values.
8894
* Here we cover a few common ones related to `ActionController::Parameters`.

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

+41-17
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,68 @@
1-
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
24
<qhelp>
35

6+
47
<overview>
5-
<p>
6-
Directly incorporating user input into a URL redirect request without validating the input
8+
<p>Directly incorporating user input into a URL redirect request without validating the input
79
can facilitate phishing attacks. In these attacks, unsuspecting users can be redirected to a
810
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>
11+
controlled by the attacker.</p>
1212

13+
</overview>
1314
<recommendation>
14-
<p>
15-
To guard against untrusted URL redirection, it is advisable to avoid putting user input
15+
16+
<p>To guard against untrusted URL redirection, it is advisable to avoid putting user input
1617
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+
redirects on the server; then choose from that list based on the user input provided.</p>
19+
<p>
20+
If this is not possible, then the user input should be validated in some other way,
21+
for example, by verifying that the target URL is on the same host as the current page.
1822
</p>
1923
</recommendation>
2024

25+
2126
<example>
2227
<p>
2328
The following example shows an HTTP request parameter being used directly in a URL redirect
2429
without validating the input, which facilitates phishing attacks:
2530
</p>
2631

27-
<sample src="examples/redirect_bad.rb"/>
32+
<sample src="examples/url_redirect.rb"/>
2833

2934
<p>
30-
One way to remedy the problem is to validate the user input against a known fixed string
35+
One way to remedy the problem is to validate the user input against a set of known fixed strings
3136
before doing the redirection:
3237
</p>
3338

34-
<sample src="examples/redirect_good.rb"/>
39+
<sample src="examples/url_redirect_good.rb"/>
40+
41+
<p>
42+
Alternatively, we can check that the target URL does not redirect to a different host
43+
by checking that the URL is either relative or on a known good host:
44+
</p>
45+
46+
<sample src="examples/url_redirect_domain.rb"/>
47+
48+
<p>
49+
Note that as written, the above code will allow redirects to URLs on <code>example.com</code>,
50+
which is harmless but perhaps not intended. You can substitute your own domain (if known) for
51+
<code>example.com</code> to prevent this.
52+
</p>
53+
3554
</example>
3655

3756
<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>
4357

58+
<li>
59+
OWASP:
60+
<a href="https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html">
61+
Unvalidated Redirects and Forwards Cheat Sheet</a>.
62+
</li>
63+
<li>
64+
Rails Guides: <a href="https://guides.rubyonrails.org/security.html#redirection-and-files">Redirection and Files</a>.
65+
</li>
66+
67+
</references>
4468
</qhelp>

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

-11
This file was deleted.

‎ruby/ql/src/queries/security/cwe-601/examples/redirect_bad.rb renamed to ‎ruby/ql/src/queries/security/cwe-601/examples/url_redirect.rb

File renamed without changes.

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

+21
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
require 'uri'
2+
3+
class HelloController < ActionController::Base
4+
KNOWN_HOST = "example.org"
5+
6+
def hello
7+
begin
8+
target_url = URI.parse(params[:url])
9+
10+
# Redirect if the URL is either relative or on a known good host
11+
if !target_url.host || target_url.host == KNOWN_HOST
12+
redirect_to target_url.to_s
13+
else
14+
redirect_to "/error.html" # Redirect to error page if the host is not known
15+
end
16+
rescue URI::InvalidURIError
17+
# Handle the exception, for example, by redirecting to a safe page
18+
redirect_to "/error.html"
19+
end
20+
end
21+
end

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

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
class HelloController < ActionController::Base
2+
VALID_REDIRECTS = [
3+
"http://cwe.mitre.org/data/definitions/601.html",
4+
"http://cwe.mitre.org/data/definitions/79.html"
5+
].freeze
6+
7+
def hello
8+
# GOOD: the request parameter is validated against a known list of URLs
9+
target_url = params[:url]
10+
if VALID_REDIRECTS.include?(target_url)
11+
redirect_to target_url
12+
else
13+
redirect_to "/error.html"
14+
end
15+
end
16+
end

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

+37
Original file line numberDiff line numberDiff line change
@@ -98,3 +98,40 @@ def filter_params(input_params)
9898
Rails.routes.draw do
9999
get "/r9", to: "users#route9"
100100
end
101+
102+
class DomainController < ActionController::Base
103+
KNOWN_HOST = "example.org"
104+
105+
def hello
106+
begin
107+
target_url = URI.parse(params[:url])
108+
109+
# Redirect if the URL is either relative or on a known good host
110+
if !target_url.host || target_url.host == KNOWN_HOST
111+
redirect_to target_url.to_s
112+
else
113+
redirect_to "/error.html" # Redirect to error page if the host is not known
114+
end
115+
rescue URI::InvalidURIError
116+
# Handle the exception, for example, by redirecting to a safe page
117+
redirect_to "/error.html"
118+
end
119+
end
120+
end
121+
122+
class ConstController < ActionController::Base
123+
VALID_REDIRECTS = [
124+
"http://cwe.mitre.org/data/definitions/601.html",
125+
"http://cwe.mitre.org/data/definitions/79.html"
126+
].freeze
127+
128+
def hello
129+
# GOOD: the request parameter is validated against a known list of URLs
130+
target_url = params[:url]
131+
if VALID_REDIRECTS.include?(target_url)
132+
redirect_to target_url
133+
else
134+
redirect_to "/error.html"
135+
end
136+
end
137+
end

0 commit comments

Comments
 (0)