Skip to content

Commit 6f7d4fe

Browse files
nickrolfenickrolfe
authored
Merge pull request #287 from github/unsafe-deserialization
rb/unsafe-deserialization query
2 parents 8af12a1 + 2ddca2c commit 6f7d4fe

File tree

9 files changed

+299
-0
lines changed

9 files changed

+299
-0
lines changed

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

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about unsafe
3+
* deserialization, as well as extension points for adding your own.
4+
*/
5+
6+
private import ruby
7+
private import codeql.ruby.ApiGraphs
8+
private import codeql.ruby.CFG
9+
private import codeql.ruby.DataFlow
10+
private import codeql.ruby.dataflow.RemoteFlowSources
11+
12+
module UnsafeDeserialization {
13+
/**
14+
* A data flow source for unsafe deserialization vulnerabilities.
15+
*/
16+
abstract class Source extends DataFlow::Node { }
17+
18+
/**
19+
* A data flow sink for unsafe deserialization vulnerabilities.
20+
*/
21+
abstract class Sink extends DataFlow::Node { }
22+
23+
/**
24+
* A sanitizer for unsafe deserialization vulnerabilities.
25+
*/
26+
abstract class Sanitizer extends DataFlow::Node { }
27+
28+
/**
29+
* Additional taint steps for "unsafe deserialization" vulnerabilities.
30+
*/
31+
predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
32+
base64DecodeTaintStep(fromNode, toNode)
33+
}
34+
35+
/** A source of remote user input, considered as a flow source for unsafe deserialization. */
36+
class RemoteFlowSourceAsSource extends Source {
37+
RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
38+
}
39+
40+
/**
41+
* An argument in a call to `Marshal.load` or `Marshal.restore`, considered a
42+
* sink for unsafe deserialization.
43+
*/
44+
class MarshalLoadOrRestoreArgument extends Sink {
45+
MarshalLoadOrRestoreArgument() {
46+
this = API::getTopLevelMember("Marshal").getAMethodCall(["load", "restore"]).getArgument(0)
47+
}
48+
}
49+
50+
/**
51+
* An argument in a call to `YAML.load`, considered a sink for unsafe
52+
* deserialization.
53+
*/
54+
class YamlLoadArgument extends Sink {
55+
YamlLoadArgument() {
56+
this = API::getTopLevelMember("YAML").getAMethodCall("load").getArgument(0)
57+
}
58+
}
59+
60+
/**
61+
* An argument in a call to `JSON.load` or `JSON.restore`, considered a sink
62+
* for unsafe deserialization.
63+
*/
64+
class JsonLoadArgument extends Sink {
65+
JsonLoadArgument() {
66+
this = API::getTopLevelMember("JSON").getAMethodCall(["load", "restore"]).getArgument(0)
67+
}
68+
}
69+
70+
/**
71+
* `Base64.decode64` propagates taint from its argument to its return value.
72+
*/
73+
predicate base64DecodeTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
74+
exists(DataFlow::CallNode callNode |
75+
callNode =
76+
API::getTopLevelMember("Base64")
77+
.getAMethodCall(["decode64", "strict_decode64", "urlsafe_decode64"])
78+
|
79+
fromNode = callNode.getArgument(0) and
80+
toNode = callNode
81+
)
82+
}
83+
}

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
/**
2+
* Provides a taint-tracking configuration for reasoning about unsafe deserialization.
3+
*
4+
* Note, for performance reasons: only import this file if
5+
* `UnsafeDeserialization::Configuration` is needed, otherwise
6+
* `UnsafeDeserializationCustomizations` should be imported instead.
7+
*/
8+
9+
private import ruby
10+
private import codeql.ruby.DataFlow
11+
private import codeql.ruby.TaintTracking
12+
import UnsafeDeserializationCustomizations
13+
14+
/**
15+
* A taint-tracking configuration for reasoning about unsafe deserialization.
16+
*/
17+
class Configuration extends TaintTracking::Configuration {
18+
Configuration() { this = "UnsafeDeserialization" }
19+
20+
override predicate isSource(DataFlow::Node source) {
21+
source instanceof UnsafeDeserialization::Source
22+
}
23+
24+
override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeDeserialization::Sink }
25+
26+
override predicate isSanitizer(DataFlow::Node node) {
27+
super.isSanitizer(node) or
28+
node instanceof UnsafeDeserialization::Sanitizer
29+
}
30+
31+
override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
32+
UnsafeDeserialization::isAdditionalTaintStep(fromNode, toNode)
33+
}
34+
}

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

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>
6+
Deserializing untrusted data using any method that allows the construction of
7+
arbitrary objects is easily exploitable and, in many cases, allows an attacker
8+
to execute arbitrary code.
9+
</p>
10+
</overview>
11+
12+
<recommendation>
13+
<p>
14+
Avoid deserialization of untrusted data if possible. If the architecture permits
15+
it, use serialization formats that cannot represent arbitarary objects. For
16+
libraries that support it, such as the Ruby standard library's <code>JSON</code>
17+
module, ensure that the parser is configured to disable
18+
deserialization of arbitrary objects.
19+
</p>
20+
</recommendation>
21+
22+
<example>
23+
<p>
24+
The following example calls the <code>Marshal.load</code>, <code>JSON.load</code>, and
25+
<code>YAML.load</code> methods on data from an HTTP request. Since these methods
26+
are capable of deserializing to arbitrary objects, this is inherently unsafe.
27+
</p>
28+
<sample src="examples/UnsafeDeserializationBad.rb"/>
29+
<p>
30+
Using <code>JSON.parse</code> and <code>YAML.safe_load</code> instead, as in the
31+
following example, removes the vulnerability. Note that there is no safe way to
32+
deserialize untrusted data using <code>Marshal</code>.
33+
</p>
34+
<sample src="examples/UnsafeDeserializationGood.rb"/>
35+
</example>
36+
37+
<references>
38+
39+
<li>
40+
OWASP vulnerability description:
41+
<a href="https://www.owasp.org/index.php/Deserialization_of_untrusted_data">deserialization of untrusted data</a>.
42+
</li>
43+
<li>
44+
Ruby documentation: <a href="https://docs.ruby-lang.org/en/3.0.0/doc/security_rdoc.html">guidance on deserializing objects safely</a>.
45+
</li>
46+
<li>
47+
Ruby documentation: <a href="https://ruby-doc.org/core-3.0.2/Marshal.html#module-Marshal-label-Security+considerations">security guidance on the Marshal library</a>.
48+
</li>
49+
<li>
50+
Ruby documentation: <a href="https://ruby-doc.org/stdlib-3.0.2/libdoc/json/rdoc/JSON.html#method-i-load">security guidance on JSON.load</a>.
51+
</li>
52+
<li>
53+
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>.
54+
</li>
55+
</references>
56+
57+
</qhelp>

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* @name Deserialization of user-controlled data
3+
* @description Deserializing user-controlled data may allow attackers to
4+
* execute arbitrary code.
5+
* @kind path-problem
6+
* @problem.severity warning
7+
* @security-severity 9.8
8+
* @precision high
9+
* @id rb/unsafe-deserialization
10+
* @tags security
11+
* external/cwe/cwe-502
12+
*/
13+
14+
import ruby
15+
import DataFlow::PathGraph
16+
import codeql.ruby.DataFlow
17+
import codeql.ruby.security.UnsafeDeserializationQuery
18+
19+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
20+
where cfg.hasFlowPath(source, sink)
21+
select sink.getNode(), source, sink, "Unsafe deserialization of $@.", source.getNode(), "user input"

