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.

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
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
33 changes: 33 additions & 0 deletions ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll
Original file line number Diff line number Diff line change
Expand Up @@ -792,3 +792,36 @@ class ActiveRecordScopeCallTarget extends AdditionalCallTarget {
)
}
}

/** Sinks for the mass assignment query. */
private module MassAssignmentSinks {
private import codeql.ruby.security.MassAssignmentCustomizations

pragma[nomagic]
private predicate massAssignmentCall(DataFlow::CallNode call, string name) {
call = activeRecordBaseClass().getAMethodCall(name)
or
call instanceof ActiveRecordInstanceMethodCall and
call.getMethodName() = name
}

/** A call to a method that sets attributes of an database record using a hash. */
private class MassAssignmentSink extends MassAssignment::Sink {
MassAssignmentSink() {
exists(DataFlow::CallNode call, string name | massAssignmentCall(call, name) |
name =
[
"build", "create", "create!", "create_with", "create_or_find_by", "create_or_find_by!",
"find_or_create_by", "find_or_create_by!", "find_or_initialize_by", "insert", "insert!",
"insert_all", "insert_all!", "instantiate", "new", "update", "update!", "upsert",
"upsert_all"
] and
this = call.getArgument(0)
or
// These methods have an optional first id parameter.
name = ["update", "update!"] and
this = call.getArgument(1)
)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/**
* Provides default sources, sinks, sanitizers, and flow steps for
* detecting insecure mass assignment, as well as extension points for adding your own.
*/

private import codeql.ruby.AST
private import codeql.ruby.DataFlow
private import codeql.ruby.TaintTracking
private import codeql.ruby.dataflow.RemoteFlowSources

/**
* Provides default sources, sinks, sanitizers, and flow steps for
* detecting insecure mass assignment, as well as extension points for adding your own.
*/
module MassAssignment {
/**
* A data flow source for user input used for mass assignment.
*/
abstract class Source extends DataFlow::Node { }

/**
* A data flow sink for user input used for mass assignment.
*/
abstract class Sink extends DataFlow::Node { }

/**
* A sanitizer for insecure mass assignment.
*/
abstract class Sanitizer extends DataFlow::Node { }

/**
* A call that permits arbitrary parameters to be used for mass assignment.
*/
abstract class MassPermit extends DataFlow::Node {
/** Gets the argument for the parameters to be permitted. */
abstract DataFlow::Node getParamsArgument();

/** Gets the result node of the permitted parameters. */
abstract DataFlow::Node getPermittedParamsResult();
}

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.(DataFlow::CallNode).getMethodName() = "permit!" }

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

override DataFlow::Node getPermittedParamsResult() {
result = this
or
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 }
}
}
65 changes: 65 additions & 0 deletions ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/**
* Provides a taint tracking configuration for reasoning about insecure mass assignment.
*/

private import codeql.ruby.AST
private import codeql.ruby.DataFlow
private import codeql.ruby.TaintTracking
private import codeql.ruby.dataflow.RemoteFlowSources
private import MassAssignmentCustomizations

private module FlowState {
private newtype TState =
TUnpermitted() or
TPermitted()

/** A flow state used to distinguish whether arbitrary user parameters have been permitted to be used for mass assignment. */
class State extends TState {
string toString() {
this = TUnpermitted() and result = "unpermitted"
or
this = TPermitted() and result = "permitted"
}
}

/** A flow state used for user parameters for which arbitrary parameters have not been permitted to use for mass assignment. */
class Unpermitted extends State, TUnpermitted { }

/** A flow state used for user parameters for which arbitrary parameters have been permitted to use for mass assignment. */
class Permitted extends State, TPermitted { }
}

/** A flow configuration for reasoning about insecure mass assignment. */
private module Config implements DataFlow::StateConfigSig {
class FlowState = FlowState::State;

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

predicate isSink(DataFlow::Node node, FlowState state) {
node instanceof MassAssignment::Sink and
state instanceof FlowState::Permitted
}

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 }

predicate isAdditionalFlowStep(
DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
) {
exists(MassAssignment::MassPermit permit |
node1 = permit.getParamsArgument() and
state1 instanceof FlowState::Unpermitted and
node2 = permit.getPermittedParamsResult() and
state2 instanceof FlowState::Permitted
)
}
}

/** Taint tracking for reasoning about user input used for mass assignment. */
module MassAssignmentFlow = TaintTracking::GlobalWithState<Config>;
4 changes: 4 additions & 0 deletions ruby/ql/src/change-notes/2024-03-22-mass-assignment.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: newQuery
---
* Added a new query, `rb/insecure-mass-assignment`, for finding instances of mass assignment operations accepting arbitrary parameters from remote user input.
34 changes: 34 additions & 0 deletions ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
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
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.
</p>
</overview>
<recommendation>
<p>
When using a mass assignment operation from user supplied parameters, use <code>ActionController::Parameters#permit</code> to restrict the possible parameters
a user can supply, rather than <code>ActionController::Parameters#permit!</code>, which permits arbitrary parameters to be used for mass assignment.
</p>
</recommendation>
<example>
<p>
In the following example, <code>permit!</code> is used which allows arbitrary parameters to be supplied by the user.
</p>
<sample src="examples/MassAssignmentBad.rb" />
<p>

</p>
<p>
In the following example, only specific parameters are permitted, so the mass assignment is safe.
</p>
<sample src="examples/MassAssignmentGood.rb" />
</example>

<references>
joefarebrother marked this conversation as resolved.
Show resolved Hide resolved
<li>Rails guides: <a href="https://guides.rubyonrails.org/action_controller_overview.html#strong-parameters">Strong Parameters</a>.</li>
</references>
</qhelp>
20 changes: 20 additions & 0 deletions ruby/ql/src/queries/security/cwe-915/MassAssignment.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* @name Insecure Mass Assignment
* @description Using mass assignment with user-controlled attributes allows unintended parameters to be set.
* @kind path-problem
* @problem.severity error
* @security-severity 9.8
* @precision high
* @id rb/insecure-mass-assignment
* @tags security
* external/cwe/cwe-915
*/

import codeql.ruby.security.MassAssignmentQuery
import MassAssignmentFlow::PathGraph

from MassAssignmentFlow::PathNode source, MassAssignmentFlow::PathNode sink
where MassAssignmentFlow::flowPath(source, sink)
select sink.getNode(), source, sink,
"This mass assignment operation can assign user-controlled attributes from $@.", source.getNode(),
"this remote flow source"
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
class UserController < ActionController::Base
def create
# BAD: arbitrary params are permitted to be used for this assignment
User.new(user_params).save!
end

def user_params
params.require(:user).permit!
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
class UserController < ActionController::Base
def create
# GOOD: the permitted parameters are explicitly specified
User.new(user_params).save!
end

def user_params
params.require(:user).permit(:name, :email)
end
end
Loading
Loading