Merge pull request #287 from github/unsafe-deserialization

rb/unsafe-deserialization query

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

@@ -0,0 +1,83 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about unsafe
+ * deserialization, as well as extension points for adding your own.
+ */
+
+private import ruby
+private import codeql.ruby.ApiGraphs
+private import codeql.ruby.CFG
+private import codeql.ruby.DataFlow
+private import codeql.ruby.dataflow.RemoteFlowSources
+
+module UnsafeDeserialization {
+  /**
+   * A data flow source for unsafe deserialization vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for unsafe deserialization vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer for unsafe deserialization vulnerabilities.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * Additional taint steps for "unsafe deserialization" vulnerabilities.
+   */
+  predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
+    base64DecodeTaintStep(fromNode, toNode)
+  }
+
+  /** A source of remote user input, considered as a flow source for unsafe deserialization. */
+  class RemoteFlowSourceAsSource extends Source {
+    RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
+  }
+
+  /**
+   * An argument in a call to `Marshal.load` or `Marshal.restore`, considered a
+   * sink for unsafe deserialization.
+   */
+  class MarshalLoadOrRestoreArgument extends Sink {
+    MarshalLoadOrRestoreArgument() {
+      this = API::getTopLevelMember("Marshal").getAMethodCall(["load", "restore"]).getArgument(0)
+    }
+  }
+
+  /**
+   * An argument in a call to `YAML.load`, considered a sink for unsafe
+   * deserialization.
+   */
+  class YamlLoadArgument extends Sink {
+    YamlLoadArgument() {
+      this = API::getTopLevelMember("YAML").getAMethodCall("load").getArgument(0)
+    }
+  }
+
+  /**
+   * An argument in a call to `JSON.load` or `JSON.restore`, considered a sink
+   * for unsafe deserialization.
+   */
+  class JsonLoadArgument extends Sink {
+    JsonLoadArgument() {
+      this = API::getTopLevelMember("JSON").getAMethodCall(["load", "restore"]).getArgument(0)
+    }
+  }
+
+  /**
+   * `Base64.decode64` propagates taint from its argument to its return value.
+   */
+  predicate base64DecodeTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
+    exists(DataFlow::CallNode callNode |
+      callNode =
+        API::getTopLevelMember("Base64")
+            .getAMethodCall(["decode64", "strict_decode64", "urlsafe_decode64"])
+    |
+      fromNode = callNode.getArgument(0) and
+      toNode = callNode
+    )
+  }
+}

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

@@ -0,0 +1,34 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about unsafe deserialization.
+ *
+ * Note, for performance reasons: only import this file if
+ * `UnsafeDeserialization::Configuration` is needed, otherwise
+ * `UnsafeDeserializationCustomizations` should be imported instead.
+ */
+
+private import ruby
+private import codeql.ruby.DataFlow
+private import codeql.ruby.TaintTracking
+import UnsafeDeserializationCustomizations
+
+/**
+ * A taint-tracking configuration for reasoning about unsafe deserialization.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "UnsafeDeserialization" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof UnsafeDeserialization::Source
+  }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeDeserialization::Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    super.isSanitizer(node) or
+    node instanceof UnsafeDeserialization::Sanitizer
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
+    UnsafeDeserialization::isAdditionalTaintStep(fromNode, toNode)
+  }
+}

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

@@ -0,0 +1,57 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Deserializing untrusted data using any method that allows the construction of
+arbitrary objects is easily exploitable and, in many cases, allows an attacker
+to execute arbitrary code.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Avoid deserialization of untrusted data if possible. If the architecture permits
+it, use serialization formats that cannot represent arbitarary objects. For
+libraries that support it, such as the Ruby standard library's <code>JSON</code>
+module, ensure that the parser is configured to disable
+deserialization of arbitrary objects.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example calls the <code>Marshal.load</code>, <code>JSON.load</code>, and
+<code>YAML.load</code> methods on data from an HTTP request. Since these methods
+are capable of deserializing to arbitrary objects, this is inherently unsafe.
+</p>
+<sample src="examples/UnsafeDeserializationBad.rb"/>
+<p>
+Using <code>JSON.parse</code> and <code>YAML.safe_load</code> instead, as in the
+following example, removes the vulnerability. Note that there is no safe way to
+deserialize untrusted data using <code>Marshal</code>.
+</p>
+<sample src="examples/UnsafeDeserializationGood.rb"/>
+</example>
+
+<references>
+
+<li>
+OWASP vulnerability description:
+<a href="https://www.owasp.org/index.php/Deserialization_of_untrusted_data">deserialization of untrusted data</a>.
+</li>
+<li>
+Ruby documentation: <a href="https://docs.ruby-lang.org/en/3.0.0/doc/security_rdoc.html">guidance on deserializing objects safely</a>.
+</li>
+<li>
+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>.
+</li>
+<li>
+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>.
+</li>
+<li>
+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>.
+</li>
+</references>
+
+</qhelp>

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

@@ -0,0 +1,21 @@
+/**
+ * @name Deserialization of user-controlled data
+ * @description Deserializing user-controlled data may allow attackers to
+ *              execute arbitrary code.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 9.8
+ * @precision high
+ * @id rb/unsafe-deserialization
+ * @tags security
+ *       external/cwe/cwe-502
+ */
+
+import ruby
+import DataFlow::PathGraph
+import codeql.ruby.DataFlow
+import codeql.ruby.security.UnsafeDeserializationQuery
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Unsafe deserialization of $@.", source.getNode(), "user input"

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

