Ruby: additional unsafe deserialization sinks for ox, oj

Author: p-

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

@@ -126,42 +126,42 @@ module UnsafeDeserialization {
  }
  }
 
- private string getAKnownOjModeName(boolean isSafe) {
- result = ["compat", "custom", "json", "null", "rails", "strict", "wab"] and isSafe = true
- or
- result = "object" and isSafe = false
- }
-
- private predicate isOjModePair(CfgNodes::ExprNodes::PairCfgNode p, string modeValue) {
+ /**
+ * Oj/Ox common code to establish whether a deserialization mode is defined.
+ */
+ private predicate isModePair(CfgNodes::ExprNodes::PairCfgNode p, string modeValue) {
  p.getKey().getConstantValue().isStringlikeValue("mode") and
  DataFlow::exprNode(p.getValue()).getALocalSource().getConstantValue().isSymbol(modeValue)
  }
 
  /**
  * A node representing a hash that contains the key `:mode`.
  */
- private class OjOptionsHashWithModeKey extends DataFlow::Node {
+ private class OptionsHashWithModeKey extends DataFlow::Node {
  private string modeValue;
 
- OjOptionsHashWithModeKey() {
+ OptionsHashWithModeKey() {
  exists(DataFlow::LocalSourceNode options |
  options.flowsTo(this) and
- isOjModePair(options.asExpr().(CfgNodes::ExprNodes::HashLiteralCfgNode).getAKeyValuePair(),
+ isModePair(options.asExpr().(CfgNodes::ExprNodes::HashLiteralCfgNode).getAKeyValuePair(),
  modeValue)
  )
  }
 
  /**
- * Holds if this hash node contains a `:mode` key whose value is one known
- * to be `isSafe` with untrusted data.
+ * Holds if this hash node contains the `:mode`
  */
- predicate hasKnownMode(boolean isSafe) { modeValue = getAKnownOjModeName(isSafe) }
+ predicate hasKnownMode(string mode) { modeValue = mode }
+ }
 
- /**
- * Holds if this hash node contains a `:mode` key whose value is one of the
- * `Oj` modes known to be safe to use with untrusted data.
- */
- predicate hasSafeMode() { this.hasKnownMode(true) }
+ /**
+ * Unsafe deserialization utilizing the Oj gem
+ * See: https://github.com/ohler55/oj
+ */
+ private string getAKnownOjModeName(boolean isSafe) {
+ result = ["compat", "custom", "json", "null", "rails", "strict", "wab"] and isSafe = true
+ or
+ result = "object" and isSafe = false
  }
 
  /**
@@ -197,9 +197,9 @@ module UnsafeDeserialization {
  */
  predicate hasExplicitKnownMode(boolean isSafe) {
  exists(DataFlow::Node arg, int i | i >= 1 and arg = this.getArgument(i) |
- arg.(OjOptionsHashWithModeKey).hasKnownMode(isSafe)
+ arg.(OptionsHashWithModeKey).hasKnownMode(getAKnownOjModeName(isSafe))
  or
- isOjModePair(arg.asExpr(), getAKnownOjModeName(isSafe))
+ isModePair(arg.asExpr(), getAKnownOjModeName(isSafe))
  )
  }
  }
@@ -223,13 +223,109 @@ module UnsafeDeserialization {
  // anywhere to set the default options to a known safe mode.
  not ojLoad.hasExplicitKnownMode(_) and
  not exists(SetOjDefaultOptionsCall setOpts |
- setOpts.getValue().(OjOptionsHashWithModeKey).hasSafeMode()
+ setOpts.getValue().(OptionsHashWithModeKey).hasKnownMode(getAKnownOjModeName(true))
  )
  )
  )
  }
  }
 
