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