Skip to content

Commit 2f9f1f7

Browse files
nickrolfenickrolfe
authored
Merge pull request #11166 from github/nickrolfe/active_support_flow_summaries
Ruby: generalise summaries for ActiveSupport Hash extensions
2 parents a8ed6ba + eb2a487 commit 2f9f1f7

File tree

9 files changed

+564
-28
lines changed

9 files changed

+564
-28
lines changed

‎ruby/ql/lib/change-notes/2022-11-08-activesupport-hash-extensions.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Taint flow through the `ActiveSupport` extensions `Hash#reverse_merge` and `Hash:reverse_merge!`, and their aliases, is now modeled more generally, where previously it was only modeled in the context of `ActionController` parameters.

‎ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -636,13 +636,14 @@ private module ParamsSummaries {
636636
* Returns current ActionController::Parameters instance with current hash merged into other_hash.
637637
* `#reverse_merge!`
638638
* `#with_defaults!`
639+
* `#reverse_update`
639640
* Returns a new ActionController::Parameters with all keys from current hash merged into other_hash.
640641
*/
641642
private class MergeBangSummary extends SummarizedCallable {
642643
MergeBangSummary() { this = "ActionController::Parameters#merge!" }
643644

644645
override MethodCall getACall() {
645-
result.getMethodName() = ["merge!", "reverse_merge!", "with_defaults!"] and
646+
result.getMethodName() = ["merge!", "reverse_merge!", "with_defaults!", "reverse_update"] and
646647
paramsInstance().getALocalUse().asExpr().getExpr() =
647648
[result.getReceiver(), result.getArgument(0)]
648649
}

‎ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,32 @@ module ActiveSupport {
120120
}
121121
}
122122

123+
/**
124+
* Flow summary for `reverse_merge`, and its alias `with_defaults`.
125+
*/
126+
private class ReverseMergeSummary extends SimpleSummarizedCallable {
127+
ReverseMergeSummary() { this = ["reverse_merge", "with_defaults"] }
128+
129+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
130+
input = "Argument[self,0].WithElement[any]" and
131+
output = "ReturnValue" and
132+
preservesValue = true
133+
}
134+
}
135+
136+
/**
137+
* Flow summary for `reverse_merge!`, and its aliases `with_defaults!` and `reverse_update`.
138+
*/
139+
private class ReverseMergeBangSummary extends SimpleSummarizedCallable {
140+
ReverseMergeBangSummary() { this = ["reverse_merge!", "with_defaults!", "reverse_update"] }
141+
142+
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
143+
input = "Argument[self,0].WithElement[any]" and
144+
output = ["ReturnValue", "Argument[self]"] and
145+
preservesValue = true
146+
}
147+
}
148+
123149
private class TransformSummary extends SimpleSummarizedCallable {
124150
TransformSummary() {
125151
this =

‎ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected‎

Lines changed: 344 additions & 0 deletions
Large diffs are not rendered by default.

‎ruby/ql/test/library-tests/dataflow/hash-flow/hash_flow.rb‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -842,3 +842,125 @@ def m48()
842842
end
843843

844844
m48()
845+
846+
def m49()
847+
hash1 = {
848+
a: taint(49.1),
849+
b: 1,
850+
c: taint(49.2)
851+
}
852+
hash2 = {
853+
d: taint(49.3),
854+
e: 1,
855+
f: taint(49.4)
856+
}
857+
858+
hash3 = hash1.reverse_merge(hash2)
859+
sink (hash3[:a]) # $ hasValueFlow=49.1
860+
sink (hash3[:b])
861+
sink (hash3[:c]) # $ hasValueFlow=49.2
862+
sink (hash3[:d]) # $ hasValueFlow=49.3
863+
sink (hash3[:e])
864+
sink (hash3[:f]) # $ hasValueFlow=49.4
865+
866+
# alias for reverse_merge
867+
hash4 = hash1.with_defaults(hash2)
868+
sink (hash4[:a]) # $ hasValueFlow=49.1
869+
sink (hash4[:b])
870+
sink (hash4[:c]) # $ hasValueFlow=49.2
871+
sink (hash4[:d]) # $ hasValueFlow=49.3
872+
sink (hash4[:e])
873+
sink (hash4[:f]) # $ hasValueFlow=49.4
874+
end
875+
876+
m49()
877+
878+
def m50()
879+
hash1 = {
880+
a: taint(50.1),
881+
b: 1,
882+
c: taint(50.2)
883+
}
884+
hash2 = {
885+
d: taint(50.3),
886+
e: 1,
887+
f: taint(50.4)
888+
}
889+
890+
hash = hash1.reverse_merge!(hash2)
891+
sink (hash[:a]) # $ hasValueFlow=50.1
892+
sink (hash[:b])
893+
sink (hash[:c]) # $ hasValueFlow=50.2
894+
sink (hash[:d]) # $ hasValueFlow=50.3
895+
sink (hash[:e])
896+
sink (hash[:f]) # $ hasValueFlow=50.4
897+
898+
sink (hash1[:a]) # $ hasValueFlow=50.1
899+
sink (hash1[:b])
900+
sink (hash1[:c]) # $ hasValueFlow=50.2
901+
sink (hash1[:d]) # $ hasValueFlow=50.3
902+
sink (hash1[:e])
903+
sink (hash1[:f]) # $ hasValueFlow=50.4
904+
end
905+
906+
m50()
907+
908+
def m51()
909+
hash1 = {
910+
a: taint(51.1),
911+
b: 1,
912+
c: taint(51.2)
913+
}
914+
hash2 = {
915+
d: taint(51.3),
916+
e: 1,
917+
f: taint(51.4)
918+
}
919+
920+
hash = hash1.with_defaults!(hash2)
921+
sink (hash[:a]) # $ hasValueFlow=51.1
922+
sink (hash[:b])
923+
sink (hash[:c]) # $ hasValueFlow=51.2
924+
sink (hash[:d]) # $ hasValueFlow=51.3
925+
sink (hash[:e])
926+
sink (hash[:f]) # $ hasValueFlow=51.4
927+
928+
sink (hash1[:a]) # $ hasValueFlow=51.1
929+
sink (hash1[:b])
930+
sink (hash1[:c]) # $ hasValueFlow=51.2
931+
sink (hash1[:d]) # $ hasValueFlow=51.3
932+
sink (hash1[:e])
933+
sink (hash1[:f]) # $ hasValueFlow=51.4
934+
end
935+
936+
m51()
937+
938+
def m52()
939+
hash1 = {
940+
a: taint(52.1),
941+
b: 1,
942+
c: taint(52.2)
943+
}
944+
hash2 = {
945+
d: taint(52.3),
946+
e: 1,
947+
f: taint(52.4)
948+
}
949+
950+
hash = hash1.with_defaults!(hash2)
951+
sink (hash[:a]) # $ hasValueFlow=52.1
952+
sink (hash[:b])
953+
sink (hash[:c]) # $ hasValueFlow=52.2
954+
sink (hash[:d]) # $ hasValueFlow=52.3
955+
sink (hash[:e])
956+
sink (hash[:f]) # $ hasValueFlow=52.4
957+
958+
sink (hash1[:a]) # $ hasValueFlow=52.1
959+
sink (hash1[:b])
960+
sink (hash1[:c]) # $ hasValueFlow=52.2
961+
sink (hash1[:d]) # $ hasValueFlow=52.3
962+
sink (hash1[:e])
963+
sink (hash1[:f]) # $ hasValueFlow=52.4
964+
end
965+
966+
m52()

‎ruby/ql/test/library-tests/dataflow/hash-flow/type-tracking-hash-flow.expected‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,3 +31,9 @@
3131
| hash_flow.rb:782:10:782:17 | ...[...] | Unexpected result: hasValueFlow=46.3 |
3232
| hash_flow.rb:839:22:839:42 | # $ hasValueFlow=48.3 | Missing result:hasValueFlow=48.3 |
3333
| hash_flow.rb:841:22:841:42 | # $ hasValueFlow=48.4 | Missing result:hasValueFlow=48.4 |
34+
| hash_flow.rb:901:22:901:42 | # $ hasValueFlow=50.3 | Missing result:hasValueFlow=50.3 |
35+
| hash_flow.rb:903:22:903:42 | # $ hasValueFlow=50.4 | Missing result:hasValueFlow=50.4 |
36+
| hash_flow.rb:931:22:931:42 | # $ hasValueFlow=51.3 | Missing result:hasValueFlow=51.3 |
37+
| hash_flow.rb:933:22:933:42 | # $ hasValueFlow=51.4 | Missing result:hasValueFlow=51.4 |
38+
| hash_flow.rb:961:22:961:42 | # $ hasValueFlow=52.3 | Missing result:hasValueFlow=52.3 |
39+
| hash_flow.rb:963:22:963:42 | # $ hasValueFlow=52.4 | Missing result:hasValueFlow=52.4 |

‎ruby/ql/test/library-tests/frameworks/ActionController.expected‎

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
actionControllerControllerClasses
22
| action_controller/input_access.rb:1:1:50:3 | UsersController |
3-
| action_controller/params_flow.rb:1:1:153:3 | MyController |
4-
| action_controller/params_flow.rb:161:1:169:3 | Subclass |
3+
| action_controller/params_flow.rb:1:1:162:3 | MyController |
4+
| action_controller/params_flow.rb:170:1:178:3 | Subclass |
55
| active_record/ActiveRecord.rb:23:1:39:3 | FooController |
66
| active_record/ActiveRecord.rb:41:1:64:3 | BarController |
77
| active_record/ActiveRecord.rb:66:1:98:3 | BazController |
@@ -48,8 +48,9 @@ actionControllerActionMethods
4848
| action_controller/params_flow.rb:125:3:132:5 | m30 |
4949
| action_controller/params_flow.rb:134:3:141:5 | m31 |
5050
| action_controller/params_flow.rb:143:3:150:5 | m32 |
51-
| action_controller/params_flow.rb:156:3:158:5 | m33 |
52-
| action_controller/params_flow.rb:162:3:164:5 | m34 |
51+
| action_controller/params_flow.rb:152:3:159:5 | m33 |
52+
| action_controller/params_flow.rb:165:3:167:5 | m34 |
53+
| action_controller/params_flow.rb:171:3:173:5 | m35 |
5354
| active_record/ActiveRecord.rb:27:3:38:5 | some_request_handler |
5455
| active_record/ActiveRecord.rb:42:3:47:5 | some_other_request_handler |
5556
| active_record/ActiveRecord.rb:49:3:63:5 | safe_paths |
@@ -120,9 +121,12 @@ paramsCalls
120121
| action_controller/params_flow.rb:144:10:144:15 | call to params |
121122
| action_controller/params_flow.rb:145:32:145:37 | call to params |
122123
| action_controller/params_flow.rb:148:22:148:27 | call to params |
123-
| action_controller/params_flow.rb:157:10:157:15 | call to params |
124-
| action_controller/params_flow.rb:163:10:163:15 | call to params |
125-
| action_controller/params_flow.rb:167:10:167:15 | call to params |
124+
| action_controller/params_flow.rb:153:10:153:15 | call to params |
125+
| action_controller/params_flow.rb:154:32:154:37 | call to params |
126+
| action_controller/params_flow.rb:157:22:157:27 | call to params |
127+
| action_controller/params_flow.rb:166:10:166:15 | call to params |
128+
| action_controller/params_flow.rb:172:10:172:15 | call to params |
129+
| action_controller/params_flow.rb:176:10:176:15 | call to params |
126130
| action_mailer/mailer.rb:3:10:3:15 | call to params |
127131
| active_record/ActiveRecord.rb:28:30:28:35 | call to params |
128132
| active_record/ActiveRecord.rb:29:29:29:34 | call to params |
@@ -198,9 +202,12 @@ paramsSources
198202
| action_controller/params_flow.rb:144:10:144:15 | call to params |
199203
| action_controller/params_flow.rb:145:32:145:37 | call to params |
200204
| action_controller/params_flow.rb:148:22:148:27 | call to params |
201-
| action_controller/params_flow.rb:157:10:157:15 | call to params |
202-
| action_controller/params_flow.rb:163:10:163:15 | call to params |
203-
| action_controller/params_flow.rb:167:10:167:15 | call to params |
205+
| action_controller/params_flow.rb:153:10:153:15 | call to params |
206+
| action_controller/params_flow.rb:154:32:154:37 | call to params |
207+
| action_controller/params_flow.rb:157:22:157:27 | call to params |
208+
| action_controller/params_flow.rb:166:10:166:15 | call to params |
209+
| action_controller/params_flow.rb:172:10:172:15 | call to params |
210+
| action_controller/params_flow.rb:176:10:176:15 | call to params |
204211
| action_mailer/mailer.rb:3:10:3:15 | call to params |
205212
| active_record/ActiveRecord.rb:28:30:28:35 | call to params |
206213
| active_record/ActiveRecord.rb:29:29:29:34 | call to params |
@@ -315,9 +322,12 @@ httpInputAccesses
315322
| action_controller/params_flow.rb:144:10:144:15 | call to params | ActionController::Metal#params |
316323
| action_controller/params_flow.rb:145:32:145:37 | call to params | ActionController::Metal#params |
317324
| action_controller/params_flow.rb:148:22:148:27 | call to params | ActionController::Metal#params |
318-
| action_controller/params_flow.rb:157:10:157:15 | call to params | ActionController::Metal#params |
319-
| action_controller/params_flow.rb:163:10:163:15 | call to params | ActionController::Metal#params |
320-
| action_controller/params_flow.rb:167:10:167:15 | call to params | ActionController::Metal#params |
325+
| action_controller/params_flow.rb:153:10:153:15 | call to params | ActionController::Metal#params |
326+
| action_controller/params_flow.rb:154:32:154:37 | call to params | ActionController::Metal#params |
327+
| action_controller/params_flow.rb:157:22:157:27 | call to params | ActionController::Metal#params |
328+
| action_controller/params_flow.rb:166:10:166:15 | call to params | ActionController::Metal#params |
329+
| action_controller/params_flow.rb:172:10:172:15 | call to params | ActionController::Metal#params |
330+
| action_controller/params_flow.rb:176:10:176:15 | call to params | ActionController::Metal#params |
321331
| action_mailer/mailer.rb:3:10:3:15 | call to params | ActionController::Metal#params |
322332
| active_record/ActiveRecord.rb:28:30:28:35 | call to params | ActionController::Metal#params |
323333
| active_record/ActiveRecord.rb:29:29:29:34 | call to params | ActionController::Metal#params |

‎ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected‎

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,13 @@ edges
4444
| params_flow.rb:145:32:145:37 | call to params : | params_flow.rb:145:10:145:38 | call to with_defaults! |
4545
| params_flow.rb:148:5:148:5 | [post] p : | params_flow.rb:149:10:149:10 | p |
4646
| params_flow.rb:148:22:148:27 | call to params : | params_flow.rb:148:5:148:5 | [post] p : |
47-
| params_flow.rb:157:10:157:15 | call to params : | params_flow.rb:157:10:157:19 | ...[...] |
48-
| params_flow.rb:163:10:163:15 | call to params : | params_flow.rb:163:10:163:19 | ...[...] |
49-
| params_flow.rb:167:10:167:15 | call to params : | params_flow.rb:167:10:167:19 | ...[...] |
47+
| params_flow.rb:153:10:153:15 | call to params : | params_flow.rb:153:10:153:44 | call to reverse_update |
48+
| params_flow.rb:154:32:154:37 | call to params : | params_flow.rb:154:10:154:38 | call to reverse_update |
49+
| params_flow.rb:157:5:157:5 | [post] p : | params_flow.rb:158:10:158:10 | p |
50+
| params_flow.rb:157:22:157:27 | call to params : | params_flow.rb:157:5:157:5 | [post] p : |
51+
| params_flow.rb:166:10:166:15 | call to params : | params_flow.rb:166:10:166:19 | ...[...] |
52+
| params_flow.rb:172:10:172:15 | call to params : | params_flow.rb:172:10:172:19 | ...[...] |
53+
| params_flow.rb:176:10:176:15 | call to params : | params_flow.rb:176:10:176:19 | ...[...] |
5054
nodes
5155
| params_flow.rb:3:10:3:15 | call to params : | semmle.label | call to params : |
5256
| params_flow.rb:3:10:3:19 | ...[...] | semmle.label | ...[...] |
@@ -133,12 +137,19 @@ nodes
133137
| params_flow.rb:148:5:148:5 | [post] p : | semmle.label | [post] p : |
134138
| params_flow.rb:148:22:148:27 | call to params : | semmle.label | call to params : |
135139
| params_flow.rb:149:10:149:10 | p | semmle.label | p |
136-
| params_flow.rb:157:10:157:15 | call to params : | semmle.label | call to params : |
137-
| params_flow.rb:157:10:157:19 | ...[...] | semmle.label | ...[...] |
138-
| params_flow.rb:163:10:163:15 | call to params : | semmle.label | call to params : |
139-
| params_flow.rb:163:10:163:19 | ...[...] | semmle.label | ...[...] |
140-
| params_flow.rb:167:10:167:15 | call to params : | semmle.label | call to params : |
141-
| params_flow.rb:167:10:167:19 | ...[...] | semmle.label | ...[...] |
140+
| params_flow.rb:153:10:153:15 | call to params : | semmle.label | call to params : |
141+
| params_flow.rb:153:10:153:44 | call to reverse_update | semmle.label | call to reverse_update |
142+
| params_flow.rb:154:10:154:38 | call to reverse_update | semmle.label | call to reverse_update |
143+
| params_flow.rb:154:32:154:37 | call to params : | semmle.label | call to params : |
144+
| params_flow.rb:157:5:157:5 | [post] p : | semmle.label | [post] p : |
145+
| params_flow.rb:157:22:157:27 | call to params : | semmle.label | call to params : |
146+
| params_flow.rb:158:10:158:10 | p | semmle.label | p |
147+
| params_flow.rb:166:10:166:15 | call to params : | semmle.label | call to params : |
148+
| params_flow.rb:166:10:166:19 | ...[...] | semmle.label | ...[...] |
149+
| params_flow.rb:172:10:172:15 | call to params : | semmle.label | call to params : |
150+
| params_flow.rb:172:10:172:19 | ...[...] | semmle.label | ...[...] |
151+
| params_flow.rb:176:10:176:15 | call to params : | semmle.label | call to params : |
152+
| params_flow.rb:176:10:176:19 | ...[...] | semmle.label | ...[...] |
142153
subpaths
143154
#select
144155
| params_flow.rb:3:10:3:19 | ...[...] | params_flow.rb:3:10:3:15 | call to params : | params_flow.rb:3:10:3:19 | ...[...] | $@ | params_flow.rb:3:10:3:15 | call to params : | call to params : |
@@ -182,6 +193,9 @@ subpaths
182193
| params_flow.rb:144:10:144:44 | call to with_defaults! | params_flow.rb:144:10:144:15 | call to params : | params_flow.rb:144:10:144:44 | call to with_defaults! | $@ | params_flow.rb:144:10:144:15 | call to params : | call to params : |
183194
| params_flow.rb:145:10:145:38 | call to with_defaults! | params_flow.rb:145:32:145:37 | call to params : | params_flow.rb:145:10:145:38 | call to with_defaults! | $@ | params_flow.rb:145:32:145:37 | call to params : | call to params : |
184195
| params_flow.rb:149:10:149:10 | p | params_flow.rb:148:22:148:27 | call to params : | params_flow.rb:149:10:149:10 | p | $@ | params_flow.rb:148:22:148:27 | call to params : | call to params : |
185-
| params_flow.rb:157:10:157:19 | ...[...] | params_flow.rb:157:10:157:15 | call to params : | params_flow.rb:157:10:157:19 | ...[...] | $@ | params_flow.rb:157:10:157:15 | call to params : | call to params : |
186-
| params_flow.rb:163:10:163:19 | ...[...] | params_flow.rb:163:10:163:15 | call to params : | params_flow.rb:163:10:163:19 | ...[...] | $@ | params_flow.rb:163:10:163:15 | call to params : | call to params : |
187-
| params_flow.rb:167:10:167:19 | ...[...] | params_flow.rb:167:10:167:15 | call to params : | params_flow.rb:167:10:167:19 | ...[...] | $@ | params_flow.rb:167:10:167:15 | call to params : | call to params : |
196+
| params_flow.rb:153:10:153:44 | call to reverse_update | params_flow.rb:153:10:153:15 | call to params : | params_flow.rb:153:10:153:44 | call to reverse_update | $@ | params_flow.rb:153:10:153:15 | call to params : | call to params : |
197+
| params_flow.rb:154:10:154:38 | call to reverse_update | params_flow.rb:154:32:154:37 | call to params : | params_flow.rb:154:10:154:38 | call to reverse_update | $@ | params_flow.rb:154:32:154:37 | call to params : | call to params : |
198+
| params_flow.rb:158:10:158:10 | p | params_flow.rb:157:22:157:27 | call to params : | params_flow.rb:158:10:158:10 | p | $@ | params_flow.rb:157:22:157:27 | call to params : | call to params : |
199+
| params_flow.rb:166:10:166:19 | ...[...] | params_flow.rb:166:10:166:15 | call to params : | params_flow.rb:166:10:166:19 | ...[...] | $@ | params_flow.rb:166:10:166:15 | call to params : | call to params : |
200+
| params_flow.rb:172:10:172:19 | ...[...] | params_flow.rb:172:10:172:15 | call to params : | params_flow.rb:172:10:172:19 | ...[...] | $@ | params_flow.rb:172:10:172:15 | call to params : | call to params : |
201+
| params_flow.rb:176:10:176:19 | ...[...] | params_flow.rb:176:10:176:15 | call to params : | params_flow.rb:176:10:176:19 | ...[...] | $@ | params_flow.rb:176:10:176:15 | call to params : | call to params : |

‎ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,17 +149,26 @@ def m32
149149
sink p # $hasTaintFlow
150150
end
151151

152+
def m33
153+
sink params.reverse_update({a: 1, b: 2}) # $hasTaintFlow
154+
sink {a: 1}.reverse_update(params) # $hasTaintFlow
155+
156+
p = {a: 1}
157+
p.reverse_update(params)
158+
sink p # $hasTaintFlow
159+
end
160+
152161
include Mixin
153162
end
154163

155164
module Mixin
156-
def m33
165+
def m34
157166
sink params[:x] # $hasTaintFlow
158167
end
159168
end
160169

161170
class Subclass < MyController
162-
def m34
171+
def m35
163172
sink params[:x] # $hasTaintFlow
164173
end
165174

0 commit comments

Comments
 (0)