Merge branch 'main' into post-release-prep/codeql-cli-2.7.5

This commit is contained in:
Andrew Eisenberg
2022-01-14 08:23:43 -08:00
744 changed files with 39341 additions and 19067 deletions

View File

@@ -645,6 +645,37 @@ module Path {
}
}
/**
* A data-flow node that may configure behavior relating to cookie security.
*
* Extend this class to refine existing API models. If you want to model new APIs,
* extend `CookieSecurityConfigurationSetting::Range` instead.
*/
class CookieSecurityConfigurationSetting extends DataFlow::Node instanceof CookieSecurityConfigurationSetting::Range {
/**
* Gets a description of how this cookie setting may weaken application security.
* This predicate has no results if the setting is considered to be safe.
*/
string getSecurityWarningMessage() { result = super.getSecurityWarningMessage() }
}
/** Provides a class for modeling new cookie security setting APIs. */
module CookieSecurityConfigurationSetting {
/**
* A data-flow node that may configure behavior relating to cookie security.
*
* Extend this class to model new APIs. If you want to refine existing API models,
* extend `CookieSecurityConfigurationSetting` instead.
*/
abstract class Range extends DataFlow::Node {
/**
* Gets a description of how this cookie setting may weaken application security.
* This predicate has no results if the setting is considered to be safe.
*/
abstract string getSecurityWarningMessage();
}
}
/**
* A data-flow node that logs data.
*

File diff suppressed because it is too large Load Diff

File diff suppressed because it is too large Load Diff

View File

@@ -3,6 +3,17 @@ private import DataFlowImplSpecific::Public
import Cached
module DataFlowImplCommonPublic {
/** A state value to track during data flow. */
class FlowState = string;
/**
* The default state, which is used when the state is unspecified for a source
* or a sink.
*/
class FlowStateEmpty extends FlowState {
FlowStateEmpty() { this = "" }
}
private newtype TFlowFeature =
TFeatureHasSourceCallContext() or
TFeatureHasSinkCallContext() or

View File

@@ -748,14 +748,16 @@ predicate clearsContent(Node n, Content c) {
FlowSummaryImpl::Private::Steps::summaryClearsContent(n, c)
}
private newtype TDataFlowType = TTodoDataFlowType()
private newtype TDataFlowType =
TTodoDataFlowType() or
TTodoDataFlowType2() // Add a dummy value to prevent bad functionality-induced joins arising from a type of size 1.
class DataFlowType extends TDataFlowType {
string toString() { result = "" }
}
/** Gets the type of `n` used for type pruning. */
DataFlowType getNodeType(NodeImpl n) { any() }
DataFlowType getNodeType(NodeImpl n) { result = TTodoDataFlowType() and exists(n) }
/** Gets a string representation of a `DataFlowType`. */
string ppReprType(DataFlowType t) { result = t.toString() }

View File

@@ -659,4 +659,15 @@ module Consistency {
not phiHasInputFromBlock(_, def, _) and
not uncertainWriteDefinitionInput(_, def)
}
query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) |
ssaDefReachesReadWithinBlock(v, def, bb, i) and
(bb != bbDef or i < iDef)
or
ssaDefReachesRead(v, def, bb, i) and
not ssaDefReachesReadWithinBlock(v, def, bb, i) and
not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _)
)
}
}

View File