‎ql/src/queries/security/cwe-502/examples/UnsafeDeserializationBad.rb

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
require 'json'
2+
require 'yaml'
3+
4+
class UserController < ActionController::Base
5+
def marshal_example
6+
data = Base64.decode64 params[:data]
7+
object = Marshal.load data
8+
# ...
9+
end
10+
11+
def json_example
12+
object = JSON.load params[:json]
13+
# ...
14+
end
15+
16+
def yaml_example
17+
object = YAML.load params[:yaml]
18+
# ...
19+
end
20+
end

‎ql/src/queries/security/cwe-502/examples/UnsafeDeserializationGood.rb

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
require 'json'
2+
3+
class UserController < ActionController::Base
4+
def safe_json_example
5+
object = JSON.parse params[:json]
6+
# ...
7+
end
8+
9+
def safe_yaml_example
10+
object = YAML.safe_load params[:yaml]
11+
# ...
12+
end
13+
end

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
edges
2+
| UnsafeDeserialization.rb:8:39:8:44 | call to params : | UnsafeDeserialization.rb:9:27:9:41 | serialized_data |
3+
| UnsafeDeserialization.rb:14:39:14:44 | call to params : | UnsafeDeserialization.rb:15:30:15:44 | serialized_data |
4+
| UnsafeDeserialization.rb:20:17:20:22 | call to params : | UnsafeDeserialization.rb:21:24:21:32 | json_data |
5+
| UnsafeDeserialization.rb:26:17:26:22 | call to params : | UnsafeDeserialization.rb:27:27:27:35 | json_data |
6+
| UnsafeDeserialization.rb:38:17:38:22 | call to params : | UnsafeDeserialization.rb:39:24:39:32 | yaml_data |
7+
nodes
8+
| UnsafeDeserialization.rb:8:39:8:44 | call to params : | semmle.label | call to params : |
9+
| UnsafeDeserialization.rb:9:27:9:41 | serialized_data | semmle.label | serialized_data |
10+
| UnsafeDeserialization.rb:14:39:14:44 | call to params : | semmle.label | call to params : |
11+
| UnsafeDeserialization.rb:15:30:15:44 | serialized_data | semmle.label | serialized_data |
12+
| UnsafeDeserialization.rb:20:17:20:22 | call to params : | semmle.label | call to params : |
13+
| UnsafeDeserialization.rb:21:24:21:32 | json_data | semmle.label | json_data |
14+
| UnsafeDeserialization.rb:26:17:26:22 | call to params : | semmle.label | call to params : |
15+
| UnsafeDeserialization.rb:27:27:27:35 | json_data | semmle.label | json_data |
16+
| UnsafeDeserialization.rb:38:17:38:22 | call to params : | semmle.label | call to params : |
17+
| UnsafeDeserialization.rb:39:24:39:32 | yaml_data | semmle.label | yaml_data |
18+
#select
19+
| UnsafeDeserialization.rb:9:27:9:41 | serialized_data | UnsafeDeserialization.rb:8:39:8:44 | call to params : | UnsafeDeserialization.rb:9:27:9:41 | serialized_data | Unsafe deserialization of $@. | UnsafeDeserialization.rb:8:39:8:44 | call to params | user input |
20+
| UnsafeDeserialization.rb:15:30:15:44 | serialized_data | UnsafeDeserialization.rb:14:39:14:44 | call to params : | UnsafeDeserialization.rb:15:30:15:44 | serialized_data | Unsafe deserialization of $@. | UnsafeDeserialization.rb:14:39:14:44 | call to params | user input |
21+
| UnsafeDeserialization.rb:21:24:21:32 | json_data | UnsafeDeserialization.rb:20:17:20:22 | call to params : | UnsafeDeserialization.rb:21:24:21:32 | json_data | Unsafe deserialization of $@. | UnsafeDeserialization.rb:20:17:20:22 | call to params | user input |
22+
| UnsafeDeserialization.rb:27:27:27:35 | json_data | UnsafeDeserialization.rb:26:17:26:22 | call to params : | UnsafeDeserialization.rb:27:27:27:35 | json_data | Unsafe deserialization of $@. | UnsafeDeserialization.rb:26:17:26:22 | call to params | user input |
23+
| UnsafeDeserialization.rb:39:24:39:32 | yaml_data | UnsafeDeserialization.rb:38:17:38:22 | call to params : | UnsafeDeserialization.rb:39:24:39:32 | yaml_data | Unsafe deserialization of $@. | UnsafeDeserialization.rb:38:17:38:22 | call to params | user input |

