diff --git a/ruby/ql/lib/change-notes/2024-03-19-activerecord-scopes.md b/ruby/ql/lib/change-notes/2024-03-19-activerecord-scopes.md new file mode 100644 index 000000000000..963479568a0b --- /dev/null +++ b/ruby/ql/lib/change-notes/2024-03-19-activerecord-scopes.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Data flow is now tracked through `ActiveRecord` scopes. diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll index e7898a1ec4fc..4ac9031f05cb 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll @@ -429,7 +429,20 @@ private Callable viableSourceCallableInit(RelevantCall call) { result = getIniti /** Holds if `call` may resolve to the returned source-code method. */ private DataFlowCallable viableSourceCallable(DataFlowCall call) { result = viableSourceCallableNonInit(call) or - result.asCfgScope() = viableSourceCallableInit(call.asCall()) + result.asCfgScope() = viableSourceCallableInit(call.asCall()) or + result = any(AdditionalCallTarget t).viableTarget(call.asCall()) +} + +/** + * A unit class for adding additional call steps. + * + * Extend this class to add additional call steps to the data flow graph. + */ +class AdditionalCallTarget extends Unit { + /** + * Gets a viable target for `call`. + */ + abstract DataFlowCallable viableTarget(CfgNodes::ExprNodes::CallCfgNode call); } /** Holds if `call` may resolve to the returned summarized library method. */ diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll index 7573e099c199..0f30f2146df1 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll @@ -765,3 +765,30 @@ private class ActiveRecordCollectionProxyModelInstantiation extends ActiveRecord result = this.(ActiveRecordCollectionProxyMethodCall).getAssociation().getTargetClass() } } + +/** + * An additional call step for calls to ActiveRecord scopes. For example, in the following code: + * + * ```rb + * class User < ActiveRecord::Base + * scope :with_role, ->(role) { where(role: role) } + * end + * + * User.with_role(r) + * ``` + * + * the call to `with_role` targets the lambda, and argument `r` flows to the parameter `role`. + */ +class ActiveRecordScopeCallTarget extends AdditionalCallTarget { + override DataFlowCallable viableTarget(ExprNodes::CallCfgNode scopeCall) { + exists(DataFlow::ModuleNode model, string scopeName | + model = activeRecordBaseClass().getADescendentModule() and + exists(DataFlow::CallNode scope | + scope = model.getAModuleLevelCall("scope") and + scope.getArgument(0).getConstantValue().isStringlikeValue(scopeName) and + scope.getArgument(1).asCallable().asCallableAstNode() = result.asCfgScope() + ) and + scopeCall = model.getAnImmediateReference().getAMethodCall(scopeName).asExpr() + ) + } +} diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll index b2a30beafc3a..2da521e54a1b 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll @@ -1855,7 +1855,8 @@ module Enumerable { override predicate propagatesFlow(string input, string output, boolean preservesValue) { input = "Argument[self].Element[any]" and - output = "Argument[block].Parameter[0]" and + // For `Hash#map`, the value flows to parameter 1 + output = "Argument[block].Parameter[0, 1]" and preservesValue = true or input = "Argument[block].ReturnValue" and diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Hash.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Hash.qll index 4871d8d99243..38a9a70f0d3f 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/core/Hash.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Hash.qll @@ -523,3 +523,15 @@ private class ValuesSummary extends SimpleSummarizedCallable { preservesValue = true } } + +// We don't (yet) track data flow through hash keys, but this is still useful in cases where a +// whole hash(like) object is tainted, such as `ActionController#params`. +private class KeysSummary extends SimpleSummarizedCallable { + KeysSummary() { this = "keys" } + + override predicate propagatesFlow(string input, string output, boolean preservesValue) { + input = "Argument[self]" and + output = "ReturnValue.Element[?]" and + preservesValue = false + } +} diff --git a/ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected b/ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected index d2da8837a568..68cb5a53dc23 100644 --- a/ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected +++ b/ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected @@ -1089,6 +1089,13 @@ edges | hash_flow.rb:994:30:994:40 | call to taint | hash_flow.rb:994:14:994:47 | ...[...] [element :b] | provenance | | | hash_flow.rb:996:14:996:15 | h2 [element :b] | hash_flow.rb:996:14:996:19 | ...[...] | provenance | | | hash_flow.rb:998:14:998:15 | h2 [element :b] | hash_flow.rb:998:14:998:18 | ...[...] | provenance | | +| hash_flow.rb:1011:5:1011:5 | h [element :a] | hash_flow.rb:1012:5:1012:5 | h [element :a] | provenance | | +| hash_flow.rb:1011:9:1011:45 | call to [] [element :a] | hash_flow.rb:1011:5:1011:5 | h [element :a] | provenance | | +| hash_flow.rb:1011:14:1011:24 | call to taint | hash_flow.rb:1011:9:1011:45 | call to [] [element :a] | provenance | | +| hash_flow.rb:1012:5:1012:5 | h [element :a] | hash_flow.rb:1012:15:1012:15 | k | provenance | | +| hash_flow.rb:1012:5:1012:5 | h [element :a] | hash_flow.rb:1012:18:1012:18 | v | provenance | | +| hash_flow.rb:1012:15:1012:15 | k | hash_flow.rb:1014:14:1014:14 | k | provenance | | +| hash_flow.rb:1012:18:1012:18 | v | hash_flow.rb:1013:14:1013:14 | v | provenance | | nodes | hash_flow.rb:10:5:10:8 | hash [element 0] | semmle.label | hash [element 0] | | hash_flow.rb:10:5:10:8 | hash [element :a] | semmle.label | hash [element :a] | @@ -2251,6 +2258,14 @@ nodes | hash_flow.rb:996:14:996:19 | ...[...] | semmle.label | ...[...] | | hash_flow.rb:998:14:998:15 | h2 [element :b] | semmle.label | h2 [element :b] | | hash_flow.rb:998:14:998:18 | ...[...] | semmle.label | ...[...] | +| hash_flow.rb:1011:5:1011:5 | h [element :a] | semmle.label | h [element :a] | +| hash_flow.rb:1011:9:1011:45 | call to [] [element :a] | semmle.label | call to [] [element :a] | +| hash_flow.rb:1011:14:1011:24 | call to taint | semmle.label | call to taint | +| hash_flow.rb:1012:5:1012:5 | h [element :a] | semmle.label | h [element :a] | +| hash_flow.rb:1012:15:1012:15 | k | semmle.label | k | +| hash_flow.rb:1012:18:1012:18 | v | semmle.label | v | +| hash_flow.rb:1013:14:1013:14 | v | semmle.label | v | +| hash_flow.rb:1014:14:1014:14 | k | semmle.label | k | subpaths hashLiteral | hash_flow.rb:10:12:21:5 | call to [] | @@ -2324,6 +2339,7 @@ hashLiteral | hash_flow.rb:946:13:950:5 | call to [] | | hash_flow.rb:971:9:971:38 | ...[...] | | hash_flow.rb:994:14:994:47 | ...[...] | +| hash_flow.rb:1011:9:1011:45 | call to [] | #select | hash_flow.rb:22:10:22:17 | ...[...] | hash_flow.rb:11:15:11:24 | call to taint | hash_flow.rb:22:10:22:17 | ...[...] | $@ | hash_flow.rb:11:15:11:24 | call to taint | call to taint | | hash_flow.rb:24:10:24:17 | ...[...] | hash_flow.rb:13:12:13:21 | call to taint | hash_flow.rb:24:10:24:17 | ...[...] | $@ | hash_flow.rb:13:12:13:21 | call to taint | call to taint | @@ -2569,3 +2585,5 @@ hashLiteral | hash_flow.rb:975:10:975:13 | ...[...] | hash_flow.rb:971:23:971:31 | call to taint | hash_flow.rb:975:10:975:13 | ...[...] | $@ | hash_flow.rb:971:23:971:31 | call to taint | call to taint | | hash_flow.rb:996:14:996:19 | ...[...] | hash_flow.rb:994:30:994:40 | call to taint | hash_flow.rb:996:14:996:19 | ...[...] | $@ | hash_flow.rb:994:30:994:40 | call to taint | call to taint | | hash_flow.rb:998:14:998:18 | ...[...] | hash_flow.rb:994:30:994:40 | call to taint | hash_flow.rb:998:14:998:18 | ...[...] | $@ | hash_flow.rb:994:30:994:40 | call to taint | call to taint | +| hash_flow.rb:1013:14:1013:14 | v | hash_flow.rb:1011:14:1011:24 | call to taint | hash_flow.rb:1013:14:1013:14 | v | $@ | hash_flow.rb:1011:14:1011:24 | call to taint | call to taint | +| hash_flow.rb:1014:14:1014:14 | k | hash_flow.rb:1011:14:1011:24 | call to taint | hash_flow.rb:1014:14:1014:14 | k | $@ | hash_flow.rb:1011:14:1011:24 | call to taint | call to taint | diff --git a/ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.ql b/ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.ql index e3b694d3e757..5ec8ec0a0d66 100644 --- a/ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.ql +++ b/ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.ql @@ -5,7 +5,7 @@ import codeql.ruby.AST import codeql.ruby.CFG import TestUtilities.InlineFlowTest -import ValueFlowTest +import DefaultFlowTest import ValueFlow::PathGraph query predicate hashLiteral(CfgNodes::ExprNodes::HashLiteralCfgNode n) { any() } diff --git a/ruby/ql/test/library-tests/dataflow/hash-flow/hash_flow.rb b/ruby/ql/test/library-tests/dataflow/hash-flow/hash_flow.rb index 14c2504f959f..edc1e325b09d 100644 --- a/ruby/ql/test/library-tests/dataflow/hash-flow/hash_flow.rb +++ b/ruby/ql/test/library-tests/dataflow/hash-flow/hash_flow.rb @@ -59,7 +59,7 @@ def m3() x = {a: taint(3.2), b: 1} hash2 = Hash[x] sink(hash2[:a]) # $ hasValueFlow=3.2 - sink(hash2[:b]) + sink(hash2[:b]) # $ hasTaintFlow=3.2 hash3 = Hash[[[:a, taint(3.3)], [:b, 1]]] sink(hash3[:a]) # $ hasValueFlow=3.3 @@ -75,7 +75,7 @@ def m3() hash6 = Hash[{"a" => taint(3.6), "b" => 1}] sink(hash6["a"]) # $ hasValueFlow=3.6 - sink(hash6["b"]) + sink(hash6["b"]) # $ hasTaintFlow=3.6 end m3() @@ -1000,3 +1000,17 @@ def m54(i) end M54.new.m54(:b) + +def m55 + h = taint(55.1) + keys = h.keys + sink(keys[f()]) # $ hasTaintFlow=55.1 +end + +def m56 + h = { a: taint(56.1), taint(56.2) => :b } + h.map do |k, v| + sink(v) # $ hasValueFlow=56.1 + sink(k) # $ MISSING: hasValueFlow=56.2 SPURIOUS: hasValueFlow=56.1 + end +end diff --git a/ruby/ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb b/ruby/ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb index ad074de5e980..f1f8d680b751 100644 --- a/ruby/ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb +++ b/ruby/ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb @@ -204,3 +204,14 @@ def show Regression.connection.execute("SELECT * FROM users WHERE id = #{permitted_params[:user_id]}") end end + +class User + scope :with_role, ->(role) { where("role = #{role}") } +end + +class UsersController < ActionController::Base + def index + # BAD: user input passed to scope which uses it without sanitization. + @users = User.with_role(params[:role]) + end +end diff --git a/ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected b/ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected index 8b6c5bf4d16f..6593e7606da0 100644 --- a/ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected +++ b/ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected @@ -76,6 +76,9 @@ edges | ActiveRecordInjection.rb:203:77:203:102 | ...[...] | ActiveRecordInjection.rb:203:43:203:104 | "SELECT * FROM users WHERE id ..." | provenance | | | ActiveRecordInjection.rb:204:69:204:84 | call to permitted_params | ActiveRecordInjection.rb:204:69:204:94 | ...[...] | provenance | | | ActiveRecordInjection.rb:204:69:204:94 | ...[...] | ActiveRecordInjection.rb:204:35:204:96 | "SELECT * FROM users WHERE id ..." | provenance | | +| ActiveRecordInjection.rb:209:24:209:27 | role | ActiveRecordInjection.rb:209:38:209:53 | "role = #{...}" | provenance | | +| ActiveRecordInjection.rb:215:29:215:34 | call to params | ActiveRecordInjection.rb:215:29:215:41 | ...[...] | provenance | | +| ActiveRecordInjection.rb:215:29:215:41 | ...[...] | ActiveRecordInjection.rb:209:24:209:27 | role | provenance | | | ArelInjection.rb:4:5:4:8 | name | ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | provenance | | | ArelInjection.rb:4:5:4:8 | name | ArelInjection.rb:7:39:7:80 | "SELECT * FROM users WHERE nam..." | provenance | | | ArelInjection.rb:4:12:4:17 | call to params | ArelInjection.rb:4:12:4:29 | ...[...] | provenance | | @@ -201,6 +204,10 @@ nodes | ActiveRecordInjection.rb:204:35:204:96 | "SELECT * FROM users WHERE id ..." | semmle.label | "SELECT * FROM users WHERE id ..." | | ActiveRecordInjection.rb:204:69:204:84 | call to permitted_params | semmle.label | call to permitted_params | | ActiveRecordInjection.rb:204:69:204:94 | ...[...] | semmle.label | ...[...] | +| ActiveRecordInjection.rb:209:24:209:27 | role | semmle.label | role | +| ActiveRecordInjection.rb:209:38:209:53 | "role = #{...}" | semmle.label | "role = #{...}" | +| ActiveRecordInjection.rb:215:29:215:34 | call to params | semmle.label | call to params | +| ActiveRecordInjection.rb:215:29:215:41 | ...[...] | semmle.label | ...[...] | | ArelInjection.rb:4:5:4:8 | name | semmle.label | name | | ArelInjection.rb:4:12:4:17 | call to params | semmle.label | call to params | | ArelInjection.rb:4:12:4:29 | ...[...] | semmle.label | ...[...] | @@ -257,6 +264,7 @@ subpaths | ActiveRecordInjection.rb:194:37:194:41 | query | ActiveRecordInjection.rb:199:5:199:10 | call to params | ActiveRecordInjection.rb:194:37:194:41 | query | This SQL query depends on a $@. | ActiveRecordInjection.rb:199:5:199:10 | call to params | user-provided value | | ActiveRecordInjection.rb:203:43:203:104 | "SELECT * FROM users WHERE id ..." | ActiveRecordInjection.rb:199:5:199:10 | call to params | ActiveRecordInjection.rb:203:43:203:104 | "SELECT * FROM users WHERE id ..." | This SQL query depends on a $@. | ActiveRecordInjection.rb:199:5:199:10 | call to params | user-provided value | | ActiveRecordInjection.rb:204:35:204:96 | "SELECT * FROM users WHERE id ..." | ActiveRecordInjection.rb:199:5:199:10 | call to params | ActiveRecordInjection.rb:204:35:204:96 | "SELECT * FROM users WHERE id ..." | This SQL query depends on a $@. | ActiveRecordInjection.rb:199:5:199:10 | call to params | user-provided value | +| ActiveRecordInjection.rb:209:38:209:53 | "role = #{...}" | ActiveRecordInjection.rb:215:29:215:34 | call to params | ActiveRecordInjection.rb:209:38:209:53 | "role = #{...}" | This SQL query depends on a $@. | ActiveRecordInjection.rb:215:29:215:34 | call to params | user-provided value | | ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | ArelInjection.rb:4:12:4:17 | call to params | ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | This SQL query depends on a $@. | ArelInjection.rb:4:12:4:17 | call to params | user-provided value | | ArelInjection.rb:7:39:7:80 | "SELECT * FROM users WHERE nam..." | ArelInjection.rb:4:12:4:17 | call to params | ArelInjection.rb:7:39:7:80 | "SELECT * FROM users WHERE nam..." | This SQL query depends on a $@. | ArelInjection.rb:4:12:4:17 | call to params | user-provided value | | PgInjection.rb:14:15:14:18 | qry1 | PgInjection.rb:6:12:6:17 | call to params | PgInjection.rb:14:15:14:18 | qry1 | This SQL query depends on a $@. | PgInjection.rb:6:12:6:17 | call to params | user-provided value |