@@ -0,0 +1,20 @@
+require 'json'
+require 'yaml'
+
+class UserController < ActionController::Base
+  def marshal_example
+    data = Base64.decode64 params[:data]
+    object = Marshal.load data
+    # ...
+  end
+
+  def json_example
+    object = JSON.load params[:json]
+    # ...
+  end
+
+  def yaml_example
+    object = YAML.load params[:yaml]
+    # ...
+  end
+end

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

@@ -0,0 +1,13 @@
+require 'json'
+
+class UserController < ActionController::Base
+  def safe_json_example
+    object = JSON.parse params[:json]
+    # ...
+  end
+
+  def safe_yaml_example
+    object = YAML.safe_load params[:yaml]
+    # ...
+  end
+end

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

@@ -0,0 +1,23 @@
+edges
+| UnsafeDeserialization.rb:8:39:8:44 | call to params :  | UnsafeDeserialization.rb:9:27:9:41 | serialized_data |
+| UnsafeDeserialization.rb:14:39:14:44 | call to params :  | UnsafeDeserialization.rb:15:30:15:44 | serialized_data |
+| UnsafeDeserialization.rb:20:17:20:22 | call to params :  | UnsafeDeserialization.rb:21:24:21:32 | json_data |
+| UnsafeDeserialization.rb:26:17:26:22 | call to params :  | UnsafeDeserialization.rb:27:27:27:35 | json_data |
+| UnsafeDeserialization.rb:38:17:38:22 | call to params :  | UnsafeDeserialization.rb:39:24:39:32 | yaml_data |
+nodes
+| UnsafeDeserialization.rb:8:39:8:44 | call to params :  | semmle.label | call to params :  |
+| UnsafeDeserialization.rb:9:27:9:41 | serialized_data | semmle.label | serialized_data |
+| UnsafeDeserialization.rb:14:39:14:44 | call to params :  | semmle.label | call to params :  |
+| UnsafeDeserialization.rb:15:30:15:44 | serialized_data | semmle.label | serialized_data |
+| UnsafeDeserialization.rb:20:17:20:22 | call to params :  | semmle.label | call to params :  |
+| UnsafeDeserialization.rb:21:24:21:32 | json_data | semmle.label | json_data |
+| UnsafeDeserialization.rb:26:17:26:22 | call to params :  | semmle.label | call to params :  |
+| UnsafeDeserialization.rb:27:27:27:35 | json_data | semmle.label | json_data |
+| UnsafeDeserialization.rb:38:17:38:22 | call to params :  | semmle.label | call to params :  |
+| UnsafeDeserialization.rb:39:24:39:32 | yaml_data | semmle.label | yaml_data |
+#select
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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

@@ -0,0 +1 @@
+queries/security/cwe-502/UnsafeDeserialization.ql

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

@@ -0,0 +1,47 @@
+require "base64"
+require "json"
+require "yaml"
+
+class UsersController < ActionController::Base
+  # BAD
+  def route0
+    serialized_data = Base64.decode64 params[:key]
+    object = Marshal.load serialized_data
+  end
+
+  # BAD
+  def route1
+    serialized_data = Base64.decode64 params[:key]
+    object = Marshal.restore serialized_data
+  end
+
+  # BAD
+  def route2
+    json_data = params[:key]
+    object = JSON.load json_data
+  end
+
+  # BAD
+  def route3
+    json_data = params[:key]
+    object = JSON.restore json_data
+  end
+
+  # GOOD - JSON.parse is safe to use on untrusted data
+  def route4
+    json_data = params[:key]
+    object = JSON.parse json_data
+  end
+
+  # BAD
+  def route5
+    yaml_data = params[:key]
+    object = YAML.load yaml_data
+  end
+
+  # GOOD
+  def route6
+    yaml_data = params[:key]
+    object = YAML.safe_load yaml_data
+  end
+end