Skip to content

Commit 1410574

Browse files
am0o0hmac
authored andcommitted
make seperate steps for YAML.parse* and use getAsuccessor*() to reach final to_ruby method call, All parts have Rewritten with API graphs exclusively
1 parent c22cbf5 commit 1410574

File tree

4 files changed

+66
-13
lines changed

4 files changed

+66
-13
lines changed

ruby/ql/lib/codeql/ruby/frameworks/Yaml.qll

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,25 +10,48 @@ private import codeql.ruby.ApiGraphs
1010
* A taint step related to the result of `YAML.parse` calls, or similar.
1111
* In the following example, this step will propagate taint from
1212
* `source` to `sink`:
13-
*
13+
* this contains two seperate steps:
1414
* ```rb
1515
* x = source
16-
* result = YAML.parse(x)
17-
* sink result.to_ruby # Unsafe call
16+
* sink = YAML.parse(x)
17+
* ```
18+
* By second step
19+
* source is a Successor of `YAML.parse(x)`
20+
* which ends with `to_ruby` or an Element of `to_ruby`
21+
* ```ruby
22+
* sink source.to_ruby # Unsafe call
1823
* ```
1924
*/
2025
private class YamlParseStep extends AdditionalTaintStep {
2126
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
22-
exists(DataFlow::CallNode yamlParserMethod |
23-
succ = yamlParserMethod.getAMethodCall("to_ruby") and
27+
exists(API::Node yamlParserMethod |
28+
succ = yamlParserMethod.getReturn().asSource() and
2429
(
25-
yamlParserMethod = yamlNode().getAMethodCall(["parse", "parse_stream"]) and
26-
pred = [yamlParserMethod.getArgument(0), yamlParserMethod.getKeywordArgument("yaml")]
30+
yamlParserMethod = yamlNode().getMethod(["parse", "parse_stream"]) and
31+
pred =
32+
[yamlParserMethod.getParameter(0), yamlParserMethod.getKeywordParameter("yaml")].asSink()
2733
or
28-
yamlParserMethod = yamlNode().getAMethodCall("parse_file") and
29-
pred = [yamlParserMethod.getArgument(0), yamlParserMethod.getKeywordArgument("filename")]
34+
yamlParserMethod = yamlNode().getMethod("parse_file") and
35+
pred =
36+
[yamlParserMethod.getParameter(0), yamlParserMethod.getKeywordParameter("filename")]
37+
.asSink()
3038
)
3139
)
40+
or
41+
exists(API::Node yamlParserMethod |
42+
succ =
43+
[
44+
yamlParserMethod.getASuccessor*().getMethod("to_ruby").getReturn().asSource(),
45+
yamlParserMethod
46+
.getASuccessor*()
47+
.getMethod("to_ruby")
48+
.getReturn()
49+
.getAnElement()
50+
.asSource()
51+
] and
52+
yamlParserMethod = yamlNode().getMethod(["parse", "parse_stream", "parse_file"]) and
53+
pred = yamlParserMethod.getReturn().asSource()
54+
)
3255
}
3356
}
3457

ruby/ql/lib/codeql/ruby/security/UnsafeDeserializationCustomizations.qll

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,12 +97,26 @@ module UnsafeDeserialization {
9797

9898
/**
9999
* An argument in a call to `YAML.parse*`, considered a sink for unsafe deserialization
100-
* if there is a call to `to_ruby` on the returned value.
100+
* if there is a call to `to_ruby` on the returned value of any Successor.
101101
*/
102102
class YamlParseArgument extends Sink {
103103
YamlParseArgument() {
104104
this =
105-
yamlNode().getAMethodCall(["parse", "parse_stream", "parse_file"]).getAMethodCall("to_ruby")
105+
[
106+
yamlNode()
107+
.getMethod(["parse", "parse_stream", "parse_file"])
108+
.getASuccessor*()
109+
.getMethod("to_ruby")
110+
.getReturn()
111+
.asSource(),
112+
yamlNode()
113+
.getMethod(["parse", "parse_stream", "parse_file"])
114+
.getASuccessor*()
115+
.getMethod("to_ruby")
116+
.getReturn()
117+
.getAnElement()
118+
.asSource()
119+
]
106120
}
107121
}
108122

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
queries/security/cwe-502/UnsafeDeserialization.ql
1+
queries/security/cwe-502/UnsafeDeserialization.ql

ruby/ql/test/query-tests/security/cwe-502/unsafe-deserialization/YAMLUnsafeDeserialization.rb

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,23 @@ def example
1515
parse_output.to_ruby
1616
Psych.parse(params[:yaml_string]).to_ruby
1717
Psych.parse_file(params[:yaml_file]).to_ruby
18-
18+
parsed_yaml.children.each do |child|
19+
puts child.to_ruby
20+
end
21+
Psych.parse_stream(params[:yaml_string]) do |document|
22+
puts document.to_ruby
23+
end
24+
parsed_yaml.children.first.to_ruby
25+
parsed_yaml = Psych.parse_stream(params[:yaml_string])
26+
content = parsed_yaml.children[0].children[0].children
27+
parsed = parsed_yaml.to_ruby[0]
28+
parsed = content.to_ruby[0]
29+
Psych.parse(params[:yaml_string]).children[0].to_ruby
30+
# FP
31+
parsed_yaml = Psych2.parse_stream(params[:yaml_string])
32+
content = parsed_yaml.children[0].children[0].children
33+
parsed = parsed_yaml.to_ruby
34+
parsed = parsed_yaml.to_ruby[0]
1935
end
2036
end
2137

0 commit comments

Comments
 (0)