‎ql/test/query-tests/security/cwe-502/UnsafeDeserialization.qlref

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

‎ql/test/query-tests/security/cwe-502/UnsafeDeserialization.rb

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
require "base64"
2+
require "json"
3+
require "yaml"
4+
5+
class UsersController < ActionController::Base
6+
# BAD
7+
def route0
8+
serialized_data = Base64.decode64 params[:key]
9+
object = Marshal.load serialized_data
10+
end
11+
12+
# BAD
13+
def route1
14+
serialized_data = Base64.decode64 params[:key]
15+
object = Marshal.restore serialized_data
16+
end
17+
18+
# BAD
19+
def route2
20+
json_data = params[:key]
21+
object = JSON.load json_data
22+
end
23+
24+
# BAD
25+
def route3
26+
json_data = params[:key]
27+
object = JSON.restore json_data
28+
end
29+
30+
# GOOD - JSON.parse is safe to use on untrusted data
31+
def route4
32+
json_data = params[:key]
33+
object = JSON.parse json_data
34+
end
35+
36+
# BAD
37+
def route5
38+
yaml_data = params[:key]
39+
object = YAML.load yaml_data
40+
end
41+
42+
# GOOD
43+
def route6
44+
yaml_data = params[:key]
45+
object = YAML.safe_load yaml_data
46+
end
47+
end

0 commit comments

Comments
 (0)