@@ -11,6 +11,12 @@ private import FlowSummaryImpl as FlowSummaryImpl
*/
predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
/**
* Holds if `guard` should be a sanitizer guard in all global taint flow configurations
* but not in local taint.
*/
predicate defaultTaintSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
/**
* Holds if default `TaintTracking::Configuration`s should allow implicit reads
* of `c` at sinks and inputs to additional taint steps.

View File

@@ -61,7 +61,7 @@ abstract class Configuration extends DataFlow::Configuration {
* The smaller this predicate is, the faster `hasFlow()` will converge.
*/
// overridden to provide taint-tracking specific qldoc
abstract override predicate isSource(DataFlow::Node source);
override predicate isSource(DataFlow::Node source) { none() }
/**
* Holds if `sink` is a relevant taint sink.
@@ -69,7 +69,7 @@ abstract class Configuration extends DataFlow::Configuration {
* The smaller this predicate is, the faster `hasFlow()` will converge.
*/
// overridden to provide taint-tracking specific qldoc
abstract override predicate isSink(DataFlow::Node sink);
override predicate isSink(DataFlow::Node sink) { none() }
/** Holds if the node `node` is a taint sanitizer. */
predicate isSanitizer(DataFlow::Node node) { none() }
@@ -93,7 +93,7 @@ abstract class Configuration extends DataFlow::Configuration {
predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
this.isSanitizerGuard(guard)
this.isSanitizerGuard(guard) or defaultTaintSanitizerGuard(guard)
}
/**

View File

@@ -12,6 +12,7 @@ private import codeql.ruby.frameworks.ActiveRecord
private import codeql.ruby.frameworks.ActiveStorage
private import codeql.ruby.ast.internal.Module
private import codeql.ruby.ApiGraphs
private import codeql.ruby.security.OpenSSL
/**
* A reference to either `Rails::Railtie`, `Rails::Engine`, or `Rails::Application`.
@@ -47,85 +48,220 @@ private DataFlow::CallNode getAConfigureCallNode() {
}
/**
* An access to a Rails config object.
* Classes representing accesses to the Rails config object.
*/
private class ConfigSourceNode extends DataFlow::LocalSourceNode {
ConfigSourceNode() {
// `Foo < Rails::Application ... config ...`
exists(MethodCall configCall | this.asExpr().getExpr() = configCall |
configCall.getMethodName() = "config" and
configCall.getEnclosingModule() instanceof RailtieClass
)
or
// `Rails.application.config`
this =
API::getTopLevelMember("Rails")
.getReturn("application")
.getReturn("config")
.getAnImmediateUse()
or
// `Rails.application.configure { ... config ... }`
// `Rails::Application.configure { ... config ... }`
exists(DataFlow::CallNode configureCallNode, Block block, MethodCall configCall |
configCall = this.asExpr().getExpr()
|
configureCallNode = getAConfigureCallNode() and
block = configureCallNode.getBlock().asExpr().getExpr() and
configCall.getParent+() = block and
configCall.getMethodName() = "config"
)
private module Config {
/**
* An access to a Rails config object.
*/
private class SourceNode extends DataFlow::LocalSourceNode {
SourceNode() {
// `Foo < Rails::Application ... config ...`
exists(MethodCall configCall | this.asExpr().getExpr() = configCall |
configCall.getMethodName() = "config" and
configCall.getEnclosingModule() instanceof RailtieClass
)
or
// `Rails.application.config`
this =
API::getTopLevelMember("Rails")
.getReturn("application")
.getReturn("config")
.getAnImmediateUse()
or
// `Rails.application.configure { ... config ... }`
// `Rails::Application.configure { ... config ... }`
exists(DataFlow::CallNode configureCallNode, Block block, MethodCall configCall |
configCall = this.asExpr().getExpr()
|
configureCallNode = getAConfigureCallNode() and
block = configureCallNode.asExpr().getExpr().(MethodCall).getBlock() and
configCall.getParent+() = block and
configCall.getMethodName() = "config"
)
}
}
/**
* A reference to the Rails config object.
*/
class Node extends DataFlow::Node {
Node() { exists(SourceNode src | src.flowsTo(this)) }
}
/**
* A reference to the ActionController config object.
*/
class ActionControllerNode extends DataFlow::Node {
ActionControllerNode() {
exists(DataFlow::CallNode source |
source.getReceiver() instanceof Node and
source.getMethodName() = "action_controller"
|
source.flowsTo(this)
)
}
}
/**
* A reference to the ActionDispatch config object.
*/
class ActionDispatchNode extends DataFlow::Node {
ActionDispatchNode() {
exists(DataFlow::CallNode source |
source.getReceiver() instanceof Node and
source.getMethodName() = "action_dispatch"
|
source.flowsTo(this)
)
}
}
}
private class ConfigNode extends DataFlow::Node {
ConfigNode() { exists(ConfigSourceNode src | src.flowsTo(this)) }
}
// A call where the Rails application config is the receiver
private class CallAgainstConfig extends DataFlow::CallNode {
CallAgainstConfig() { this.getReceiver() instanceof ConfigNode }
MethodCall getCall() { result = this.asExpr().getExpr() }
}
private class ActionControllerConfigNode extends DataFlow::Node {
ActionControllerConfigNode() {
exists(CallAgainstConfig source | source.getCall().getMethodName() = "action_controller" |
source.flowsTo(this)
)
/**
* Classes representing nodes that set a Rails configuration value.
*/
private module Settings {
private predicate isInTestConfiguration(Location loc) {
loc.getFile().getRelativePath().matches("%test/%") or
loc.getFile().getStem() = "test"
}
}
/** Holds if `node` can contain `value`. */
private predicate hasBooleanValue(DataFlow::Node node, boolean value) {
exists(DataFlow::LocalSourceNode literal |
literal.asExpr().getExpr().(BooleanLiteral).getValue() = value and
literal.flowsTo(node)
)
}
private class Setting extends DataFlow::CallNode {
Setting() {
// exclude some test configuration
not isInTestConfiguration(this.getLocation()) and
this.getReceiver+() instanceof Config::Node and
this.asExpr().getExpr() instanceof SetterMethodCall
}
}
// `<actionControllerConfig>.allow_forgery_protection = <verificationSetting>`
private DataFlow::CallNode getAnAllowForgeryProtectionCall(boolean verificationSetting) {
// exclude some test configuration
not (
result.getLocation().getFile().getRelativePath().matches("%test/%") or
result.getLocation().getFile().getStem() = "test"
) and
result.getReceiver() instanceof ActionControllerConfigNode and
result.asExpr().getExpr().(MethodCall).getMethodName() = "allow_forgery_protection=" and
hasBooleanValue(result.getArgument(0), verificationSetting)
private class LiteralSetting extends Setting {
Literal valueLiteral;
LiteralSetting() {
exists(DataFlow::LocalSourceNode lsn |
lsn.asExpr().getExpr() = valueLiteral and
lsn.flowsTo(this.getArgument(0))
)
}
string getValueText() { result = valueLiteral.getValueText() }
string getSettingString() { result = this.getMethodName() + this.getValueText() }
}
/**
* A node that sets a boolean value.
*/
class BooleanSetting extends LiteralSetting {
override BooleanLiteral valueLiteral;
boolean getValue() { result = valueLiteral.getValue() }
}
/**
* A node that sets a Stringlike value.
*/
class StringlikeSetting extends LiteralSetting {
override StringlikeLiteral valueLiteral;
}
/**
* A node that sets a Stringlike value, or `nil`.
*/
class NillableStringlikeSetting extends LiteralSetting {
NillableStringlikeSetting() {
valueLiteral instanceof StringlikeLiteral or
valueLiteral instanceof NilLiteral
}
string getStringValue() { result = valueLiteral.(StringlikeLiteral).getValueText() }
predicate isNilValue() { valueLiteral instanceof NilLiteral }
}
}
/**
* A `DataFlow::Node` that may enable or disable Rails CSRF protection in
* production code.
*/
private class AllowForgeryProtectionSetting extends CSRFProtectionSetting::Range {
private boolean verificationSetting;
private class AllowForgeryProtectionSetting extends Settings::BooleanSetting,
CSRFProtectionSetting::Range {
AllowForgeryProtectionSetting() {
this.getReceiver() instanceof Config::ActionControllerNode and
this.getMethodName() = "allow_forgery_protection="
}
AllowForgeryProtectionSetting() { this = getAnAllowForgeryProtectionCall(verificationSetting) }
override boolean getVerificationSetting() { result = this.getValue() }
}
override boolean getVerificationSetting() { result = verificationSetting }
/**
* Sets the cipher to be used for encrypted cookies. Defaults to "aes-256-gcm".
* This can be set to any cipher supported by
* https://ruby-doc.org/stdlib-2.7.1/libdoc/openssl/rdoc/OpenSSL/Cipher.html
*/
private class EncryptedCookieCipherSetting extends Settings::StringlikeSetting,
CookieSecurityConfigurationSetting::Range {
EncryptedCookieCipherSetting() {
this.getReceiver() instanceof Config::ActionDispatchNode and
this.getMethodName() = "encrypted_cookie_cipher="
}
OpenSSLCipher getCipher() { this.getValueText() = result.getName() }
OpenSSLCipher getDefaultCipher() { result.getName() = "aes-256-gcm" }
override string getSecurityWarningMessage() {
this.getCipher().isWeak() and
result = this.getValueText() + " is a weak cipher."
}
}
/**
* If true, signed and encrypted cookies will use the AES-256-GCM cipher rather
* than the older AES-256-CBC cipher. Defaults to true.
*/
private class UseAuthenticatedCookieEncryptionSetting extends Settings::BooleanSetting,
CookieSecurityConfigurationSetting::Range {
UseAuthenticatedCookieEncryptionSetting() {
this.getReceiver() instanceof Config::ActionDispatchNode and
this.getMethodName() = "use_authenticated_cookie_encryption="
}
boolean getDefaultValue() { result = true }
override string getSecurityWarningMessage() {
this.getValue() = false and
result = this.getSettingString() + " selects a weaker block mode for authenticated cookies."
}
}
// TODO: this may also take a proc that specifies how to handle specific requests
/**
* Configures the default value of the `SameSite` attribute when setting cookies.
* Valid string values are `strict`, `lax`, and `none`.
* The attribute can be omitted by setting this to `nil`.
* The default if unset is `:lax`.
* https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite#strict
*/
private class CookiesSameSiteProtectionSetting extends Settings::NillableStringlikeSetting,
CookieSecurityConfigurationSetting::Range {
CookiesSameSiteProtectionSetting() {
this.getReceiver() instanceof Config::ActionDispatchNode and
this.getMethodName() = "cookies_same_site_protection="
}
string getDefaultValue() { result = "lax" }
override string getSecurityWarningMessage() {
// Mark unset as being potentially dangerous, as not all browsers default to "lax"
this.getStringValue().toLowerCase() = "none" and
result = "Setting 'SameSite' to 'None' may make an application more vulnerable to CSRF attacks."
or
this.isNilValue() and
result = "Unsetting 'SameSite' can disable same-site cookie restrictions in some browsers."
}
}
// TODO: initialization hooks, e.g. before_configuration, after_initialize...
// TODO: initializers

View File

@@ -539,6 +539,55 @@ private class EdgeLabel extends TInputSymbol {
}
}
/**
* A RegExp term that acts like a plus.
* Either it's a RegExpPlus, or it is a range {1,X} where X is >= 30.
* 30 has been chosen as a threshold because for exponential blowup 2^30 is enough to get a decent DOS attack.
*/
private class EffectivelyPlus extends RegExpTerm {
EffectivelyPlus() {
this instanceof RegExpPlus
or
exists(RegExpRange range |
range.getLowerBound() = 1 and
(range.getUpperBound() >= 30 or not exists(range.getUpperBound()))
|
this = range
)
}
}
/**
* A RegExp term that acts like a star.
* Either it's a RegExpStar, or it is a range {0,X} where X is >= 30.
*/
private class EffectivelyStar extends RegExpTerm {
EffectivelyStar() {
this instanceof RegExpStar
or
exists(RegExpRange range |
range.getLowerBound() = 0 and
(range.getUpperBound() >= 30 or not exists(range.getUpperBound()))
|
this = range
)
}
}
/**
* A RegExp term that acts like a question mark.
* Either it's a RegExpQuestion, or it is a range {0,1}.
*/
private class EffectivelyQuestion extends RegExpTerm {
EffectivelyQuestion() {
this instanceof RegExpOpt
or
exists(RegExpRange range | range.getLowerBound() = 0 and range.getUpperBound() = 1 |
this = range
)
}
}
/**
* Gets the state before matching `t`.
*/
@@ -559,14 +608,14 @@ State after(RegExpTerm t) {
or
exists(RegExpGroup grp | t = grp.getAChild() | result = after(grp))
or
exists(RegExpStar star | t = star.getAChild() | result = before(star))
exists(EffectivelyStar star | t = star.getAChild() | result = before(star))
or
exists(RegExpPlus plus | t = plus.getAChild() |
exists(EffectivelyPlus plus | t = plus.getAChild() |
result = before(plus) or
result = after(plus)
)
or
exists(RegExpOpt opt | t = opt.getAChild() | result = after(opt))
exists(EffectivelyQuestion opt | t = opt.getAChild() | result = after(opt))
or
exists(RegExpRoot root | t = root | result = AcceptAnySuffix(root))
}
@@ -617,15 +666,17 @@ predicate delta(State q1, EdgeLabel lbl, State q2) {
or
exists(RegExpGroup grp | lbl = Epsilon() | q1 = before(grp) and q2 = before(grp.getChild(0)))
or
exists(RegExpStar star | lbl = Epsilon() |
exists(EffectivelyStar star | lbl = Epsilon() |
q1 = before(star) and q2 = before(star.getChild(0))
or
q1 = before(star) and q2 = after(star)
)
or
exists(RegExpPlus plus | lbl = Epsilon() | q1 = before(plus) and q2 = before(plus.getChild(0)))
exists(EffectivelyPlus plus | lbl = Epsilon() |
q1 = before(plus) and q2 = before(plus.getChild(0))
)
or
exists(RegExpOpt opt | lbl = Epsilon() |
exists(EffectivelyQuestion opt | lbl = Epsilon() |
q1 = before(opt) and q2 = before(opt.getChild(0))
or
q1 = before(opt) and q2 = after(opt)

View File

@@ -0,0 +1,5 @@
---
category: newQuery
---
lgtm,codescanning
* 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.

View File

@@ -0,0 +1,48 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
Cookies can be used for security measures, such as authenticating a user
based on cookies sent with a request. Misconfiguration of cookie settings
in a web application can expose users to attacks that compromise these
security measures.
</p>
</overview>
<recommendation>
<p>
Modern web frameworks typically have good default configuration for cookie
settings. If an application overrides these settings, then take care to
ensure that these changes are necessary and that they don't weaken the
cookie configuration.
</p>
</recommendation>
<example>
<p>
In the first example, the value of
<code>config.action_dispatch.cookies_same_site_protection</code> is set to
<code>:none</code>. This has the effect of setting the default
<code>SameSite</code> attribute sent by the server when setting a cookie
to <code>None</code> rather than the default of <code>Lax</code>. This may
make the application more vulnerable to cross-site request forgery
attacks.
</p>
<p>
In the second example, this option is instead set to <code>:strict</code>.
This is a stronger restriction than the default of <code>:lax</code>, and
doesn't compromise on cookie security.
</p>
<sample src="examples/weak_cookie_configuration.rb" />
</example>
<references>
<li>OWASP: <a href="https://owasp.org/www-community/SameSite">SameSite</a>.</li>
<li>Rails: <a href="https://guides.rubyonrails.org/configuring.html#configuring-action-dispatch">Configuring Action Dispatch</a>.</li>
</references>
</qhelp>

View File

@@ -0,0 +1,19 @@
/**
* @name Weak cookie configuration
* @description Misconfiguring how cookies are encrypted or sent can expose a user to various attacks.
* @kind problem
* @problem.severity warning
* @security-severity 7.8
* @id rb/weak-cookie-configuration
* @tags external/cwe/cwe-732
* external/cwe/cwe-1275
* security
* @precision high
*/
import ruby
import codeql.ruby.Concepts
import codeql.ruby.Frameworks
from CookieSecurityConfigurationSetting s
select s, s.getSecurityWarningMessage()

View File

@@ -0,0 +1,9 @@
module App
class Application < Rails::Application
# Sets default `Set-Cookie` `SameSite` attribute to `None`
config.action_dispatch.cookies_same_site_protection = :none
# Sets default `Set-Cookie` `SameSite` attribute to `Strict`
config.action_dispatch.cookies_same_site_protection = :strict
end
end

View File

@@ -362,11 +362,11 @@ bad84 = /^((?:a{0|-)|\w\{\d)+X$/
bad85 = /^((?:a{0,|-)|\w\{\d,)+X$/
bad86 = /^((?:a{0,2|-)|\w\{\d,\d)+X$/
# GOOD:
good42 = /^((?:a{0,2}|-)|\w\{\d,\d\})+X$/
# NOT GOOD
bad87 = /^((?:a{0,2}|-)|\w\{\d,\d\})+X$/
# NOT GOOD
bad87 = /^X(\u0061|a)*Y$/
bad88 = /^X(\u0061|a)*Y$/
# GOOD
good43 = /^X(\u0061|b)+Y$/

View File

@@ -0,0 +1,5 @@
| app/config/application.rb:14:5:14:50 | call to encrypted_cookie_cipher= | DES is a weak cipher. |
| app/config/application.rb:17:5:17:50 | call to encrypted_cookie_cipher= | AES-256-ECB is a weak cipher. |
| app/config/application.rb:23:5:23:62 | call to use_authenticated_cookie_encryption= | use_authenticated_cookie_encryption=false selects a weaker block mode for authenticated cookies. |
| app/config/application.rb:32:5:32:55 | call to cookies_same_site_protection= | Setting 'SameSite' to 'None' may make an application more vulnerable to CSRF attacks. |
| app/config/application.rb:35:5:35:55 | call to cookies_same_site_protection= | Unsetting 'SameSite' can disable same-site cookie restrictions in some browsers. |

View File

@@ -0,0 +1 @@
queries/security/cwe-732/WeakCookieConfiguration.ql

View File

@@ -0,0 +1,37 @@
require 'rails'
module App
class Application < Rails::Application
config.load_defaults 6.0
# GOOD: strong cipher
config.action_dispatch.encrypted_cookie_cipher = "aes-256-gcm"
# GOOD: strong cipher
config.action_dispatch.encrypted_cookie_cipher = "ChaCha"
# BAD: weak block encryption algorithm
config.action_dispatch.encrypted_cookie_cipher = "DES"
# BAD: weak block encryption mode
config.action_dispatch.encrypted_cookie_cipher = "AES-256-ECB"
# GOOD
config.action_dispatch.use_authenticated_cookie_encryption = true
# BAD: less secure block encryption mode
config.action_dispatch.use_authenticated_cookie_encryption = false
# GOOD
config.action_dispatch.cookies_same_site_protection = :lax
# GOOD
config.action_dispatch.cookies_same_site_protection = "strict"
# BAD: disabling same-site protections for sending cookies
config.action_dispatch.cookies_same_site_protection = :none
# BAD: not all browsers default to `lax` if unset
config.action_dispatch.cookies_same_site_protection = nil
end
end