Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ruby: Add query for insecure mass assignment #15987

Merged
merged 15 commits into from
Apr 12, 2024
Merged

Ruby: Add query for insecure mass assignment #15987

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0f45a53
Add mass assignment query
joefarebrother Mar 20, 2024
8983898
Add test cases
joefarebrother Mar 20, 2024
a8aac31
Add qhelp
joefarebrother Mar 21, 2024
507a610
Reorganise into Custimizations file + add some more sinks on ActiveRe…
joefarebrother Mar 22, 2024
b741453
Add test cases
joefarebrother Mar 22, 2024
01f7124
Add change note and update severity
joefarebrother Mar 22, 2024
a6ee19c
Fix query id
joefarebrother Mar 22, 2024
592acb9
Add missing .s to qldoc
joefarebrother Mar 22, 2024
fb19288
Address review comments - Fix docs typo and add a reference
joefarebrother Mar 25, 2024
3c96bf6
Fix bad join
hvitved Apr 3, 2024
c2d771b
Ruby: Reduce alerts produced by `MassAssignment.ql`
hvitved Apr 3, 2024
976ca48
Review suggestions - rename sink class and add barrier out
joefarebrother Apr 10, 2024
0a3d73d
Add flow steps and sanitizers for `permit` calls
joefarebrother Apr 10, 2024
ec973ac
Use not exists
joefarebrother Apr 11, 2024
06d7b3c
Use cfg nodes
joefarebrother Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,8 +806,8 @@ private module MassAssignmentSinks {
}

