Skip to content

Commit 5cebcad

Browse files
joefarebrotherjoefarebrother
authored
Merge pull request #15987 from joefarebrother/ruby-mass-reassignment
Ruby: Add query for insecure mass assignment
2 parents 854dfb3 + 06d7b3c commit 5cebcad

11 files changed

Lines changed: 415 additions & 0 deletions

File tree

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -792,3 +792,36 @@ class ActiveRecordScopeCallTarget extends AdditionalCallTarget {
792792
)
793793
}
794794
}
795+
796+
/** Sinks for the mass assignment query. */
797+
private module MassAssignmentSinks {
798+
private import codeql.ruby.security.MassAssignmentCustomizations
799+
800+
pragma[nomagic]
801+
private predicate massAssignmentCall(DataFlow::CallNode call, string name) {
802+
call = activeRecordBaseClass().getAMethodCall(name)
803+
or
804+
call instanceof ActiveRecordInstanceMethodCall and
805+
call.getMethodName() = name
806+
}
807+
808+
/** A call to a method that sets attributes of an database record using a hash. */
809+
private class MassAssignmentSink extends MassAssignment::Sink {
810+
MassAssignmentSink() {
811+
exists(DataFlow::CallNode call, string name | massAssignmentCall(call, name) |
812+
name =
813+
[
814+
"build", "create", "create!", "create_with", "create_or_find_by", "create_or_find_by!",
815+
"find_or_create_by", "find_or_create_by!", "find_or_initialize_by", "insert", "insert!",
816+
"insert_all", "insert_all!", "instantiate", "new", "update", "update!", "upsert",
817+
"upsert_all"
818+
] and
819+
this = call.getArgument(0)
820+
or
821+
// These methods have an optional first id parameter.
822+
name = ["update", "update!"] and
823+
this = call.getArgument(1)
824+
)
825+
}
826+
}
827+
}

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

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/**
2+
* Provides default sources, sinks, sanitizers, and flow steps for
3+
* detecting insecure mass assignment, as well as extension points for adding your own.
4+
*/
5+
6+
private import codeql.ruby.AST
7+
private import codeql.ruby.controlflow.CfgNodes
8+
private import codeql.ruby.DataFlow
9+
private import codeql.ruby.TaintTracking
10+
private import codeql.ruby.dataflow.RemoteFlowSources
11+
12+
/**
13+
* Provides default sources, sinks, sanitizers, and flow steps for
14+
* detecting insecure mass assignment, as well as extension points for adding your own.
15+
*/
16+
module MassAssignment {
17+
/**
18+
* A data flow source for user input used for mass assignment.
19+
*/
20+
abstract class Source extends DataFlow::Node { }
21+
22+
/**
23+
* A data flow sink for user input used for mass assignment.
24+
*/
25+
abstract class Sink extends DataFlow::Node { }
26+
27+
/**
28+
* A sanitizer for insecure mass assignment.
29+
*/
30+
abstract class Sanitizer extends DataFlow::Node { }
31+
32+
/**
33+
* A call that permits arbitrary parameters to be used for mass assignment.
34+
*/
35+
abstract class MassPermit extends DataFlow::Node {
36+
/** Gets the argument for the parameters to be permitted. */
37+
abstract DataFlow::Node getParamsArgument();
38+
39+
/** Gets the result node of the permitted parameters. */
40+
abstract DataFlow::Node getPermittedParamsResult();
41+
}
42+
43+
private class RemoteSource extends Source instanceof RemoteFlowSource { }
44+
45+
/** A call to `permit!`, which permits each key of its receiver. */
46+
private class PermitBangCall extends MassPermit instanceof DataFlow::CallNode {
47+
PermitBangCall() { this.(DataFlow::CallNode).getMethodName() = "permit!" }
48+
49+
override DataFlow::Node getParamsArgument() { result = this.(DataFlow::CallNode).getReceiver() }
50+
51+
override DataFlow::Node getPermittedParamsResult() {
52+
result = this
53+
or
54+
result.(DataFlow::PostUpdateNode).getPreUpdateNode() = this.getParamsArgument()
55+
}
56+
}
57+
58+
/** Holds if `h` is an empty hash or contains an empty hash at one if its (possibly nested) values. */
59+
private predicate hasEmptyHash(ExprCfgNode e) {
60+
e instanceof ExprNodes::HashLiteralCfgNode and
61+
not exists(e.(ExprNodes::HashLiteralCfgNode).getAKeyValuePair())
62+
or
63+
hasEmptyHash(e.(ExprNodes::HashLiteralCfgNode).getAKeyValuePair().getValue())
64+
or
65+
hasEmptyHash(e.(ExprNodes::PairCfgNode).getValue())
66+
or
67+
hasEmptyHash(e.(ExprNodes::ArrayLiteralCfgNode).getAnArgument())
68+
}
69+
70+
/** A call to `permit` that fully specifies the permitted parameters. */
71+
private class PermitCallSanitizer extends Sanitizer, DataFlow::CallNode {
72+
PermitCallSanitizer() {
73+
this.getMethodName() = "permit" and
74+
not hasEmptyHash(this.getArgument(_).getExprNode())
75+
}
76+
}
77+
78+
/** A call to `permit` that uses an empty hash, which allows arbitrary keys to be specified. */
79+
private class PermitCallMassPermit extends MassPermit instanceof DataFlow::CallNode {
80+
PermitCallMassPermit() {
81+
this.(DataFlow::CallNode).getMethodName() = "permit" and
82+
hasEmptyHash(this.(DataFlow::CallNode).getArgument(_).getExprNode())
83+
}
84+
85+
override DataFlow::Node getParamsArgument() { result = this.(DataFlow::CallNode).getReceiver() }
86+
87+
override DataFlow::Node getPermittedParamsResult() { result = this }
88+
}
89+
90+
/** A call to `to_unsafe_h`, which allows arbitrary parameter. */
91+
private class ToUnsafeHashCall extends MassPermit instanceof DataFlow::CallNode {
92+
ToUnsafeHashCall() { this.(DataFlow::CallNode).getMethodName() = "to_unsafe_h" }
93+
94+
override DataFlow::Node getParamsArgument() { result = this.(DataFlow::CallNode).getReceiver() }
95+
96+
override DataFlow::Node getPermittedParamsResult() { result = this }
97+
}
98+
}

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about insecure mass assignment.
3+
*/
4+
5+
private import codeql.ruby.AST
6+
private import codeql.ruby.DataFlow
7+
private import codeql.ruby.TaintTracking
8+
private import codeql.ruby.dataflow.RemoteFlowSources
9+
private import MassAssignmentCustomizations
10+
11+
private module FlowState {
12+
private newtype TState =
13+
TUnpermitted() or
14+
TPermitted()
15+
16+
/** A flow state used to distinguish whether arbitrary user parameters have been permitted to be used for mass assignment. */
17+
class State extends TState {
18+
string toString() {
19+
this = TUnpermitted() and result = "unpermitted"
20+
or
21+
this = TPermitted() and result = "permitted"
22+
}
23+
}
24+
25+
/** A flow state used for user parameters for which arbitrary parameters have not been permitted to use for mass assignment. */
26+
class Unpermitted extends State, TUnpermitted { }
27+
28+
/** A flow state used for user parameters for which arbitrary parameters have been permitted to use for mass assignment. */
29+
class Permitted extends State, TPermitted { }
30+
}
31+
32+
/** A flow configuration for reasoning about insecure mass assignment. */
33+
private module Config implements DataFlow::StateConfigSig {
34+
class FlowState = FlowState::State;
35+
36+
predicate isSource(DataFlow::Node node, FlowState state) {
37+
node instanceof MassAssignment::Source and
38+
state instanceof FlowState::Unpermitted
39+
}
40+
41+
predicate isSink(DataFlow::Node node, FlowState state) {
42+
node instanceof MassAssignment::Sink and
43+
state instanceof FlowState::Permitted
44+
}
45+
46+
predicate isBarrierIn(DataFlow::Node node, FlowState state) { isSource(node, state) }
47+
48+
predicate isBarrierOut(DataFlow::Node node, FlowState state) { isSink(node, state) }
49+
50+
predicate isBarrier(DataFlow::Node node) { node instanceof MassAssignment::Sanitizer }
51+
52+
predicate isAdditionalFlowStep(
53+
DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
54+
) {
55+
exists(MassAssignment::MassPermit permit |
56+
node1 = permit.getParamsArgument() and
57+
state1 instanceof FlowState::Unpermitted and
58+
node2 = permit.getPermittedParamsResult() and
59+
state2 instanceof FlowState::Permitted
60+
)
61+
}
62+
}
63+
64+
/** Taint tracking for reasoning about user input used for mass assignment. */
65+
module MassAssignmentFlow = TaintTracking::GlobalWithState<Config>;