+ /**
+ * The first argument in a call to `Oj.object_load`, always considered as a
+ * sink for unsafe deserialization. (global and local mode options are ignored)
+ */
+ class OjObjectLoadArgument extends Sink {
+ OjObjectLoadArgument() {
+ this = API::getTopLevelMember("Oj").getAMethodCall("object_load").getArgument(0)
+ }
+ }
+
+ /**
+ * Unsafe deserialization utilizing the Ox gem
+ * See: https://github.com/ohler55/ox
+ */
+ private string getAKnownOxModeName(boolean isSafe) {
+ result = ["generic", "limited", "hash", "hash_no_attrs"] and isSafe = true
+ or
+ result = "object" and isSafe = false
+ }
+
+ /**
+ * A call node that sets `Ox.default_options`.
+ *
+ * ```rb
+ * Ox.default_options = { mode: :limited, effort: :tolerant }
+ * ```
+ */
+ private class SetOxDefaultOptionsCall extends DataFlow::CallNode {
+ SetOxDefaultOptionsCall() {
+ this = API::getTopLevelMember("Ox").getAMethodCall("default_options=")
+ }
+
+ /**
+ * Gets the value being assigned to `Ox.default_options`.
+ */
+ DataFlow::Node getValue() {
+ result.asExpr() =
+ this.getArgument(0).asExpr().(CfgNodes::ExprNodes::AssignExprCfgNode).getRhs()
+ }
+ }
+
+ /**
+ * A call to `Ox.load`.
+ */
+ private class OxLoadCall extends DataFlow::CallNode {
+ OxLoadCall() { this = API::getTopLevelMember("Ox").getAMethodCall("load") }
+
+ /**
+ * Holds if this call to `Ox.load` includes an explicit options hash
+ * argument that sets the mode to one that is known to be `isSafe`.
+ */
+ predicate hasExplicitKnownMode(boolean isSafe) {
+ exists(DataFlow::Node arg, int i | i >= 1 and arg = this.getArgument(i) |
+ arg.(OptionsHashWithModeKey).hasKnownMode(getAKnownOxModeName(isSafe))
+ or
+ isModePair(arg.asExpr(), getAKnownOxModeName(isSafe))
+ )
+ }
+ }
+
+ /**
+ * An argument in a call to `Ox.load` where the mode is `:object` (not the default),
+ * considered a sink for unsafe deserialization.
+ */
+ class UnsafeOxLoadArgument extends Sink {
+ UnsafeOxLoadArgument() {
+ exists(OxLoadCall oxLoad |
+ this = oxLoad.getArgument(0) and
+ // Exclude calls that explicitly pass a safe mode option.
+ not oxLoad.hasExplicitKnownMode(true) and
+ (
+ // Sinks to include:
+ // - Calls with an explicit, unsafe mode option.
+ oxLoad.hasExplicitKnownMode(false)
+ or
+ // - Calls with no explicit mode option and there exists a call
+ // anywhere to set the default options to an unsafe mode (object).
+ not oxLoad.hasExplicitKnownMode(_) and
+ exists(SetOxDefaultOptionsCall setOpts |
+ setOpts.getValue().(OptionsHashWithModeKey).hasKnownMode(getAKnownOxModeName(false))
+ )
+ )
+ )
+ }
+ }
+
+ /**
+ * The first argument in a call to `Ox.parse_obj`, always considered as a
+ * sink for unsafe deserialization.
+ */
+ class OxParseObjArgument extends Sink {
+ OxParseObjArgument() {
+ this = API::getTopLevelMember("Ox").getAMethodCall("parse_obj").getArgument(0)
+ }
+ }
+
  /**
  * An argument in a call to `Plist.parse_xml` where `marshal` is `true` (which is
  * the default), considered a sink for unsafe deserialization.

ruby/ql/src/change-notes/2023-10-19-unsafe-deserialization-sinks.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Added new unsafe deserialization sinks for the ox gem.
+* Added an additional unsafe deserialization sink for the oj gem.

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

@@ -27,6 +27,13 @@ method. In <code>psych</code> version 4.0.0 and above, the <code>load</code> met
 safely be used.
 </p>
 
+<p>
+If deserializing an untrusted XML document using the <code>ox</code> gem,
+do not use <code>parse_obj</code> and <code>load</code> using the non-default :object mode.
+Instead use the <code>load</code> method in the default mode or better explicitly set a safe
+mode such as :hash. 
+</p>
+
 <p>
 To safely deserialize <a href="https://en.wikipedia.org/wiki/Property_list">Property List</a>
 files using the <code>plist</code> gem, ensure that you pass <code>marshal: false</code>
@@ -37,8 +44,8 @@ when calling <code>Plist.parse_xml</code>.
 <example>
 <p>
 The following example calls the <code>Marshal.load</code>,
-<code>JSON.load</code>, <code>YAML.load</code>, and <code>Oj.load</code> methods
-on data from an HTTP request. Since these methods are capable of deserializing
+<code>JSON.load</code>, <code>YAML.load</code>, <code>Oj.load</code> and <code>Ox.parse_obj</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"/>

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

@@ -23,4 +23,9 @@ def oj_example
  object = Oj.load params[:json]
  # ...
  end
+
+ def ox_example
+ object = Ox.parse_obj params[:xml]
+ # ...
+ end
 end

ruby/ql/test/query-tests/security/cwe-502/ox-global-options/OxGlobalOptions.rb

@@ -0,0 +1,16 @@
+require "ox"
+
+class UsersController < ActionController::Base
+ # BAD - Ox.load is unsafe when the mode :object is set globally
+ def route0
+ xml_data = params[:key]
+ object = Ox.load xml_data
+ end
+
+ # GOOD - the unsafe mode set globally is overridden with an insecure mode passed as
+ # a call argument
+ def route1
+ xml_data = params[:key]
+ object = Ox.load xml_data, mode: :generic
+ end
+end

ruby/ql/test/query-tests/security/cwe-502/ox-global-options/Startup.rb

@@ -0,0 +1,5 @@
+require "ox"
+
+# Set the default mode for the Ox library to use the :object mode, which makes
+# Ox.load unsafe for untrusted data.
+Ox.default_options = { :mode => :object }

ruby/ql/test/query-tests/security/cwe-502/ox-global-options/UnsafeDeserialization.expected

@@ -0,0 +1,12 @@
+edges
+| OxGlobalOptions.rb:6:5:6:12 | xml_data | OxGlobalOptions.rb:7:22:7:29 | xml_data |
+| OxGlobalOptions.rb:6:16:6:21 | call to params | OxGlobalOptions.rb:6:16:6:27 | ...[...] |
+| OxGlobalOptions.rb:6:16:6:27 | ...[...] | OxGlobalOptions.rb:6:5:6:12 | xml_data |
+nodes
+| OxGlobalOptions.rb:6:5:6:12 | xml_data | semmle.label | xml_data |
+| OxGlobalOptions.rb:6:16:6:21 | call to params | semmle.label | call to params |
+| OxGlobalOptions.rb:6:16:6:27 | ...[...] | semmle.label | ...[...] |
+| OxGlobalOptions.rb:7:22:7:29 | xml_data | semmle.label | xml_data |
+subpaths
+#select
+| OxGlobalOptions.rb:7:22:7:29 | xml_data | OxGlobalOptions.rb:6:16:6:21 | call to params | OxGlobalOptions.rb:7:22:7:29 | xml_data | Unsafe deserialization depends on a $@. | OxGlobalOptions.rb:6:16:6:21 | call to params | user-provided value |

ruby/ql/test/query-tests/security/cwe-502/ox-global-options/UnsafeDeserialization.qlref

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