Skip to content

Commit 409f46e

Browse files
hmachmac
authored
Merge pull request github#14308 from hmac/hmac-rb-csrf-not-enabled
Ruby: Add a query for CSRF protection not enabled
2 parents 3c8c458 + 806f42e commit 409f46e

File tree

17 files changed

+490
-35
lines changed

17 files changed

+490
-35
lines changed

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

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

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

5679
/**
@@ -74,6 +97,18 @@ class ActionControllerClass extends DataFlow::ClassNode {
7497
}
7598
}
7699

100+
private DataFlow::ConstRef actionControllerBaseClass() {
101+
result =
102+
[
103+
DataFlow::getConstant("ActionController").getConstant("Base"),
104+
// ActionController::Metal and ActionController::API technically don't contain all of the
105+
// methods available in Base, such as those for rendering views.
106+
// However we prefer to be over-sensitive in this case in order to find more results.
107+
DataFlow::getConstant("ActionController").getConstant("Metal"),
108+
DataFlow::getConstant("ActionController").getConstant("API")
109+
]
110+
}
111+
77112
private API::Node actionControllerInstance() {
78113
result = any(ActionControllerClass cls).getSelf().track()
79114
}
@@ -407,27 +442,6 @@ class ActionControllerSkipForgeryProtectionCall extends CsrfProtectionSetting::R
407442
override boolean getVerificationSetting() { result = false }
408443
}
409444

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

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

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,254 @@
1+
/**
2+
* Provides classes and predicates for Gemfiles, including version constraint logic.
3+
*/
4+
5+
private import codeql.ruby.AST
6+
7+
/**
8+
* Provides classes and predicates for Gemfiles, including version constraint logic.
9+
*/
10+
module Gemfile {
11+
private File getGemfile() { result.getBaseName() = "Gemfile" }
12+
13+
/**
14+
* A call to `gem` inside a gemfile. This defines a dependency. For example:
15+
*
16+
* ```rb
17+
* gem "actionpack", "~> 7.0.0"
18+
* ```
19+
*
20+
* This call defines a dependency on the `actionpack` gem, with version constraint `~> 7.0.0`.
21+
* For detail on version constraints, see the `VersionConstraint` class.
22+
*/
23+
class Gem extends MethodCall {
24+
Gem() { this.getMethodName() = "gem" and this.getFile() = getGemfile() }
25+
26+
/**
27+
* Gets the name of the gem in this version constraint.
28+
*/
29+
string getName() { result = this.getArgument(0).getConstantValue().getStringlikeValue() }
30+
31+
/**
32+
* Gets the `i`th version string for this gem. A single `gem` call may have multiple version constraints, for example:
33+
*
34+
* ```rb
35+
* gem "json", "3.4.0", ">= 3.0"
36+
* ```
37+
*/
38+
string getVersionString(int i) {
39+
result = this.getArgument(i + 1).getConstantValue().getStringlikeValue()
40+
}
41+
42+
/**
43+
* Gets a version constraint defined by this call.
44+
*/
45+
VersionConstraint getAVersionConstraint() { result = this.getVersionString(_) }
46+
}
47+
48+
private newtype TComparator =
49+
TEq() or
50+
TNeq() or
51+
TGt() or
52+
TLt() or
53+
TGeq() or
54+
TLeq() or
55+
TPGeq()
56+
57+
/**
58+
* A comparison operator in a version constraint.
59+
*/
60+
private class Comparator extends TComparator {
61+
string toString() { result = this.toSourceString() }
62+
63+
/**
64+
* Gets the representation of the comparator in source code.
65+
* This is defined separately so that we can change the `toString` implementation without breaking `parseConstraint`.
66+
*/
67+
string toSourceString() {
68+
this = TEq() and result = "="
69+
or
70+
this = TNeq() and result = "!="
71+
or
72+
this = TGt() and result = ">"
73+
or
74+
this = TLt() and result = "<"
75+
or
76+
this = TGeq() and result = ">="
77+
or
78+
this = TLeq() and result = "<="
79+
or
80+
this = TPGeq() and result = "~>"
81+
}
82+
}
83+
84+
bindingset[s]
85+
private predicate parseExactVersion(string s, string version) {
86+
version = s.regexpCapture("\\s*(\\d+\\.\\d+\\.\\d+)\\s*", 1)
87+
}
88+
89+
bindingset[s]
90+
private predicate parseConstraint(string s, Comparator c, string version) {
91+
exists(string pattern | pattern = "(=|!=|>=?|<=?|~>)\\s+(.+)" |
92+
c.toSourceString() = s.regexpCapture(pattern, 1) and version = s.regexpCapture(pattern, 2)
93+
)
94+
}
95+
96+
/**
97+
* A version constraint in a `gem` call. This consists of a version number and an optional comparator, for example
98+
* `>= 1.2.3`.
99+
*/
100+
class VersionConstraint extends string {
101+
Comparator comp;
102+
string versionString;
103+
104+
VersionConstraint() {
105+
this = any(Gem g).getVersionString(_) and
106+
(
107+
parseConstraint(this, comp, versionString)
108+
or
109+
parseExactVersion(this, versionString) and comp = TEq()
110+
)
111+
}
112+
113+
/**
114+
* Gets the string defining the version number used in this constraint.
115+
*/
116+
string getVersionString() { result = versionString }
117+
118+
/**
119+
* Gets the `Version` used in this constraint.
120+
*/
121+
Version getVersion() { result = this.getVersionString() }
122+
123+
/**
124+
* Holds if `other` is a version which is strictly greater than the range described by this version constraint.
125+
*/
126+
bindingset[other]
127+
predicate before(string other) {
128+
comp = TEq() and this.getVersion().before(other)
129+
or
130+
comp = TLt() and
131+
(this.getVersion().before(other) or this.getVersion().equal(other))
132+
or
133+
comp = TLeq() and this.getVersion().before(other)
134+
or
135+
// ~> x.y.z <=> >= x.y.z && < x.(y+1).0
136+
// ~> x.y <=> >= x.y && < (x+1).0
137+
comp = TPGeq() and
138+
exists(int thisMajor, int thisMinor, int otherMajor, int otherMinor |
139+
thisMajor = this.getVersion().getMajor() and
140+
thisMinor = this.getVersion().getMinor() and
141+
exists(string maj, string mi | normalizeSemver(other, _, maj, mi, _) |
142+
otherMajor = maj.toInt() and otherMinor = mi.toInt()
143+
)
144+
|
145+
exists(this.getVersion().getPatch()) and
146+
(
147+
thisMajor < otherMajor
148+
or
149+
thisMajor = otherMajor and
150+
thisMinor < otherMinor
151+
)
152+
or
153+
not exists(this.getVersion().getPatch()) and
154+
thisMajor < otherMajor
155+
)
156+
// if the comparator is > or >=, it has no upper bound and therefore isn't guaranteed to be before any other version.
157+
}
158+
}
159+
160+
/**
161+
* A version number in a version constraint. For example, in the following code
162+
*
163+
* ```rb
164+
* gem "json", ">= 3.4.5"
165+
* ```
166+
*
167+
* The version is `3.4.5`.
168+
*/
169+
private class Version extends string {
170+
string normalized;
171+
172+
Version() {
173+
this = any(Gem c).getAVersionConstraint().getVersionString() and
174+
normalized = normalizeSemver(this)
175+
}
176+
177+
/**
178+
* Holds if this version is strictly before the version defined by `other`.
179+
*/
180+
bindingset[other]
181+
predicate before(string other) { normalized < normalizeSemver(other) }
182+
183+
/**
184+
* Holds if this versino is equal to the version defined by `other`.
185+
*/
186+
bindingset[other]
187+
predicate equal(string other) { normalized = normalizeSemver(other) }
188+
189+
/**
190+
* Holds if this version is strictly after the version defined by `other`.
191+
*/
192+
bindingset[other]
193+
predicate after(string other) { normalized > normalizeSemver(other) }
194+
195+
/**
196+
* Holds if this version defines a patch number.
197+
*/
198+
predicate hasPatch() { exists(getPatch(this)) }
199+
200+
/**
201+
* Gets the major number of this version.
202+
*/
203+
int getMajor() { result = getMajor(normalized).toInt() }
204+
205+
/**
206+
* Gets the minor number of this version, if it exists.
207+
*/
208+
int getMinor() { result = getMinor(normalized).toInt() }
209+
210+
/**
211+
* Gets the patch number of this version, if it exists.
212+
*/
213+
int getPatch() { result = getPatch(normalized).toInt() }
214+
}
215+
216+
/**
217+
* Normalizes a SemVer string such that the lexicographical ordering
218+
* of two normalized strings is consistent with the SemVer ordering.
219+
*
220+
* Pre-release information and build metadata is not supported.
221+
*/
222+
bindingset[orig]
223+
private predicate normalizeSemver(
224+
string orig, string normalized, string major, string minor, string patch
225+
) {
226+
major = getMajor(orig) and
227+
(
228+
minor = getMinor(orig)
229+
or
230+
not exists(getMinor(orig)) and minor = "0"
231+
) and
232+
(
233+
patch = getPatch(orig)
234+
or
235+
not exists(getPatch(orig)) and patch = "0"
236+
) and
237+
normalized = leftPad(major) + "." + leftPad(minor) + "." + leftPad(patch)
238+
}
239+
240+
bindingset[orig]
241+
private string normalizeSemver(string orig) { normalizeSemver(orig, result, _, _, _) }
242+
243+
bindingset[s]
244+
private string getMajor(string s) { result = s.regexpCapture("(\\d+).*", 1) }
245+
246+
bindingset[s]
247+
private string getMinor(string s) { result = s.regexpCapture("(\\d+)\\.(\\d+).*", 2) }
248+
249+
bindingset[s]
250+
private string getPatch(string s) { result = s.regexpCapture("(\\d+)\\.(\\d+)\\.(\\d+).*", 3) }
251+
252+
bindingset[str]
253+
private string leftPad(string str) { result = ("000" + str).suffix(str.length()) }
254+
}

‎ruby/ql/src/change-notes/2023-09-25-csrf-protection-not-enabled.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `rb/csrf-protection-not-enabled`, to detect cases where Cross-Site Request Forgery protection is not enabled in Ruby on Rails controllers.

0 commit comments

Comments
 (0)