Skip to content

Commit fad73d7

Browse files
aibaarsaibaars
authored
Merge pull request #13307 from hmac/amammad-ruby-YAMLunsafeLoad
Ruby: Add YAML unsafe deserialization sinks
2 parents 6b9c00e + 7324d17 commit fad73d7

File tree

8 files changed

+179
-2
lines changed

8 files changed

+179
-2
lines changed

‎ruby/ql/lib/change-notes/2023-05-27-unsafe-deserialization.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+
* Additional sinks for `rb/unsafe-deserialization` have been added. This includes various methods from the `yaml` and `plist` gems, which deserialize YAML and Property List data, respectively.

‎ruby/ql/lib/codeql/ruby/Frameworks.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,4 +34,5 @@ private import codeql.ruby.frameworks.Twirp
3434
private import codeql.ruby.frameworks.Sqlite3
3535
private import codeql.ruby.frameworks.Mysql2
3636
private import codeql.ruby.frameworks.Pg
37+
private import codeql.ruby.frameworks.Yaml
3738
private import codeql.ruby.frameworks.Sequel

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/**
2+
* Provides modeling for the `YAML` and `Psych` libraries.
3+
*/
4+
5+
private import codeql.ruby.dataflow.FlowSteps
6+
private import codeql.ruby.DataFlow
7+
private import codeql.ruby.ApiGraphs
8+
9+
/**
10+
* A taint step related to the result of `YAML.parse` calls, or similar.
11+
* In the following example, this step will propagate taint from
12+
* `source` to `sink`:
13+
*
14+
* ```rb
15+
* x = source
16+
* result = YAML.parse(x)
17+
* sink result.to_ruby # Unsafe call
18+
* ```
19+
*/
20+
private class YamlParseStep extends AdditionalTaintStep {
21+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
22+
exists(DataFlow::CallNode yamlParserMethod |
23+
succ = yamlParserMethod.getAMethodCall("to_ruby") and
24+
(
25+
yamlParserMethod = yamlNode().getAMethodCall(["parse", "parse_stream"]) and
26+
pred = [yamlParserMethod.getArgument(0), yamlParserMethod.getKeywordArgument("yaml")]
27+
or
28+
yamlParserMethod = yamlNode().getAMethodCall("parse_file") and
29+
pred = [yamlParserMethod.getArgument(0), yamlParserMethod.getKeywordArgument("filename")]
30+
)
31+
)
32+
}
33+
}
34+
35+
private API::Node yamlNode() { result = API::getTopLevelMember(["YAML", "Psych"]) }

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

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,13 +75,34 @@ module UnsafeDeserialization {
7575
}
7676

7777
/**
78-
* An argument in a call to `YAML.load`, considered a sink
78+
* An argument in a call to `YAML.unsafe_*` and `YAML.load_stream` , considered a sink
7979
* for unsafe deserialization. The `YAML` module is an alias of `Psych` in
8080
* recent versions of Ruby.
8181
*/
8282
class YamlLoadArgument extends Sink {
8383
YamlLoadArgument() {
84-
this = API::getTopLevelMember(["YAML", "Psych"]).getAMethodCall("load").getArgument(0)
84+
// Note: this is safe in psych/yaml >= 4.0.0.
85+
this = yamlNode().getAMethodCall("load").getArgument(0)
86+
or
87+
this =
88+
yamlNode().getAMethodCall(["unsafe_load_file", "unsafe_load", "load_stream"]).getArgument(0)
89+
or
90+
this = yamlNode().getAMethodCall(["unsafe_load", "load_stream"]).getKeywordArgument("yaml")
91+
or
92+
this = yamlNode().getAMethodCall("unsafe_load_file").getKeywordArgument("filename")
93+
}
94+
}
95+
96+
private API::Node yamlNode() { result = API::getTopLevelMember(["YAML", "Psych"]) }
97+
98+
/**
99+
* 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.
101+
*/
102+
class YamlParseArgument extends Sink {
103+
YamlParseArgument() {
104+
this =
105+
yamlNode().getAMethodCall(["parse", "parse_stream", "parse_file"]).getAMethodCall("to_ruby")
85106
}
86107
}
87108

