hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-16 16:53:25 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #14426 from hmac/hmac-ar-scopes

Browse Source 
Ruby: Track flow into ActiveRecord scopes
This commit is contained in:
Harry Maclean
2024-03-19 14:19:14 +00:00
committed by GitHub
parent f8124222cc 7e479e3c8e
commit 219cd4e415
10 changed files with 113 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
ruby/ql/lib/change-notes/2024-03-19-activerecord-scopes.md Normal file
View File

@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Data flow is now tracked through `ActiveRecord` scopes.

15
ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll
View File

@@ -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. */

27
ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll
View File

@@ -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
ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll
View File

@@ -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

12
ruby/ql/lib/codeql/ruby/frameworks/core/Hash.qll
View File

@@ -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
ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected
View File

@@ -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 |

2
ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.ql
View File

@@ -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() }

18
ruby/ql/test/library-tests/dataflow/hash-flow/hash_flow.rb
View File

@@ -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 @@ class M54
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
ruby/ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb
View File

@@ -204,3 +204,14 @@ class RegressionController < ActionController::Base
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
ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected
View File

@@ -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 |