Skip to content

Ruby: Track flow into ActiveRecord scopes #14426

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

Merged
hmac merged 6 commits into from
Mar 19, 2024
Merged

Ruby: Track flow into ActiveRecord scopes #14426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
80ae017
Ruby: Track flow into ActiveRecord scopes
hmac Oct 10, 2023
e895f96
Ruby: Taint flow to second block param in map
hmac Mar 18, 2024
187a68b
Ruby: Add flow summary for `Hash#keys`
hmac Mar 18, 2024
32b80f8
Ruby: Add tests for hash flow
hmac Mar 19, 2024
dde148e
Ruby: add changenote
hmac Mar 19, 2024
7e479e3
Ruby: Fix Hash#keys flow summary
hmac Mar 19, 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: 4 additions & 0 deletions ruby/ql/lib/change-notes/2024-03-19-activerecord-scopes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Data flow is now tracked through `ActiveRecord` scopes.
15 changes: 14 additions & 1 deletion ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
27 changes: 27 additions & 0 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 @@ -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()
)
}
}
3 changes: 2 additions & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions ruby/ql/lib/codeql/ruby/frameworks/core/Hash.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
18 changes: 18 additions & 0 deletions ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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] |
Expand Down Expand Up @@ -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 [] |
Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
2 changes: 1 addition & 1 deletion ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import codeql.ruby.AST
import codeql.ruby.CFG
import TestUtilities.InlineFlowTest
import ValueFlowTest<DefaultFlowConfig>
import DefaultFlowTest
import ValueFlow::PathGraph

query predicate hashLiteral(CfgNodes::ExprNodes::HashLiteralCfgNode n) { any() }
Expand Down
18 changes: 16 additions & 2 deletions ruby/ql/test/library-tests/dataflow/hash-flow/hash_flow.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()
Expand Down Expand Up @@ -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
11 changes: 11 additions & 0 deletions ruby/ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
8 changes: 8 additions & 0 deletions ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 | |
Expand Down Expand Up @@ -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 | ...[...] |
Expand Down Expand Up @@ -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 |
Expand Down