@@ -208,4 +229,23 @@ module UnsafeDeserialization {
208229
)
209230
}
210231
}
232+
233+
/**
234+
* An argument in a call to `Plist.parse_xml` where `marshal` is `true` (which is
235+
* the default), considered a sink for unsafe deserialization.
236+
*/
237+
class UnsafePlistParsexmlArgument extends Sink {
238+
UnsafePlistParsexmlArgument() {
239+
exists(DataFlow::CallNode plistParseXml |
240+
plistParseXml = API::getTopLevelMember("Plist").getAMethodCall("parse_xml")
241+
|
242+
this = [plistParseXml.getArgument(0), plistParseXml.getKeywordArgument("filename_or_xml")] and
243+
(
244+
plistParseXml.getKeywordArgument("marshal").getConstantValue().isBoolean(true)
245+
or
246+
plistParseXml.getNumberOfArguments() = 1
247+
)
248+
)
249+
}
250+
}
211251
}

‎ruby/ql/src/queries/security/cwe-502/UnsafeDeserialization.qhelp

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,21 @@ libraries that support it, such as the Ruby standard library's <code>JSON</code>
1717
module, ensure that the parser is configured to disable
1818
deserialization of arbitrary objects.
1919
</p>
20+
21+
<p>
22+
If deserializing an untrusted YAML document using the <code>psych</code> gem,
23+
prefer the <code>safe_load</code> and <code>safe_load_file</code> methods over
24+
<code>load</code> and <code>load_file</code>, as the former will safely
25+
handle untrusted data. Avoid passing untrusted data to the <code>load_stream</code>
26+
method. In <code>psych</code> version 4.0.0 and above, the <code>load</code> method can
27+
safely be used.
28+
</p>
29+
30+
<p>
31+
To safely deserialize <a href="https://en.wikipedia.org/wiki/Property_list">Property List</a>
32+
files using the <code>plist</code> gem, ensure that you pass <code>marshal: false</code>
33+
when calling <code>Plist.parse_xml</code>.
34+
</p>
2035
</recommendation>
2136

2237
<example>
@@ -27,6 +42,7 @@ on data from an HTTP request. Since these methods are capable of deserializing
2742
to arbitrary objects, this is inherently unsafe.
2843
</p>
2944
<sample src="examples/UnsafeDeserializationBad.rb"/>
45+
3046
<p>
3147
Using <code>JSON.parse</code> and <code>YAML.safe_load</code> instead, as in the
3248
following example, removes the vulnerability. Similarly, calling
@@ -55,6 +71,10 @@ Ruby documentation: <a href="https://ruby-doc.org/stdlib-3.0.2/libdoc/json/rdoc/
5571
<li>
5672
Ruby documentation: <a href="https://ruby-doc.org/stdlib-3.0.2/libdoc/yaml/rdoc/YAML.html#module-YAML-label-Security">security guidance on the YAML library</a>.
5773
</li>
74+
<li>
75+
You can read that how unsafe yaml load methods can lead to code executions:
76+
<a href="https://devcraft.io/2021/01/07/universal-deserialisation-gadget-for-ruby-2-x-3-x.html">Universal Deserialisation Gadget for Ruby 2.x-3.x </a>.
77+
</li>
5878
</references>
5979

6080
</qhelp>

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
require 'yaml'
2+
class UsersController < ActionController::Base
3+
def example
4+
# not safe
5+
result = Plist.parse_xml(params[:yaml_string])
6+
result = Plist.parse_xml(params[:yaml_string], marshal: true)
7+
8+
# safe
9+
result = Plist.parse_xml(params[:yaml_string], marshal: false)
10+
end
11+
end
12+
13+

