From 0f45a53adccb3cfb9a134ec0e19f196b71b9996e Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Wed, 20 Mar 2024 10:24:23 +0000 Subject: [PATCH 01/15] Add mass assignment query --- .../ruby/security/MassAssignmentQuery.qll | 104 ++++++++++++++++++ .../security/cwe-915/MassAssignment.ql | 18 +++ 2 files changed, 122 insertions(+) create mode 100644 ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll create mode 100644 ruby/ql/src/queries/security/cwe-915/MassAssignment.ql diff --git a/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll b/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll new file mode 100644 index 000000000000..fc145f0d6e95 --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll @@ -0,0 +1,104 @@ +/** + * 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 + +/** Provides default sources and sinks for the mass assignment query. */ +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 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 { } + + private class CreateSink extends Sink { + CreateSink() { + exists(DataFlow::CallNode create | + create.asExpr().getExpr().(MethodCall).getMethodName() = ["create", "new", "update"] and + this = create.getArgument(0) + ) + } + } + + private class PermitCall extends MassPermit instanceof DataFlow::CallNode { + PermitCall() { this.asExpr().getExpr().(MethodCall).getMethodName() = "permit!" } + + override DataFlow::Node getParamsArgument() { result = this.(DataFlow::CallNode).getReceiver() } + + override DataFlow::Node getPermittedParamsResult() { result = this } + } +} + +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 + // or + // node = any(MassAssignment::MassPermit p).getPermittedParamsResult() and + // state instanceof FlowState::Permitted + } + + predicate isSink(DataFlow::Node node, FlowState state) { + node instanceof MassAssignment::Sink and + state instanceof FlowState::Permitted + } + + 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; diff --git a/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql b/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql new file mode 100644 index 000000000000..951d1efaf560 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql @@ -0,0 +1,18 @@ +/** + * @name Insecure Mass Assignment + * @description Using mass assignment with user-controlled keys allows unintended parameters to be set. + * @kind path-problem + * @problem.severity error + * @security-severity 7.5 + * @precision high + * @id ruby/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, "mass assignment" From 89838981b74a5874f24357790c1b2fe9fde4de88 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Wed, 20 Mar 2024 17:11:40 +0000 Subject: [PATCH 02/15] Add test cases --- .../security/cwe-915/MassAssignment.expected | 12 ++++++++++++ .../security/cwe-915/MassAssignment.qlref | 1 + .../test/query-tests/security/cwe-915/test.rb | 19 +++++++++++++++++++ 3 files changed, 32 insertions(+) create mode 100644 ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected create mode 100644 ruby/ql/test/query-tests/security/cwe-915/MassAssignment.qlref create mode 100644 ruby/ql/test/query-tests/security/cwe-915/test.rb diff --git a/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected b/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected new file mode 100644 index 000000000000..3067ab1f48d4 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected @@ -0,0 +1,12 @@ +edges +| test.rb:17:9:17:14 | call to params | test.rb:17:9:17:29 | call to require | provenance | | +| test.rb:17:9:17:29 | call to require | test.rb:17:9:17:37 | call to permit! | provenance | | +| test.rb:17:9:17:37 | call to permit! | test.rb:8:18:8:28 | call to user_params | provenance | | +nodes +| test.rb:8:18:8:28 | call to user_params | semmle.label | call to user_params | +| test.rb:17:9:17:14 | call to params | semmle.label | call to params | +| test.rb:17:9:17:29 | call to require | semmle.label | call to require | +| test.rb:17:9:17:37 | call to permit! | semmle.label | call to permit! | +subpaths +#select +| test.rb:8:18:8:28 | call to user_params | test.rb:17:9:17:14 | call to params | test.rb:8:18:8:28 | call to user_params | mass assignment | diff --git a/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.qlref b/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.qlref new file mode 100644 index 000000000000..89dbc405a3ae --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.qlref @@ -0,0 +1 @@ +queries/security/cwe-915/MassAssignment.ql \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-915/test.rb b/ruby/ql/test/query-tests/security/cwe-915/test.rb new file mode 100644 index 000000000000..5dee931761dd --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-915/test.rb @@ -0,0 +1,19 @@ +class User < ApplicationRecord + +end + +class UserController < ActionController::Base + def create + # BAD: arbitrary params are permitted to be used for this assignment + User.new(user_params).save! + end + + def create2 + # GOOD: the permitted parameters are explicitly specified + User.new(params[:user].permit(:name,:address)) + end + + def user_params + params.require(:user).permit! + end +end \ No newline at end of file From a8aac318d08e127da9fc2a94e2848e3bf7decf7f Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Thu, 21 Mar 2024 22:43:48 +0000 Subject: [PATCH 03/15] Add qhelp --- .../security/cwe-915/MassAssignment.qhelp | 34 +++++++++++++++++++ .../security/cwe-915/MassAssignment.ql | 6 ++-- .../cwe-915/examples/MassAssignmentBad.rb | 10 ++++++ .../cwe-915/examples/MassAssignmentGood.rb | 10 ++++++ 4 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp create mode 100644 ruby/ql/src/queries/security/cwe-915/examples/MassAssignmentBad.rb create mode 100644 ruby/ql/src/queries/security/cwe-915/examples/MassAssignmentGood.rb diff --git a/ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp b/ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp new file mode 100644 index 000000000000..7a96cd49b049 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp @@ -0,0 +1,34 @@ + + + +