/** A call to a method that sets attributes of an database record using a hash. */
private class MassAssignmentCall extends MassAssignment::Sink {
MassAssignmentCall() {
private class MassAssignmentSink extends MassAssignment::Sink {
MassAssignmentSink() {
exists(DataFlow::CallNode call, string name | massAssignmentCall(call, name) |
name =
[
Expand Down
44 changes: 43 additions & 1 deletion ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,9 @@ module MassAssignment {

private class RemoteSource extends Source instanceof RemoteFlowSource { }

/** A call to `permit!`, which permits each key of its receiver. */
private class PermitBangCall extends MassPermit instanceof DataFlow::CallNode {
PermitBangCall() { this.asExpr().getExpr().(MethodCall).getMethodName() = "permit!" }
PermitBangCall() { this.(DataFlow::CallNode).getMethodName() = "permit!" }

override DataFlow::Node getParamsArgument() { result = this.(DataFlow::CallNode).getReceiver() }

Expand All @@ -52,4 +53,45 @@ module MassAssignment {
result.(DataFlow::PostUpdateNode).getPreUpdateNode() = this.getParamsArgument()
}
}

/** Holds if `h` is an empty hash or contains an empty hash at one if its (possibly nested) values. */
private predicate hasEmptyHash(Expr e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use CFG nodes instead of AST nodes here. I.e., use HashLiteralCfgNode instead of HashLiteral, etc.

e instanceof HashLiteral and
not exists(e.(HashLiteral).getAKeyValuePair())
or
hasEmptyHash(e.(HashLiteral).getAKeyValuePair().getValue())
or
hasEmptyHash(e.(Pair).getValue())
or
hasEmptyHash(e.(ArrayLiteral).getAnElement())
}

/** A call to `permit` that fully specifies the permitted parameters. */
private class PermitCallSanitizer extends Sanitizer, DataFlow::CallNode {
PermitCallSanitizer() {
this.getMethodName() = "permit" and
not hasEmptyHash(this.getArgument(_).asExpr().getExpr())
}
}

/** A call to `permit` that uses an empty hash, which allows arbitrary keys to be specified. */
private class PermitCallMassPermit extends MassPermit instanceof DataFlow::CallNode {
PermitCallMassPermit() {
this.(DataFlow::CallNode).getMethodName() = "permit" and
hasEmptyHash(this.(DataFlow::CallNode).getArgument(_).asExpr().getExpr())
}

override DataFlow::Node getParamsArgument() { result = this.(DataFlow::CallNode).getReceiver() }

override DataFlow::Node getPermittedParamsResult() { result = this }
}

/** A call to `to_unsafe_h`, which allows arbitrary parameter. */
private class ToUnsafeHashCall extends MassPermit instanceof DataFlow::CallNode {
ToUnsafeHashCall() { this.(DataFlow::CallNode).getMethodName() = "to_unsafe_h" }

override DataFlow::Node getParamsArgument() { result = this.(DataFlow::CallNode).getReceiver() }

override DataFlow::Node getPermittedParamsResult() { result = this }
}
}
7 changes: 3 additions & 4 deletions ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,9 @@ private module Config implements DataFlow::StateConfigSig {
state instanceof FlowState::Permitted
}

predicate isBarrierIn(DataFlow::Node node, FlowState state) {
node instanceof MassAssignment::Source and
state instanceof FlowState::Unpermitted
}
predicate isBarrierIn(DataFlow::Node node, FlowState state) { isSource(node, state) }

predicate isBarrierOut(DataFlow::Node node, FlowState state) { isSink(node, state) }

predicate isBarrier(DataFlow::Node node) { node instanceof MassAssignment::Sanitizer }

Expand Down
45 changes: 24 additions & 21 deletions ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,4 @@
edges
| test.rb:23:25:23:37 | call to [] [element 0] | test.rb:23:25:23:37 | call to [] | provenance | |
| test.rb:23:26:23:36 | call to user_params | test.rb:23:25:23:37 | call to [] [element 0] | provenance | |
| test.rb:24:26:24:38 | call to [] [element 0] | test.rb:24:26:24:38 | call to [] | provenance | |
| test.rb:24:27:24:37 | call to user_params | test.rb:24:26:24:38 | call to [] [element 0] | provenance | |
| test.rb:30:21:30:33 | call to [] [element 0] | test.rb:30:21:30:33 | call to [] | provenance | |
| test.rb:30:22:30:32 | call to user_params | test.rb:30:21:30:33 | call to [] [element 0] | provenance | |
| test.rb:43:9:43:14 | call to params | test.rb:43:9:43:29 | call to require | provenance | |
| test.rb:43:9:43:29 | call to require | test.rb:43:9:43:37 | call to permit! | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:8:18:8:28 | call to user_params | provenance | |
Expand All @@ -13,41 +7,37 @@ edges
| test.rb:43:9:43:37 | call to permit! | test.rb:20:22:20:32 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:21:21:21:31 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:22:22:22:32 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:23:26:23:36 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:24:27:24:37 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:25:21:25:31 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:26:24:26:34 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:27:22:27:32 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:28:25:28:35 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:29:21:29:31 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:30:22:30:32 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:31:32:31:42 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:32:33:32:43 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:33:36:33:46 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:34:32:34:42 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:35:33:35:43 | call to user_params | provenance | |
| test.rb:43:9:43:37 | call to permit! | test.rb:36:26:36:36 | call to user_params | provenance | |
| test.rb:47:9:47:9 | x | test.rb:48:9:48:9 | x | provenance | |
| test.rb:47:13:47:18 | call to params | test.rb:47:13:47:25 | ...[...] | provenance | |
| test.rb:47:13:47:25 | ...[...] | test.rb:47:9:47:9 | x | provenance | |
| test.rb:48:9:48:9 | [post] x | test.rb:49:18:49:18 | x | provenance | |
| test.rb:48:9:48:9 | x | test.rb:48:9:48:9 | [post] x | provenance | |
| test.rb:51:18:51:23 | call to params | test.rb:51:18:51:40 | call to permit | provenance | |
| test.rb:52:18:52:23 | call to params | test.rb:52:18:52:69 | call to permit | provenance | |
| test.rb:53:18:53:23 | call to params | test.rb:53:18:53:35 | call to to_unsafe_h | provenance | |
nodes
| test.rb:8:18:8:28 | call to user_params | semmle.label | call to user_params |
| test.rb:18:20:18:30 | call to user_params | semmle.label | call to user_params |
| test.rb:19:21:19:31 | call to user_params | semmle.label | call to user_params |
| test.rb:20:22:20:32 | call to user_params | semmle.label | call to user_params |
| test.rb:21:21:21:31 | call to user_params | semmle.label | call to user_params |
| test.rb:22:22:22:32 | call to user_params | semmle.label | call to user_params |
| test.rb:23:25:23:37 | call to [] | semmle.label | call to [] |
| test.rb:23:25:23:37 | call to [] [element 0] | semmle.label | call to [] [element 0] |
| test.rb:23:26:23:36 | call to user_params | semmle.label | call to user_params |
| test.rb:24:26:24:38 | call to [] | semmle.label | call to [] |
| test.rb:24:26:24:38 | call to [] [element 0] | semmle.label | call to [] [element 0] |
| test.rb:24:27:24:37 | call to user_params | semmle.label | call to user_params |
| test.rb:25:21:25:31 | call to user_params | semmle.label | call to user_params |
| test.rb:26:24:26:34 | call to user_params | semmle.label | call to user_params |
| test.rb:27:22:27:32 | call to user_params | semmle.label | call to user_params |
| test.rb:28:25:28:35 | call to user_params | semmle.label | call to user_params |
| test.rb:29:21:29:31 | call to user_params | semmle.label | call to user_params |
| test.rb:30:21:30:33 | call to [] | semmle.label | call to [] |
| test.rb:30:21:30:33 | call to [] [element 0] | semmle.label | call to [] [element 0] |
| test.rb:30:22:30:32 | call to user_params | semmle.label | call to user_params |
| test.rb:31:32:31:42 | call to user_params | semmle.label | call to user_params |
| test.rb:32:33:32:43 | call to user_params | semmle.label | call to user_params |
| test.rb:33:36:33:46 | call to user_params | semmle.label | call to user_params |
Expand All @@ -57,6 +47,18 @@ nodes
| test.rb:43:9:43:14 | call to params | semmle.label | call to params |
| test.rb:43:9:43:29 | call to require | semmle.label | call to require |
| test.rb:43:9:43:37 | call to permit! | semmle.label | call to permit! |
| test.rb:47:9:47:9 | x | semmle.label | x |
| test.rb:47:13:47:18 | call to params | semmle.label | call to params |
| test.rb:47:13:47:25 | ...[...] | semmle.label | ...[...] |
| test.rb:48:9:48:9 | [post] x | semmle.label | [post] x |
| test.rb:48:9:48:9 | x | semmle.label | x |
| test.rb:49:18:49:18 | x | semmle.label | x |
| test.rb:51:18:51:23 | call to params | semmle.label | call to params |
| test.rb:51:18:51:40 | call to permit | semmle.label | call to permit |
| test.rb:52:18:52:23 | call to params | semmle.label | call to params |
| test.rb:52:18:52:69 | call to permit | semmle.label | call to permit |
| test.rb:53:18:53:23 | call to params | semmle.label | call to params |
| test.rb:53:18:53:35 | call to to_unsafe_h | semmle.label | call to to_unsafe_h |
subpaths
#select
| test.rb:8:18:8:28 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:8:18:8:28 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
Expand All @@ -65,17 +67,18 @@ subpaths
| test.rb:20:22:20:32 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:20:22:20:32 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:21:21:21:31 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:21:21:21:31 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:22:22:22:32 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:22:22:22:32 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:23:25:23:37 | call to [] | test.rb:43:9:43:14 | call to params | test.rb:23:25:23:37 | call to [] | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:24:26:24:38 | call to [] | test.rb:43:9:43:14 | call to params | test.rb:24:26:24:38 | call to [] | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:25:21:25:31 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:25:21:25:31 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:26:24:26:34 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:26:24:26:34 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:27:22:27:32 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:27:22:27:32 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:28:25:28:35 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:28:25:28:35 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:29:21:29:31 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:29:21:29:31 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:30:21:30:33 | call to [] | test.rb:43:9:43:14 | call to params | test.rb:30:21:30:33 | call to [] | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:31:32:31:42 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:31:32:31:42 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:32:33:32:43 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:32:33:32:43 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:33:36:33:46 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:33:36:33:46 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:34:32:34:42 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:34:32:34:42 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:35:33:35:43 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:35:33:35:43 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:36:26:36:36 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:36:26:36:36 | call to user_params | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:43:9:43:14 | call to params | this remote flow source |
| test.rb:49:18:49:18 | x | test.rb:47:13:47:18 | call to params | test.rb:49:18:49:18 | x | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:47:13:47:18 | call to params | this remote flow source |
| test.rb:51:18:51:40 | call to permit | test.rb:51:18:51:23 | call to params | test.rb:51:18:51:40 | call to permit | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:51:18:51:23 | call to params | this remote flow source |
| test.rb:52:18:52:69 | call to permit | test.rb:52:18:52:23 | call to params | test.rb:52:18:52:69 | call to permit | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:52:18:52:23 | call to params | this remote flow source |
| test.rb:53:18:53:35 | call to to_unsafe_h | test.rb:53:18:53:23 | call to params | test.rb:53:18:53:35 | call to to_unsafe_h | This mass assignment operation can assign user-controlled attributes from $@. | test.rb:53:18:53:23 | call to params | this remote flow source |
11 changes: 11 additions & 0 deletions ruby/ql/test/query-tests/security/cwe-915/test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,15 @@ def create3
def user_params
params.require(:user).permit!
end

def create4
x = params[:user]
x.permit!
User.new(x) # BAD
User.new(x.permit(:name,:address)) # GOOD
User.new(params.permit(user: {})) # BAD
User.new(params.permit(user: [:name, :address, {friends:{}}])) # BAD
User.new(params.to_unsafe_h) # BAD
User.new(params.permit(user: [:name, :address]).to_unsafe_h) # GOOD
end
end
Loading