Skip to content

Commit 49d826f

Browse files
hmachmac
committed
Ruby: Add a query for CSRF protection not enabled
Specifically in Rails apps, we look for root ActionController classes without a call to `protect_from_forgery`.
1 parent a0f91fb commit 49d826f

7 files changed

Lines changed: 144 additions & 33 deletions

File tree

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

Lines changed: 47 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,35 @@ private import codeql.ruby.dataflow.internal.DataFlowDispatch
2121
module ActionController {
2222
// TODO: move the rest of this file inside this module.
2323
import codeql.ruby.frameworks.actioncontroller.Filters
24+
25+
/**
26+
* An ActionController class which sits at the top of the class hierarchy.
27+
* In other words, it does not subclass any other class in source code.
28+
*/
29+
class RootController extends ActionControllerClass {
30+
RootController() {
31+
not exists(ActionControllerClass parent | this != parent and this = parent.getADescendent())
32+
}
33+
}
34+
35+
/**
36+
* A call to `protect_from_forgery`.
37+
*/
38+
class ProtectFromForgeryCall extends CsrfProtectionSetting::Range, DataFlow::CallNode {
39+
ProtectFromForgeryCall() {
40+
this = actionControllerInstance().getAMethodCall("protect_from_forgery")
41+
}
42+
43+
private string getWithValueText() {
44+
result = this.getKeywordArgument("with").getConstantValue().getSymbol()
45+
}
46+
47+
// Calls without `with: :exception` can allow for bypassing CSRF protection
48+
// in some scenarios.
49+
override boolean getVerificationSetting() {
50+
if this.getWithValueText() = "exception" then result = true else result = false
51+
}
52+
}
2453
}
2554