‎ruby/ql/test/query-tests/security/cwe-502/unsafe-deserialization/UnsafeDeserialization.expected

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
edges
2+
| PlistUnsafeDeserialization.rb:5:30:5:35 | call to params | PlistUnsafeDeserialization.rb:5:30:5:49 | ...[...] |
3+
| PlistUnsafeDeserialization.rb:6:30:6:35 | call to params | PlistUnsafeDeserialization.rb:6:30:6:49 | ...[...] |
24
| UnsafeDeserialization.rb:10:5:10:19 | serialized_data | UnsafeDeserialization.rb:11:27:11:41 | serialized_data |
35
| UnsafeDeserialization.rb:10:23:10:50 | call to decode64 | UnsafeDeserialization.rb:10:5:10:19 | serialized_data |
46
| UnsafeDeserialization.rb:10:39:10:44 | call to params | UnsafeDeserialization.rb:10:39:10:50 | ...[...] |
@@ -29,7 +31,21 @@ edges
2931
| UnsafeDeserialization.rb:87:5:87:13 | yaml_data | UnsafeDeserialization.rb:88:25:88:33 | yaml_data |
3032
| UnsafeDeserialization.rb:87:17:87:22 | call to params | UnsafeDeserialization.rb:87:17:87:28 | ...[...] |
3133
| UnsafeDeserialization.rb:87:17:87:28 | ...[...] | UnsafeDeserialization.rb:87:5:87:13 | yaml_data |
34+
| YAMLUnsafeDeserialization.rb:5:16:5:21 | call to params | YAMLUnsafeDeserialization.rb:5:16:5:35 | ...[...] |
35+
| YAMLUnsafeDeserialization.rb:11:23:11:28 | call to params | YAMLUnsafeDeserialization.rb:11:23:11:42 | ...[...] |
36+
| YAMLUnsafeDeserialization.rb:12:28:12:33 | call to params | YAMLUnsafeDeserialization.rb:12:28:12:45 | ...[...] |
37+
| YAMLUnsafeDeserialization.rb:13:23:13:28 | call to params | YAMLUnsafeDeserialization.rb:13:23:13:42 | ...[...] |
38+
| YAMLUnsafeDeserialization.rb:14:39:14:44 | call to params | YAMLUnsafeDeserialization.rb:14:39:14:58 | ...[...] |
39+
| YAMLUnsafeDeserialization.rb:14:39:14:58 | ...[...] | YAMLUnsafeDeserialization.rb:15:5:15:24 | call to to_ruby |
40+
| YAMLUnsafeDeserialization.rb:16:17:16:22 | call to params | YAMLUnsafeDeserialization.rb:16:17:16:36 | ...[...] |
41+
| YAMLUnsafeDeserialization.rb:16:17:16:36 | ...[...] | YAMLUnsafeDeserialization.rb:16:5:16:45 | call to to_ruby |
42+
| YAMLUnsafeDeserialization.rb:17:22:17:27 | call to params | YAMLUnsafeDeserialization.rb:17:22:17:39 | ...[...] |
43+
| YAMLUnsafeDeserialization.rb:17:22:17:39 | ...[...] | YAMLUnsafeDeserialization.rb:17:5:17:48 | call to to_ruby |
3244
nodes
45+
| PlistUnsafeDeserialization.rb:5:30:5:35 | call to params | semmle.label | call to params |
46+
| PlistUnsafeDeserialization.rb:5:30:5:49 | ...[...] | semmle.label | ...[...] |
47+
| PlistUnsafeDeserialization.rb:6:30:6:35 | call to params | semmle.label | call to params |
48+
| PlistUnsafeDeserialization.rb:6:30:6:49 | ...[...] | semmle.label | ...[...] |
3349
| UnsafeDeserialization.rb:10:5:10:19 | serialized_data | semmle.label | serialized_data |
3450
| UnsafeDeserialization.rb:10:23:10:50 | call to decode64 | semmle.label | call to decode64 |
3551
| UnsafeDeserialization.rb:10:39:10:44 | call to params | semmle.label | call to params |
@@ -74,8 +90,27 @@ nodes
7490
| UnsafeDeserialization.rb:98:24:98:32 | call to read | semmle.label | call to read |
7591
| UnsafeDeserialization.rb:101:24:101:27 | call to gets | semmle.label | call to gets |
7692
| UnsafeDeserialization.rb:104:24:104:32 | call to readlines | semmle.label | call to readlines |
93+
| YAMLUnsafeDeserialization.rb:5:16:5:21 | call to params | semmle.label | call to params |
94+
| YAMLUnsafeDeserialization.rb:5:16:5:35 | ...[...] | semmle.label | ...[...] |
95+
| YAMLUnsafeDeserialization.rb:11:23:11:28 | call to params | semmle.label | call to params |
96+
| YAMLUnsafeDeserialization.rb:11:23:11:42 | ...[...] | semmle.label | ...[...] |
97+
| YAMLUnsafeDeserialization.rb:12:28:12:33 | call to params | semmle.label | call to params |
98+
| YAMLUnsafeDeserialization.rb:12:28:12:45 | ...[...] | semmle.label | ...[...] |
99+
| YAMLUnsafeDeserialization.rb:13:23:13:28 | call to params | semmle.label | call to params |
100+
| YAMLUnsafeDeserialization.rb:13:23:13:42 | ...[...] | semmle.label | ...[...] |
101+
| YAMLUnsafeDeserialization.rb:14:39:14:44 | call to params | semmle.label | call to params |
102+
| YAMLUnsafeDeserialization.rb:14:39:14:58 | ...[...] | semmle.label | ...[...] |
103+
| YAMLUnsafeDeserialization.rb:15:5:15:24 | call to to_ruby | semmle.label | call to to_ruby |
104+
| YAMLUnsafeDeserialization.rb:16:5:16:45 | call to to_ruby | semmle.label | call to to_ruby |
105+
| YAMLUnsafeDeserialization.rb:16:17:16:22 | call to params | semmle.label | call to params |
106+
| YAMLUnsafeDeserialization.rb:16:17:16:36 | ...[...] | semmle.label | ...[...] |
107+
| YAMLUnsafeDeserialization.rb:17:5:17:48 | call to to_ruby | semmle.label | call to to_ruby |
108+
| YAMLUnsafeDeserialization.rb:17:22:17:27 | call to params | semmle.label | call to params |
109+
| YAMLUnsafeDeserialization.rb:17:22:17:39 | ...[...] | semmle.label | ...[...] |
77110
subpaths
78111
#select
112+
| PlistUnsafeDeserialization.rb:5:30:5:49 | ...[...] | PlistUnsafeDeserialization.rb:5:30:5:35 | call to params | PlistUnsafeDeserialization.rb:5:30:5:49 | ...[...] | Unsafe deserialization depends on a $@. | PlistUnsafeDeserialization.rb:5:30:5:35 | call to params | user-provided value |
113+
| PlistUnsafeDeserialization.rb:6:30:6:49 | ...[...] | PlistUnsafeDeserialization.rb:6:30:6:35 | call to params | PlistUnsafeDeserialization.rb:6:30:6:49 | ...[...] | Unsafe deserialization depends on a $@. | PlistUnsafeDeserialization.rb:6:30:6:35 | call to params | user-provided value |
79114
| UnsafeDeserialization.rb:11:27:11:41 | serialized_data | UnsafeDeserialization.rb:10:39:10:44 | call to params | UnsafeDeserialization.rb:11:27:11:41 | serialized_data | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:10:39:10:44 | call to params | user-provided value |
80115
| UnsafeDeserialization.rb:17:30:17:44 | serialized_data | UnsafeDeserialization.rb:16:39:16:44 | call to params | UnsafeDeserialization.rb:17:30:17:44 | serialized_data | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:16:39:16:44 | call to params | user-provided value |
81116
| UnsafeDeserialization.rb:23:24:23:32 | json_data | UnsafeDeserialization.rb:22:17:22:22 | call to params | UnsafeDeserialization.rb:23:24:23:32 | json_data | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:22:17:22:22 | call to params | user-provided value |
@@ -91,3 +126,10 @@ subpaths
91126
| UnsafeDeserialization.rb:98:24:98:32 | call to read | UnsafeDeserialization.rb:98:24:98:32 | call to read | UnsafeDeserialization.rb:98:24:98:32 | call to read | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:98:24:98:32 | call to read | value from stdin |
92127
| UnsafeDeserialization.rb:101:24:101:27 | call to gets | UnsafeDeserialization.rb:101:24:101:27 | call to gets | UnsafeDeserialization.rb:101:24:101:27 | call to gets | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:101:24:101:27 | call to gets | value from stdin |
93128
| UnsafeDeserialization.rb:104:24:104:32 | call to readlines | UnsafeDeserialization.rb:104:24:104:32 | call to readlines | UnsafeDeserialization.rb:104:24:104:32 | call to readlines | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:104:24:104:32 | call to readlines | value from stdin |
129+
| YAMLUnsafeDeserialization.rb:5:16:5:35 | ...[...] | YAMLUnsafeDeserialization.rb:5:16:5:21 | call to params | YAMLUnsafeDeserialization.rb:5:16:5:35 | ...[...] | Unsafe deserialization depends on a $@. | YAMLUnsafeDeserialization.rb:5:16:5:21 | call to params | user-provided value |
130+
| YAMLUnsafeDeserialization.rb:11:23:11:42 | ...[...] | YAMLUnsafeDeserialization.rb:11:23:11:28 | call to params | YAMLUnsafeDeserialization.rb:11:23:11:42 | ...[...] | Unsafe deserialization depends on a $@. | YAMLUnsafeDeserialization.rb:11:23:11:28 | call to params | user-provided value |
131+
| YAMLUnsafeDeserialization.rb:12:28:12:45 | ...[...] | YAMLUnsafeDeserialization.rb:12:28:12:33 | call to params | YAMLUnsafeDeserialization.rb:12:28:12:45 | ...[...] | Unsafe deserialization depends on a $@. | YAMLUnsafeDeserialization.rb:12:28:12:33 | call to params | user-provided value |
132+
| YAMLUnsafeDeserialization.rb:13:23:13:42 | ...[...] | YAMLUnsafeDeserialization.rb:13:23:13:28 | call to params | YAMLUnsafeDeserialization.rb:13:23:13:42 | ...[...] | Unsafe deserialization depends on a $@. | YAMLUnsafeDeserialization.rb:13:23:13:28 | call to params | user-provided value |
133+
| YAMLUnsafeDeserialization.rb:15:5:15:24 | call to to_ruby | YAMLUnsafeDeserialization.rb:14:39:14:44 | call to params | YAMLUnsafeDeserialization.rb:15:5:15:24 | call to to_ruby | Unsafe deserialization depends on a $@. | YAMLUnsafeDeserialization.rb:14:39:14:44 | call to params | user-provided value |
134+
| YAMLUnsafeDeserialization.rb:16:5:16:45 | call to to_ruby | YAMLUnsafeDeserialization.rb:16:17:16:22 | call to params | YAMLUnsafeDeserialization.rb:16:5:16:45 | call to to_ruby | Unsafe deserialization depends on a $@. | YAMLUnsafeDeserialization.rb:16:17:16:22 | call to params | user-provided value |
135+
| YAMLUnsafeDeserialization.rb:17:5:17:48 | call to to_ruby | YAMLUnsafeDeserialization.rb:17:22:17:27 | call to params | YAMLUnsafeDeserialization.rb:17:5:17:48 | call to to_ruby | Unsafe deserialization depends on a $@. | YAMLUnsafeDeserialization.rb:17:22:17:27 | call to params | user-provided value |

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
require 'yaml'
2+
class UsersController < ActionController::Base
3+
def example
4+
# safe
5+
Psych.load(params[:yaml_string])
6+
Psych.load_file(params[:yaml_file])
7+
Psych.parse_stream(params[:yaml_string])
8+
Psych.parse(params[:yaml_string])
9+
Psych.parse_file(params[:yaml_file])
10+
# unsafe
11+
Psych.unsafe_load(params[:yaml_string])
12+
Psych.unsafe_load_file(params[:yaml_file])
13+
Psych.load_stream(params[:yaml_string])
14+
parse_output = Psych.parse_stream(params[:yaml_string])
15+
parse_output.to_ruby
16+
Psych.parse(params[:yaml_string]).to_ruby
17+
Psych.parse_file(params[:yaml_file]).to_ruby
18+
19+
end
20+
end
21+
22+

0 commit comments

Comments
 (0)