‎ruby/ql/src/change-notes/2024-03-22-mass-assignment.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/insecure-mass-assignment`, for finding instances of mass assignment operations accepting arbitrary parameters from remote user input.

‎ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Operations that allow for mass assignment (setting multiple attributes of an object using a hash), such as <code>ActiveRecord::Base.new</code>, should take care not to
8+
allow arbitrary parameters to be set by the user. Otherwise, unintended attributes may be set, such as an <code>is_admin</code> field for a <code>User</code> object.
9+
</p>
10+
</overview>
11+
<recommendation>
12+
<p>
13+
When using a mass assignment operation from user supplied parameters, use <code>ActionController::Parameters#permit</code> to restrict the possible parameters
14+
a user can supply, rather than <code>ActionController::Parameters#permit!</code>, which permits arbitrary parameters to be used for mass assignment.
15+
</p>
16+
</recommendation>
17+
<example>
18+
<p>
19+
In the following example, <code>permit!</code> is used which allows arbitrary parameters to be supplied by the user.
20+
</p>
21+
<sample src="examples/MassAssignmentBad.rb" />
22+
<p>
23+
24+
</p>
25+
<p>
26+
In the following example, only specific parameters are permitted, so the mass assignment is safe.
27+
</p>
28+
<sample src="examples/MassAssignmentGood.rb" />
29+
</example>
30+
31+
<references>
32+
<li>Rails guides: <a href="https://guides.rubyonrails.org/action_controller_overview.html#strong-parameters">Strong Parameters</a>.</li>
33+
</references>
34+
</qhelp>

‎ruby/ql/src/queries/security/cwe-915/MassAssignment.ql‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* @name Insecure Mass Assignment
3+
* @description Using mass assignment with user-controlled attributes allows unintended parameters to be set.
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @security-severity 9.8
7+
* @precision high
8+
* @id rb/insecure-mass-assignment
9+
* @tags security
10+
* external/cwe/cwe-915
11+
*/
12+
13+
import codeql.ruby.security.MassAssignmentQuery
14+
import MassAssignmentFlow::PathGraph
15+
16+
from MassAssignmentFlow::PathNode source, MassAssignmentFlow::PathNode sink
17+
where MassAssignmentFlow::flowPath(source, sink)
18+
select sink.getNode(), source, sink,
19+
"This mass assignment operation can assign user-controlled attributes from $@.", source.getNode(),
20+
"this remote flow source"

‎ruby/ql/src/queries/security/cwe-915/examples/MassAssignmentBad.rb‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
class UserController < ActionController::Base
2+
def create
3+
# BAD: arbitrary params are permitted to be used for this assignment
4+
User.new(user_params).save!
5+
end
6+
7+
def user_params
8+
params.require(:user).permit!
9+
end
10+
end

‎ruby/ql/src/queries/security/cwe-915/examples/MassAssignmentGood.rb‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
class UserController < ActionController::Base
2+
def create
3+
# GOOD: the permitted parameters are explicitly specified
4+
User.new(user_params).save!
5+
end
6+
7+
def user_params
8+
params.require(:user).permit(:name, :email)
9+
end
10+
end

0 commit comments

Comments
 (0)