2655
/**
@@ -38,18 +67,10 @@ module ActionController {
3867
*/
3968
class ActionControllerClass extends DataFlow::ClassNode {
4069
ActionControllerClass() {
41-
this =
42-
[
43-
DataFlow::getConstant("ActionController").getConstant("Base"),
44-
// In Rails applications `ApplicationController` typically extends `ActionController::Base`, but we
45-
// treat it separately in case the `ApplicationController` definition is not in the database.
46-
DataFlow::getConstant("ApplicationController"),
47-
// ActionController::Metal technically doesn't contain all of the
48-
// methods available in Base, such as those for rendering views.
49-
// However we prefer to be over-sensitive in this case in order to find
50-
// more results.
51-
DataFlow::getConstant("ActionController").getConstant("Metal")
52-
].getADescendentModule()
70+
this = DataFlow::getConstant("ApplicationController").getADescendentModule()
71+
or
72+
this = actionControllerBaseClass().getADescendentModule() and
73+
not exists(DataFlow::ModuleNode m | m = actionControllerBaseClass().asModule() | this = m)
5374
}
5475

5576
/**
@@ -73,6 +94,20 @@ class ActionControllerClass extends DataFlow::ClassNode {
7394
}
7495
}
7596

97+
private DataFlow::ConstRef actionControllerBaseClass() {
98+
result =
99+
[
100+
// In Rails applications `ApplicationController` typically extends `ActionController::Base`, but we
101+
// treat it separately in case the `ApplicationController` definition is not in the database.
102+
DataFlow::getConstant("ActionController").getConstant("Base"),
103+
// ActionController::Metal technically doesn't contain all of the
104+
// methods available in Base, such as those for rendering views.
105+
// However we prefer to be over-sensitive in this case in order to find
106+
// more results.
107+
DataFlow::getConstant("ActionController").getConstant("Metal")
108+
]
109+
}
110+
76111
private API::Node actionControllerInstance() {
77112
result = any(ActionControllerClass cls).getSelf().track()
78113
}
@@ -406,27 +441,6 @@ class ActionControllerSkipForgeryProtectionCall extends CsrfProtectionSetting::R
406441
override boolean getVerificationSetting() { result = false }
407442
}
408443

409-
/**
410-
* A call to `protect_from_forgery`.
411-
*/
412-
private class ActionControllerProtectFromForgeryCall extends CsrfProtectionSetting::Range,
413-
DataFlow::CallNode
414-
{
415-
ActionControllerProtectFromForgeryCall() {
416-
this = actionControllerInstance().getAMethodCall("protect_from_forgery")
417-
}
418-
419-
private string getWithValueText() {
420-
result = this.getKeywordArgument("with").getConstantValue().getSymbol()
421-
}
422-
423-
// Calls without `with: :exception` can allow for bypassing CSRF protection
424-
// in some scenarios.
425-
override boolean getVerificationSetting() {
426-
if this.getWithValueText() = "exception" then result = true else result = false
427-
}
428-
}
429-
430444
/**
431445
* A call to `send_file`, which sends the file at the given path to the client.
432446
*/

‎ruby/ql/src/queries/security/cwe-352/CSRFProtectionNotEnabled.qhelp‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Cross-site request forgery (CSRF) is a type of vulnerability in which an
9+
attacker is able to force a user to carry out an action that the user did
10+
not intend.
11+
</p>
12+
13+
<p>
14+
The attacker tricks an authenticated user into submitting a request to the
15+
web application. Typically this request will result in a state change on
16+
the server, such as changing the user's password. The request can be
17+
initiated when the user visits a site controlled by the attacker. If the
18+
web application relies only on cookies for authentication, or on other
19+
credentials that are automatically included in the request, then this
20+
request will appear as legitimate to the server.
21+
</p>
22+
23+
<p>
24+
A common countermeasure for CSRF is to generate a unique token to be
25+
included in the HTML sent from the server to a user. This token can be
26+
used as a hidden field to be sent back with requests to the server, where
27+
the server can then check that the token is valid and associated with the
28+
relevant user session.
29+
</p>
30+
</overview>
31+
32+
<recommendation>
33+
<p>
34+
In the Rails web framework, CSRF protection is enabled by the adding a call to
35+
the <code>protect_from_forgery</code> method inside an
36+
<code>ActionController</code> class. Typically this is done in the
37+
<code>ApplicationController</code> class, or an equivalent class from which
38+
other controller classes are subclassed.
39+
40+
The default behaviour of this method is to null the session when an invalid
41+
CSRF token is provided. This may not be sufficient to avoid a CSRF
42+
vulnerability - for example if parts of the session are memoized. Calling
43+
<code>protect_from_forgery with: :exception</code> can help to avoid this
44+
by raising an exception on an invalid CSRF token instead.
45+
</p>
46+
</recommendation>
47+
48+
<example>
49+
<p>
50+
The following example shows a case where CSRF protection is enabled with
51+
a secure request handling strategy of <code>:exception</code>.
52+
</p>
53+
54+
<sample src="examples/ProtectFromForgeryGood.rb"/>
55+
56+
</example>
57+
58+
<references>
59+
<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Cross-site_request_forgery">Cross-site request forgery</a></li>
60+
<li>OWASP: <a href="https://owasp.org/www-community/attacks/csrf">Cross-site request forgery</a></li>
61+
<li>Securing Rails Applications: <a href="https://guides.rubyonrails.org/security.html#cross-site-request-forgery-csrf">Cross-Site Request Forgery (CSRF)</a></li>
62+
<li>Veracode: <a href="https://www.veracode.com/blog/managing-appsec/when-rails-protectfromforgery-fails">When Rails' protect_from_forgery Fails</a>.</li>
63+
</references>
64+
65+
</qhelp>

‎ruby/ql/src/queries/security/cwe-352/CSRFProtectionNotEnabled.ql‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name CSRF protection not enabled
3+
* @description Not enabling CSRF protection may make the application
4+
* vulnerable to a Cross-Site Request Forgery (CSRF) attack.
5+
* @kind problem
6+
* @problem.severity warning
7+
* @security-severity 8.8
8+
* @precision high
9+
* @id rb/csrf-protection-not-enabled
10+
* @tags security
11+
* external/cwe/cwe-352
12+
*/
13+
14+
import codeql.ruby.AST
15+
import codeql.ruby.Concepts
16+
import codeql.ruby.frameworks.ActionController
17+
18+
from ActionController::RootController c
19+
where
20+
not exists(ActionController::ProtectFromForgeryCall call |
21+
c.getSelf().flowsTo(call.getReceiver())
22+
)
23+
select c, "Potential CSRF vulnerability due to forgery protection not being enabled"

‎ruby/ql/src/queries/security/cwe-352/examples/ProtectFromForgeryGood.rb‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
class ApplicationController < ActionController::Base
2+
protect_from_forgery with: :exception
3+
end
4+

‎ruby/ql/test/query-tests/security/cwe-352/CSRFProtectionNotEnabled.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
| railsapp/app/controllers/alternative_root_controller.rb:1:1:3:3 | AlternativeRootController | Potential CSRF vulnerability due to forgery protection not being enabled |

‎ruby/ql/test/query-tests/security/cwe-352/CSRFProtectionNotEnabled.qlref‎

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

‎ruby/ql/test/query-tests/security/cwe-352/railsapp/app/controllers/alternative_root_controller.rb‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
class AlternativeRootController < ActionController::Base
2+
# BAD: no protect_from_forgery call
3+
end

0 commit comments

Comments
 (0)