+ Operations that allow for mass assignment (setting multiple attributes of an object using a hash), such as ActiveRecord::Base.new, should take care not to + allow arbitrary parameters to be set by the user. Otherwise, unintended attributes may be set, such as an isAdmin feild for a User object. +

+
+ +

+ When using a mass assignment operation from user supplied parameters, use ActionController::Parameters#permit to restrict the possible parameters + a user can supply, rather than ActionController::Parameters#permit!, which permits arbitrary parameters to be used for mass assignment. +

+
+ +

+ In the following example, permit! is used which allows arbitrary parameters to be supplied by the user. +

+ +

+ +

+

+ In the following example, only specific parameters are permitted, so the mass assignment is safe. +

+ +
+ + + + +
diff --git a/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql b/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql index 951d1efaf560..8803aad745a8 100644 --- a/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql +++ b/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql @@ -1,6 +1,6 @@ /** * @name Insecure Mass Assignment - * @description Using mass assignment with user-controlled keys allows unintended parameters to be set. + * @description Using mass assignment with user-controlled attributes allows unintended parameters to be set. * @kind path-problem * @problem.severity error * @security-severity 7.5 @@ -15,4 +15,6 @@ import MassAssignmentFlow::PathGraph from MassAssignmentFlow::PathNode source, MassAssignmentFlow::PathNode sink where MassAssignmentFlow::flowPath(source, sink) -select sink.getNode(), source, sink, "mass assignment" +select sink.getNode(), source, sink, + "This mass assignment operation can assign user-controlled attributes from $@.", source.getNode(), + "this remote flow source" diff --git a/ruby/ql/src/queries/security/cwe-915/examples/MassAssignmentBad.rb b/ruby/ql/src/queries/security/cwe-915/examples/MassAssignmentBad.rb new file mode 100644 index 000000000000..2824be1850b1 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-915/examples/MassAssignmentBad.rb @@ -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 \ No newline at end of file diff --git a/ruby/ql/src/queries/security/cwe-915/examples/MassAssignmentGood.rb b/ruby/ql/src/queries/security/cwe-915/examples/MassAssignmentGood.rb new file mode 100644 index 000000000000..148afeb503a5 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-915/examples/MassAssignmentGood.rb @@ -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 \ No newline at end of file From 507a6102a2c2635b1baffeb18926d55c7c6a974c Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 22 Mar 2024 11:24:32 +0000 Subject: [PATCH 04/15] Reorganise into Custimizations file + add some more sinks on ActiveRecord methods --- .../codeql/ruby/frameworks/ActiveRecord.qll | 33 +++++++++++ .../security/MassAssignmentCustomizations.qll | 55 +++++++++++++++++++ .../ruby/security/MassAssignmentQuery.qll | 49 +---------------- .../security/cwe-915/MassAssignment.expected | 2 +- 4 files changed, 92 insertions(+), 47 deletions(-) create mode 100644 ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll index 0f30f2146df1..69c65861990c 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll @@ -792,3 +792,36 @@ class ActiveRecordScopeCallTarget extends AdditionalCallTarget { ) } } + +/** Sinks for the mass assignment query */ +private module MassAssignmentSinks { + private import codeql.ruby.security.MassAssignmentCustomizations + + /** A call to a method that sets attributes of an database record using a hash. */ + private class MassAssignmentCall extends MassAssignment::Sink { + MassAssignmentCall() { + exists(DataFlow::CallNode call, string name | + ( + call = activeRecordBaseClass().getAMethodCall(name) + or + call instanceof ActiveRecordInstanceMethodCall and + call.getMethodName() = name + ) and + ( + name = + [ + "build", "create", "create!", "create_with", "create_or_find_by", + "create_or_find_by!", "find_or_create_by", "find_or_create_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) + ) + ) + } + } +} diff --git a/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll new file mode 100644 index 000000000000..3536cfef4d11 --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll @@ -0,0 +1,55 @@ +/** + * 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 { } + + private class PermitBangCall extends MassPermit instanceof DataFlow::CallNode { + PermitBangCall() { this.asExpr().getExpr().(MethodCall).getMethodName() = "permit!" } + + override DataFlow::Node getParamsArgument() { result = this.(DataFlow::CallNode).getReceiver() } + + override DataFlow::Node getPermittedParamsResult() { + result = this + or + result.(DataFlow::PostUpdateNode).getPreUpdateNode() = this.getParamsArgument() + } + } +} diff --git a/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll b/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll index fc145f0d6e95..9a6346800716 100644 --- a/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll +++ b/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll @@ -6,49 +6,7 @@ private import codeql.ruby.AST private import codeql.ruby.DataFlow private import codeql.ruby.TaintTracking private import codeql.ruby.dataflow.RemoteFlowSources - -/** Provides default sources and sinks for the mass assignment query. */ -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 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 { } - - private class CreateSink extends Sink { - CreateSink() { - exists(DataFlow::CallNode create | - create.asExpr().getExpr().(MethodCall).getMethodName() = ["create", "new", "update"] and - this = create.getArgument(0) - ) - } - } - - private class PermitCall extends MassPermit instanceof DataFlow::CallNode { - PermitCall() { this.asExpr().getExpr().(MethodCall).getMethodName() = "permit!" } - - override DataFlow::Node getParamsArgument() { result = this.(DataFlow::CallNode).getReceiver() } - - override DataFlow::Node getPermittedParamsResult() { result = this } - } -} +private import MassAssignmentCustomizations private module FlowState { private newtype TState = @@ -78,9 +36,6 @@ private module Config implements DataFlow::StateConfigSig { predicate isSource(DataFlow::Node node, FlowState state) { node instanceof MassAssignment::Source and state instanceof FlowState::Unpermitted - // or - // node = any(MassAssignment::MassPermit p).getPermittedParamsResult() and - // state instanceof FlowState::Permitted } predicate isSink(DataFlow::Node node, FlowState state) { @@ -88,6 +43,8 @@ private module Config implements DataFlow::StateConfigSig { state instanceof FlowState::Permitted } + predicate isBarrier(DataFlow::Node node) { node instanceof MassAssignment::Sanitizer } + predicate isAdditionalFlowStep( DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2 ) { diff --git a/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected b/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected index 3067ab1f48d4..8e9984495d5c 100644 --- a/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected +++ b/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected @@ -9,4 +9,4 @@ nodes | test.rb:17:9:17:37 | call to permit! | semmle.label | call to permit! | subpaths #select -| test.rb:8:18:8:28 | call to user_params | test.rb:17:9:17:14 | call to params | test.rb:8:18:8:28 | call to user_params | mass assignment | +| test.rb:8:18:8:28 | call to user_params | test.rb:17:9:17: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:17:9:17:14 | call to params | this remote flow source | From b74145349b96fad5a8374eef31f2e623a27a2ae7 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 22 Mar 2024 12:34:14 +0000 Subject: [PATCH 05/15] Add test cases --- .../codeql/ruby/frameworks/ActiveRecord.qll | 6 +- .../security/cwe-915/MassAssignment.expected | 83 +++++++++++++++++-- .../test/query-tests/security/cwe-915/test.rb | 26 ++++++ 3 files changed, 105 insertions(+), 10 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll index 69c65861990c..f7ae5570e93b 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll @@ -811,9 +811,9 @@ private module MassAssignmentSinks { name = [ "build", "create", "create!", "create_with", "create_or_find_by", - "create_or_find_by!", "find_or_create_by", "find_or_create_by!", "insert", "insert!", - "insert_all", "insert_all!", "instantiate", "new", "update", "update!", "upsert", - "upsert_all" + "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 diff --git a/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected b/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected index 8e9984495d5c..a803f99d6a6e 100644 --- a/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected +++ b/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected @@ -1,12 +1,81 @@ edges -| test.rb:17:9:17:14 | call to params | test.rb:17:9:17:29 | call to require | provenance | | -| test.rb:17:9:17:29 | call to require | test.rb:17:9:17:37 | call to permit! | provenance | | -| test.rb:17:9:17:37 | call to permit! | test.rb:8:18:8:28 | call to user_params | provenance | | +| 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 | | +| test.rb:43:9:43:37 | call to permit! | test.rb:18:20:18:30 | call to user_params | provenance | | +| test.rb:43:9:43:37 | call to permit! | test.rb:19:21:19:31 | call to user_params | provenance | | +| 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 | | nodes | test.rb:8:18:8:28 | call to user_params | semmle.label | call to user_params | -| test.rb:17:9:17:14 | call to params | semmle.label | call to params | -| test.rb:17:9:17:29 | call to require | semmle.label | call to require | -| test.rb:17:9:17:37 | call to permit! | semmle.label | call to permit! | +| 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 | +| test.rb:34:32:34:42 | call to user_params | semmle.label | call to user_params | +| test.rb:35:33:35:43 | call to user_params | semmle.label | call to user_params | +| test.rb:36:26:36:36 | call to user_params | semmle.label | call to user_params | +| 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! | subpaths #select -| test.rb:8:18:8:28 | call to user_params | test.rb:17:9:17: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:17:9:17:14 | call to params | this remote flow source | +| 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 | +| test.rb:18:20:18:30 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:18:20:18:30 | 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:19:21:19:31 | call to user_params | test.rb:43:9:43:14 | call to params | test.rb:19:21:19: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: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 | diff --git a/ruby/ql/test/query-tests/security/cwe-915/test.rb b/ruby/ql/test/query-tests/security/cwe-915/test.rb index 5dee931761dd..946425e99448 100644 --- a/ruby/ql/test/query-tests/security/cwe-915/test.rb +++ b/ruby/ql/test/query-tests/security/cwe-915/test.rb @@ -13,6 +13,32 @@ def create2 User.new(params[:user].permit(:name,:address)) end + def create3 + # each BAD + User.build(user_params) + User.create(user_params) + User.create!(user_params) + User.insert(user_params) + User.insert!(user_params) + User.insert_all([user_params]) + User.insert_all!([user_params]) + User.update(user_params) + User.update(7, user_params) + User.update!(user_params) + User.update!(7, user_params) + User.upsert(user_params) + User.upsert([user_params]) + User.find_or_create_by(user_params) + User.find_or_create_by!(user_params) + User.find_or_initialize_by(user_params) + User.create_or_find_by(user_params) + User.create_or_find_by!(user_params) + User.create_with(user_params) + + user = User.where(name:"abc") + user.update(user_params) + end + def user_params params.require(:user).permit! end From 01f712476bf35ab67eb643cf627f5e0fa17bf560 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 22 Mar 2024 14:04:15 +0000 Subject: [PATCH 06/15] Add change note and update severity --- ruby/ql/src/change-notes/2024-03-22-mass-assignment.md | 4 ++++ ruby/ql/src/queries/security/cwe-915/MassAssignment.ql | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 ruby/ql/src/change-notes/2024-03-22-mass-assignment.md diff --git a/ruby/ql/src/change-notes/2024-03-22-mass-assignment.md b/ruby/ql/src/change-notes/2024-03-22-mass-assignment.md new file mode 100644 index 000000000000..3f8743a30796 --- /dev/null +++ b/ruby/ql/src/change-notes/2024-03-22-mass-assignment.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* Added a new query, `ruby/insecure-mass-assignment`, for finding instances of mass assignment operations accepting arbitrary parameters from remote user input. \ No newline at end of file diff --git a/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql b/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql index 8803aad745a8..a1c79cfce1e9 100644 --- a/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql +++ b/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql @@ -3,7 +3,7 @@ * @description Using mass assignment with user-controlled attributes allows unintended parameters to be set. * @kind path-problem * @problem.severity error - * @security-severity 7.5 + * @security-severity 9.8 * @precision high * @id ruby/insecure-mass-assignment * @tags security From a6ee19ca2d5fe47aa49ab90c1d7ad8e36b8bd883 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 22 Mar 2024 14:36:47 +0000 Subject: [PATCH 07/15] Fix query id --- ruby/ql/src/change-notes/2024-03-22-mass-assignment.md | 2 +- ruby/ql/src/queries/security/cwe-915/MassAssignment.ql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ruby/ql/src/change-notes/2024-03-22-mass-assignment.md b/ruby/ql/src/change-notes/2024-03-22-mass-assignment.md index 3f8743a30796..566792cc5ae7 100644 --- a/ruby/ql/src/change-notes/2024-03-22-mass-assignment.md +++ b/ruby/ql/src/change-notes/2024-03-22-mass-assignment.md @@ -1,4 +1,4 @@ --- category: newQuery --- -* Added a new query, `ruby/insecure-mass-assignment`, for finding instances of mass assignment operations accepting arbitrary parameters from remote user input. \ No newline at end of file +* Added a new query, `rb/insecure-mass-assignment`, for finding instances of mass assignment operations accepting arbitrary parameters from remote user input. \ No newline at end of file diff --git a/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql b/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql index a1c79cfce1e9..8e7e60b8850c 100644 --- a/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql +++ b/ruby/ql/src/queries/security/cwe-915/MassAssignment.ql @@ -5,7 +5,7 @@ * @problem.severity error * @security-severity 9.8 * @precision high - * @id ruby/insecure-mass-assignment + * @id rb/insecure-mass-assignment * @tags security * external/cwe/cwe-915 */ From 592acb94d255188bcf23ddfe22fb9150e954b284 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 22 Mar 2024 15:28:34 +0000 Subject: [PATCH 08/15] Add missing .s to qldoc --- ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll | 2 +- .../lib/codeql/ruby/security/MassAssignmentCustomizations.qll | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll index f7ae5570e93b..46f700dbbbfa 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll @@ -793,7 +793,7 @@ class ActiveRecordScopeCallTarget extends AdditionalCallTarget { } } -/** Sinks for the mass assignment query */ +/** Sinks for the mass assignment query. */ private module MassAssignmentSinks { private import codeql.ruby.security.MassAssignmentCustomizations diff --git a/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll index 3536cfef4d11..43467c31cf9a 100644 --- a/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll @@ -32,7 +32,7 @@ module MassAssignment { * 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 */ + /** Gets the argument for the parameters to be permitted. */ abstract DataFlow::Node getParamsArgument(); /** Gets the result node of the permitted parameters. */ From fb192889816e985eac87e8e9d5a7a056f1029c3c Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 25 Mar 2024 14:44:24 +0000 Subject: [PATCH 09/15] Address review comments - Fix docs typo and add a reference --- ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp b/ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp index 7a96cd49b049..289f3a6a6ec1 100644 --- a/ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp +++ b/ruby/ql/src/queries/security/cwe-915/MassAssignment.qhelp @@ -5,7 +5,7 @@

Operations that allow for mass assignment (setting multiple attributes of an object using a hash), such as ActiveRecord::Base.new, should take care not to - allow arbitrary parameters to be set by the user. Otherwise, unintended attributes may be set, such as an isAdmin feild for a User object. + allow arbitrary parameters to be set by the user. Otherwise, unintended attributes may be set, such as an is_admin field for a User object.

@@ -29,6 +29,6 @@ - +
  • Rails guides: Strong Parameters.
  • From 3c96bf6b22862c9257fa49ba5c085eb095c07fef Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 3 Apr 2024 19:41:37 +0200 Subject: [PATCH 10/15] Fix bad join --- .../codeql/ruby/frameworks/ActiveRecord.qll | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll index 46f700dbbbfa..34f9bc5d3b91 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll @@ -797,30 +797,30 @@ class ActiveRecordScopeCallTarget extends AdditionalCallTarget { 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 MassAssignmentCall extends MassAssignment::Sink { MassAssignmentCall() { - exists(DataFlow::CallNode call, string name | - ( - call = activeRecordBaseClass().getAMethodCall(name) - or - call instanceof ActiveRecordInstanceMethodCall and - call.getMethodName() = name - ) and - ( - 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) - ) + 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) ) } } From c2d771b33450cc9ecc3fe30eb6284577530ec240 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 3 Apr 2024 19:58:51 +0200 Subject: [PATCH 11/15] Ruby: Reduce alerts produced by `MassAssignment.ql` --- ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll b/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll index 9a6346800716..5cfb28deca32 100644 --- a/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll +++ b/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll @@ -43,6 +43,11 @@ 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 isBarrier(DataFlow::Node node) { node instanceof MassAssignment::Sanitizer } predicate isAdditionalFlowStep( From 976ca48317258a5d1698043105a7df1fa56bb178 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Wed, 10 Apr 2024 10:17:19 +0100 Subject: [PATCH 12/15] Review suggestions - rename sink class and add barrier out --- ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll | 4 ++-- ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll index 34f9bc5d3b91..4db47cb83748 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll @@ -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 = [ diff --git a/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll b/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll index 5cfb28deca32..0d6bcd0a34f7 100644 --- a/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll +++ b/ruby/ql/lib/codeql/ruby/security/MassAssignmentQuery.qll @@ -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 } From 0a3d73d902889f18f4c63a9cfef002729e0ab099 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Wed, 10 Apr 2024 21:47:07 +0100 Subject: [PATCH 13/15] Add flow steps and sanitizers for `permit` calls --- .../security/MassAssignmentCustomizations.qll | 44 +++++++++++++++++- .../security/cwe-915/MassAssignment.expected | 45 ++++++++++--------- .../test/query-tests/security/cwe-915/test.rb | 11 +++++ 3 files changed, 78 insertions(+), 22 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll index 43467c31cf9a..adc587346dac 100644 --- a/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll @@ -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() } @@ -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) { + e instanceof HashLiteral and + count(e.(HashLiteral).getAKeyValuePair()) = 0 + 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 } + } } diff --git a/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected b/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected index a803f99d6a6e..01c62c8ed215 100644 --- a/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected +++ b/ruby/ql/test/query-tests/security/cwe-915/MassAssignment.expected @@ -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 | | @@ -13,20 +7,25 @@ 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 | @@ -34,20 +33,11 @@ nodes | 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 | @@ -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 | @@ -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 | diff --git a/ruby/ql/test/query-tests/security/cwe-915/test.rb b/ruby/ql/test/query-tests/security/cwe-915/test.rb index 946425e99448..a60def5d201f 100644 --- a/ruby/ql/test/query-tests/security/cwe-915/test.rb +++ b/ruby/ql/test/query-tests/security/cwe-915/test.rb @@ -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 \ No newline at end of file From ec973ac1f3071c308f6911adde2373bd11a9c318 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Thu, 11 Apr 2024 09:38:41 +0100 Subject: [PATCH 14/15] Use not exists --- .../lib/codeql/ruby/security/MassAssignmentCustomizations.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll index adc587346dac..75fb6481727b 100644 --- a/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll @@ -57,7 +57,7 @@ module MassAssignment { /** Holds if `h` is an empty hash or contains an empty hash at one if its (possibly nested) values. */ private predicate hasEmptyHash(Expr e) { e instanceof HashLiteral and - count(e.(HashLiteral).getAKeyValuePair()) = 0 + not exists(e.(HashLiteral).getAKeyValuePair()) or hasEmptyHash(e.(HashLiteral).getAKeyValuePair().getValue()) or From 06d7b3ce80aa81aa13dc2c043f5a5e652a4ba9f4 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Thu, 11 Apr 2024 22:30:41 +0100 Subject: [PATCH 15/15] Use cfg nodes --- .../security/MassAssignmentCustomizations.qll | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll index 75fb6481727b..eeb7dc7056a7 100644 --- a/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/MassAssignmentCustomizations.qll @@ -4,6 +4,7 @@ */ private import codeql.ruby.AST +private import codeql.ruby.controlflow.CfgNodes private import codeql.ruby.DataFlow private import codeql.ruby.TaintTracking private import codeql.ruby.dataflow.RemoteFlowSources @@ -55,22 +56,22 @@ module MassAssignment { } /** Holds if `h` is an empty hash or contains an empty hash at one if its (possibly nested) values. */ - private predicate hasEmptyHash(Expr e) { - e instanceof HashLiteral and - not exists(e.(HashLiteral).getAKeyValuePair()) + private predicate hasEmptyHash(ExprCfgNode e) { + e instanceof ExprNodes::HashLiteralCfgNode and + not exists(e.(ExprNodes::HashLiteralCfgNode).getAKeyValuePair()) or - hasEmptyHash(e.(HashLiteral).getAKeyValuePair().getValue()) + hasEmptyHash(e.(ExprNodes::HashLiteralCfgNode).getAKeyValuePair().getValue()) or - hasEmptyHash(e.(Pair).getValue()) + hasEmptyHash(e.(ExprNodes::PairCfgNode).getValue()) or - hasEmptyHash(e.(ArrayLiteral).getAnElement()) + hasEmptyHash(e.(ExprNodes::ArrayLiteralCfgNode).getAnArgument()) } /** 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()) + not hasEmptyHash(this.getArgument(_).getExprNode()) } } @@ -78,7 +79,7 @@ module MassAssignment { private class PermitCallMassPermit extends MassPermit instanceof DataFlow::CallNode { PermitCallMassPermit() { this.(DataFlow::CallNode).getMethodName() = "permit" and - hasEmptyHash(this.(DataFlow::CallNode).getArgument(_).asExpr().getExpr()) + hasEmptyHash(this.(DataFlow::CallNode).getArgument(_).getExprNode()) } override DataFlow::Node getParamsArgument() { result = this.(DataFlow::CallNode).getReceiver() }