Skip to content

Commit f935df9

Browse files
alexrfordalexrford
authored
Merge pull request #7313 from github/ruby/rails-cookie-config
Ruby: Add `rb/weak-cookie-configuration` query
2 parents e96fcf8 + da8c745 commit f935df9

File tree

9 files changed

+351
-60
lines changed

9 files changed

+351
-60
lines changed

‎ruby/ql/lib/codeql/ruby/Concepts.qll‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -645,6 +645,37 @@ module Path {
645645
}
646646
}
647647

648+
/**
649+
* A data-flow node that may configure behavior relating to cookie security.
650+
*
651+
* Extend this class to refine existing API models. If you want to model new APIs,
652+
* extend `CookieSecurityConfigurationSetting::Range` instead.
653+
*/
654+
class CookieSecurityConfigurationSetting extends DataFlow::Node instanceof CookieSecurityConfigurationSetting::Range {
655+
/**
656+
* Gets a description of how this cookie setting may weaken application security.
657+
* This predicate has no results if the setting is considered to be safe.
658+
*/
659+
string getSecurityWarningMessage() { result = super.getSecurityWarningMessage() }
660+
}
661+
662+
/** Provides a class for modeling new cookie security setting APIs. */
663+
module CookieSecurityConfigurationSetting {
664+
/**
665+
* A data-flow node that may configure behavior relating to cookie security.
666+
*
667+
* Extend this class to model new APIs. If you want to refine existing API models,
668+
* extend `CookieSecurityConfigurationSetting` instead.
669+
*/
670+
abstract class Range extends DataFlow::Node {
671+
/**
672+
* Gets a description of how this cookie setting may weaken application security.
673+
* This predicate has no results if the setting is considered to be safe.
674+
*/
675+
abstract string getSecurityWarningMessage();
676+
}
677+
}
678+
648679
/**
649680
* A data-flow node that logs data.
650681
*

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

Lines changed: 196 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ private import codeql.ruby.frameworks.ActiveRecord
1212
private import codeql.ruby.frameworks.ActiveStorage
1313
private import codeql.ruby.ast.internal.Module
1414
private import codeql.ruby.ApiGraphs
15+
private import codeql.ruby.security.OpenSSL
1516

1617
/**
1718
* A reference to either `Rails::Railtie`, `Rails::Engine`, or `Rails::Application`.
@@ -47,85 +48,220 @@ private DataFlow::CallNode getAConfigureCallNode() {
4748
}
4849

4950
/**
50-
* An access to a Rails config object.
51+
* Classes representing accesses to the Rails config object.
5152
*/
52-
private class ConfigSourceNode extends DataFlow::LocalSourceNode {
53-
ConfigSourceNode() {
54-
// `Foo < Rails::Application ... config ...`
55-
exists(MethodCall configCall | this.asExpr().getExpr() = configCall |
56-
configCall.getMethodName() = "config" and
57-
configCall.getEnclosingModule() instanceof RailtieClass
58-
)
59-
or
60-
// `Rails.application.config`
61-
this =
62-
API::getTopLevelMember("Rails")
63-
.getReturn("application")
64-
.getReturn("config")
65-
.getAnImmediateUse()
66-
or
67-
// `Rails.application.configure { ... config ... }`
68-
// `Rails::Application.configure { ... config ... }`
69-
exists(DataFlow::CallNode configureCallNode, Block block, MethodCall configCall |
70-
configCall = this.asExpr().getExpr()
71-
|
72-
configureCallNode = getAConfigureCallNode() and
73-
block = configureCallNode.getBlock().asExpr().getExpr() and
74-
configCall.getParent+() = block and
75-
configCall.getMethodName() = "config"
76-
)
53+
private module Config {
54+
/**
55+
* An access to a Rails config object.
56+
*/
57+
private class SourceNode extends DataFlow::LocalSourceNode {
58+
SourceNode() {
59+
// `Foo < Rails::Application ... config ...`
60+
exists(MethodCall configCall | this.asExpr().getExpr() = configCall |
61+
configCall.getMethodName() = "config" and
62+
configCall.getEnclosingModule() instanceof RailtieClass
63+
)
64+
or
65+
// `Rails.application.config`
66+
this =
67+
API::getTopLevelMember("Rails")
68+
.getReturn("application")
69+
.getReturn("config")
70+
.getAnImmediateUse()
71+
or
72+
// `Rails.application.configure { ... config ... }`
73+
// `Rails::Application.configure { ... config ... }`
74+
exists(DataFlow::CallNode configureCallNode, Block block, MethodCall configCall |
75+
configCall = this.asExpr().getExpr()
76+
|
77+
configureCallNode = getAConfigureCallNode() and
78+
block = configureCallNode.asExpr().getExpr().(MethodCall).getBlock() and
79+
configCall.getParent+() = block and
80+
configCall.getMethodName() = "config"
81+
)
82+
}
83+
}
84+
85+
/**
86+
* A reference to the Rails config object.
87+
*/
88+
class Node extends DataFlow::Node {
89+
Node() { exists(SourceNode src | src.flowsTo(this)) }
7790
}
78-
}
7991

80-
private class ConfigNode extends DataFlow::Node {
81-
ConfigNode() { exists(ConfigSourceNode src | src.flowsTo(this)) }
92+
/**
93+
* A reference to the ActionController config object.
94+
*/
95+
class ActionControllerNode extends DataFlow::Node {
96+
ActionControllerNode() {
97+
exists(DataFlow::CallNode source |
98+
source.getReceiver() instanceof Node and
99+
source.getMethodName() = "action_controller"
100+
|
101+
source.flowsTo(this)
102+
)
103+
}
104+
}
105+
106+
/**
107+
* A reference to the ActionDispatch config object.
108+
*/
109+
class ActionDispatchNode extends DataFlow::Node {
110+
ActionDispatchNode() {
111+
exists(DataFlow::CallNode source |
112+
source.getReceiver() instanceof Node and
113+
source.getMethodName() = "action_dispatch"
114+
|
115+
source.flowsTo(this)
116+
)
117+
}
118+
}
82119
}
83120

84-
// A call where the Rails application config is the receiver
85-
private class CallAgainstConfig extends DataFlow::CallNode {
86-
CallAgainstConfig() { this.getReceiver() instanceof ConfigNode }
121+
/**
122+
* Classes representing nodes that set a Rails configuration value.
123+
*/
124+
private module Settings {
125+
private predicate isInTestConfiguration(Location loc) {
126+
loc.getFile().getRelativePath().matches("%test/%") or
127+
loc.getFile().getStem() = "test"
128+
}
129+
130+
private class Setting extends DataFlow::CallNode {
131+
Setting() {
132+
// exclude some test configuration
133+
not isInTestConfiguration(this.getLocation()) and
134+
this.getReceiver+() instanceof Config::Node and
135+
this.asExpr().getExpr() instanceof SetterMethodCall
136+
}
137+
}
138+
139+
private class LiteralSetting extends Setting {
140+
Literal valueLiteral;
141+
142+
LiteralSetting() {
143+
exists(DataFlow::LocalSourceNode lsn |
144+
lsn.asExpr().getExpr() = valueLiteral and
145+
lsn.flowsTo(this.getArgument(0))
146+
)
147+
}
148+
149+
string getValueText() { result = valueLiteral.getValueText() }
150+
151+
string getSettingString() { result = this.getMethodName() + this.getValueText() }
152+
}
153+
154+
/**
155+
* A node that sets a boolean value.
156+
*/
157+
class BooleanSetting extends LiteralSetting {
158+
override BooleanLiteral valueLiteral;
159+
160+
boolean getValue() { result = valueLiteral.getValue() }
161+
}
162+
163+
/**
164+
* A node that sets a Stringlike value.
165+
*/
166+
class StringlikeSetting extends LiteralSetting {
167+
override StringlikeLiteral valueLiteral;
168+
}
169+
170+
/**
171+
* A node that sets a Stringlike value, or `nil`.
172+
*/
173+
class NillableStringlikeSetting extends LiteralSetting {
174+
NillableStringlikeSetting() {
175+
valueLiteral instanceof StringlikeLiteral or
176+
valueLiteral instanceof NilLiteral
177+
}
178+
179+
string getStringValue() { result = valueLiteral.(StringlikeLiteral).getValueText() }
87180

88-
MethodCall getCall() { result = this.asExpr().getExpr() }
181+
predicate isNilValue() { valueLiteral instanceof NilLiteral }
182+
}
89183
}
90184

91-
private class ActionControllerConfigNode extends DataFlow::Node {
92-
ActionControllerConfigNode() {
93-
exists(CallAgainstConfig source | source.getCall().getMethodName() = "action_controller" |
94-
source.flowsTo(this)
95-
)
185+
/**
186+
* A `DataFlow::Node` that may enable or disable Rails CSRF protection in
187+
* production code.
188+
*/
189+
private class AllowForgeryProtectionSetting extends Settings::BooleanSetting,
190+
CSRFProtectionSetting::Range {
191+
AllowForgeryProtectionSetting() {
192+
this.getReceiver() instanceof Config::ActionControllerNode and
193+
this.getMethodName() = "allow_forgery_protection="
96194
}
195+
196+
override boolean getVerificationSetting() { result = this.getValue() }
97197
}
98198

99-
/** Holds if `node` can contain `value`. */
100-
private predicate hasBooleanValue(DataFlow::Node node, boolean value) {
101-
exists(DataFlow::LocalSourceNode literal |
102-
literal.asExpr().getExpr().(BooleanLiteral).getValue() = value and
103-
literal.flowsTo(node)
104-
)
199+
/**
200+
* Sets the cipher to be used for encrypted cookies. Defaults to "aes-256-gcm".
201+
* This can be set to any cipher supported by
202+
* https://ruby-doc.org/stdlib-2.7.1/libdoc/openssl/rdoc/OpenSSL/Cipher.html
203+
*/
204+
private class EncryptedCookieCipherSetting extends Settings::StringlikeSetting,
205+
CookieSecurityConfigurationSetting::Range {
206+
EncryptedCookieCipherSetting() {
207+
this.getReceiver() instanceof Config::ActionDispatchNode and
208+
this.getMethodName() = "encrypted_cookie_cipher="
209+
}
210+
211+
OpenSSLCipher getCipher() { this.getValueText() = result.getName() }
212+
213+
OpenSSLCipher getDefaultCipher() { result.getName() = "aes-256-gcm" }
214+
215+
override string getSecurityWarningMessage() {
216+
this.getCipher().isWeak() and
217+
result = this.getValueText() + " is a weak cipher."
218+
}
105219
}
106220

107-
// `<actionControllerConfig>.allow_forgery_protection = <verificationSetting>`
108-
private DataFlow::CallNode getAnAllowForgeryProtectionCall(boolean verificationSetting) {
109-
// exclude some test configuration
110-
not (
111-
result.getLocation().getFile().getRelativePath().matches("%test/%") or
112-
result.getLocation().getFile().getStem() = "test"
113-
) and
114-
result.getReceiver() instanceof ActionControllerConfigNode and
115-
result.asExpr().getExpr().(MethodCall).getMethodName() = "allow_forgery_protection=" and
116-
hasBooleanValue(result.getArgument(0), verificationSetting)
221+
/**
222+
* If true, signed and encrypted cookies will use the AES-256-GCM cipher rather
223+
* than the older AES-256-CBC cipher. Defaults to true.
224+
*/
225+
private class UseAuthenticatedCookieEncryptionSetting extends Settings::BooleanSetting,
226+
CookieSecurityConfigurationSetting::Range {
227+
UseAuthenticatedCookieEncryptionSetting() {
228+
this.getReceiver() instanceof Config::ActionDispatchNode and
229+
this.getMethodName() = "use_authenticated_cookie_encryption="
230+
}
231+
232+
boolean getDefaultValue() { result = true }
233+
234+
override string getSecurityWarningMessage() {
235+
this.getValue() = false and
236+
result = this.getSettingString() + " selects a weaker block mode for authenticated cookies."
237+
}
117238
}
118239

240+
// TODO: this may also take a proc that specifies how to handle specific requests
119241
/**
120-
* A `DataFlow::Node` that may enable or disable Rails CSRF protection in
121-
* production code.
242+
* Configures the default value of the `SameSite` attribute when setting cookies.
243+
* Valid string values are `strict`, `lax`, and `none`.
244+
* The attribute can be omitted by setting this to `nil`.
245+
* The default if unset is `:lax`.
246+
* https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite#strict
122247
*/
123-
private class AllowForgeryProtectionSetting extends CSRFProtectionSetting::Range {
124-
private boolean verificationSetting;
248+
private class CookiesSameSiteProtectionSetting extends Settings::NillableStringlikeSetting,
249+
CookieSecurityConfigurationSetting::Range {
250+
CookiesSameSiteProtectionSetting() {
251+
this.getReceiver() instanceof Config::ActionDispatchNode and
252+
this.getMethodName() = "cookies_same_site_protection="
253+
}
125254

126-
AllowForgeryProtectionSetting() { this = getAnAllowForgeryProtectionCall(verificationSetting) }
255+
string getDefaultValue() { result = "lax" }
127256

128-
override boolean getVerificationSetting() { result = verificationSetting }
257+
override string getSecurityWarningMessage() {
258+
// Mark unset as being potentially dangerous, as not all browsers default to "lax"
259+
this.getStringValue().toLowerCase() = "none" and
260+
result = "Setting 'SameSite' to 'None' may make an application more vulnerable to CSRF attacks."
261+
or
262+
this.isNilValue() and
263+
result = "Unsetting 'SameSite' can disable same-site cookie restrictions in some browsers."
264+
}
129265
}
130266
// TODO: initialization hooks, e.g. before_configuration, after_initialize...
131267
// TODO: initializers

‎ruby/ql/src/change-notes/2021-12-06-weak-cookie-configuration-query.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: newQuery
3+
---
4+
lgtm,codescanning
5+
* Added a new query, `rb/weak-cookie-configuration`. The query finds cases where cookie configuration options are set to values that may make an application more vulnerable to certain attacks.

‎ruby/ql/src/queries/security/cwe-732/WeakCookieConfiguration.qhelp‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Cookies can be used for security measures, such as authenticating a user
9+
based on cookies sent with a request. Misconfiguration of cookie settings
10+
in a web application can expose users to attacks that compromise these
11+
security measures.
12+
</p>
13+
</overview>
14+
15+
<recommendation>
16+
<p>
17+
Modern web frameworks typically have good default configuration for cookie
18+
settings. If an application overrides these settings, then take care to
19+
ensure that these changes are necessary and that they don't weaken the
20+
cookie configuration.
21+
</p>
22+
</recommendation>
23+
24+
<example>
25+
<p>
26+
In the first example, the value of
27+
<code>config.action_dispatch.cookies_same_site_protection</code> is set to
28+
<code>:none</code>. This has the effect of setting the default
29+
<code>SameSite</code> attribute sent by the server when setting a cookie
30+
to <code>None</code> rather than the default of <code>Lax</code>. This may
31+
make the application more vulnerable to cross-site request forgery
32+
attacks.
33+
</p>
34+
35+
<p>
36+
In the second example, this option is instead set to <code>:strict</code>.
37+
This is a stronger restriction than the default of <code>:lax</code>, and
38+
doesn't compromise on cookie security.
39+
</p>
40+
41+
<sample src="examples/weak_cookie_configuration.rb" />
42+
</example>
43+
44+
<references>
45+
<li>OWASP: <a href="https://owasp.org/www-community/SameSite">SameSite</a>.</li>
46+
<li>Rails: <a href="https://guides.rubyonrails.org/configuring.html#configuring-action-dispatch">Configuring Action Dispatch</a>.</li>
47+
</references>
48+
</qhelp>

0 commit comments

Comments
 (0)