Merge pull request #311 from github/nickrolfe/oj

Consider Oj.load a sink for unsafe deserialization

Author: nickrolfe

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

@@ -67,6 +67,113 @@ 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(Pair p, string modeValue) {
+    p.getKey().getValueText() = "mode" and
+    exists(DataFlow::LocalSourceNode symbolLiteral, DataFlow::Node value |
+      symbolLiteral.asExpr().getExpr().(SymbolLiteral).getValueText() = modeValue and
+      symbolLiteral.flowsTo(value) and
+      value.asExpr().getExpr() = p.getValue()
+    )
+  }
+
+  /**
+   * A node representing a hash that contains the key `:mode`.
+   */
+  private class OjOptionsHashWithModeKey extends DataFlow::Node {
+    private string modeValue;
+
+    OjOptionsHashWithModeKey() {
+      exists(DataFlow::LocalSourceNode options |
+        options.flowsTo(this) and
+        isOjModePair(options.asExpr().getExpr().(HashLiteral).getAKeyValuePair(), modeValue)
+      )
+    }
+
+    /**
+     * Holds if this hash node contains a `:mode` key whose value is one known
+     * to be `isSafe` with untrusted data.
+     */
+    predicate hasKnownMode(boolean isSafe) { modeValue = getAKnownOjModeName(isSafe) }
+
+    /**
+     * 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) }
+  }
+
+  /**
+   * A call node that sets `Oj.default_options`.
+   *
+   * ```rb
+   * Oj.default_options = { allow_blank: true, mode: :compat }
+   * ```
+   */
+  private class SetOjDefaultOptionsCall extends DataFlow::CallNode {
+    SetOjDefaultOptionsCall() {
+      this = API::getTopLevelMember("Oj").getAMethodCall("default_options=")
+    }
+
+    /**
+     * Gets the value being assigned to `Oj.default_options`.
+     */
+    DataFlow::Node getValue() {
+      result.asExpr() =
+        this.getArgument(0).asExpr().(CfgNodes::ExprNodes::AssignExprCfgNode).getRhs()
+    }
+  }
+
+  /**
+   * A call to `Oj.load`.
+   */
+  private class OjLoadCall extends DataFlow::CallNode {
+    OjLoadCall() { this = API::getTopLevelMember("Oj").getAMethodCall("load") }
+
+    /**
+     * Holds if this call to `Oj.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.(OjOptionsHashWithModeKey).hasKnownMode(isSafe)
+        or
+        isOjModePair(arg.asExpr().getExpr(), getAKnownOjModeName(isSafe))
+      )
+    }
+  }
+
+  /**
+   * An argument in a call to `Oj.load` where the mode is `:object` (which is
+   * the default), considered a sink for unsafe deserialization.
+   */
+  class UnsafeOjLoadArgument extends Sink {
+    UnsafeOjLoadArgument() {
+      exists(OjLoadCall ojLoad |
+        this = ojLoad.getArgument(0) and
+        // Exclude calls that explicitly pass a safe mode option.
+        not ojLoad.hasExplicitKnownMode(true) and
+        (
+          // Sinks to include:
+          // - Calls with an explicit, unsafe mode option.
+          ojLoad.hasExplicitKnownMode(false)
+          or
+          // - Calls with no explicit mode option, unless there exists a call
+          // anywhere to set the default options to a known safe mode.
+          not ojLoad.hasExplicitKnownMode(_) and
+          not exists(SetOjDefaultOptionsCall setOpts |
+            setOpts.getValue().(OjOptionsHashWithModeKey).hasSafeMode()
+          )
+        )
+      )
+    }
+  }
+
   /**
    * `Base64.decode64` propagates taint from its argument to its return value.
    */

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

@@ -21,15 +21,18 @@ deserialization of arbitrary objects.
 
 <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.
+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
+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>.
+following example, removes the vulnerability. Similarly, calling
+<code>Oj.load</code> with any mode other than <code>:object</code> is safe, as
+is calling <code>Oj.safe_load</code>. Note that there is no safe way to deserialize
+untrusted data using <code>Marshal</code>.
 </p>
 <sample src="examples/UnsafeDeserializationGood.rb"/>
 </example>

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

@@ -1,5 +1,6 @@
 require 'json'
 require 'yaml'
+require 'oj'
 
 class UserController < ActionController::Base
   def marshal_example
@@ -17,4 +18,9 @@ def yaml_example
     object = YAML.load params[:yaml]
     # ...
   end
+
+  def oj_example
+    object = Oj.load params[:json]
+    # ...
+  end
 end

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

@@ -10,4 +10,11 @@ def safe_yaml_example
     object = YAML.safe_load params[:yaml]
     # ...
   end
+
+  def safe_oj_example
+    object = Oj.load params[:yaml], { mode: :strict }
+    # or
+    object = Oj.safe_load params[:yaml]
+    # ...
+  end
 end

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

@@ -0,0 +1,16 @@
+require "oj"
+
+class UsersController < ActionController::Base
+  # GOOD - Oj.load is safe when any mode other than :object is set globally
+  def route0
+    json_data = params[:key]
+    object = Oj.load json_data
+  end
+
+  # BAD - the safe mode set globally is overridden with an unsafe mode passed as
+  # a call argument
+  def route1
+    json_data = params[:key]
+    object = Oj.load json_data, mode: :object
+  end
+end

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

@@ -0,0 +1,5 @@
+require "oj"
+
+# Set the default mode for the Oj library to use the :compat mode, which makes
+# Oj.load safe for untrusted data.
+Oj.default_options = { :mode => :compat }

ql/test/query-tests/security/cwe-502/oj-global-options/OjGlobalOptions.rb

@@ -0,0 +1,8 @@
+edges
+| OjGlobalOptions.rb:13:17:13:22 | call to params :  | OjGlobalOptions.rb:14:22:14:30 | json_data |
+nodes
+| OjGlobalOptions.rb:13:17:13:22 | call to params :  | semmle.label | call to params :  |
+| OjGlobalOptions.rb:14:22:14:30 | json_data | semmle.label | json_data |
+subpaths
+#select
+| OjGlobalOptions.rb:14:22:14:30 | json_data | OjGlobalOptions.rb:13:17:13:22 | call to params :  | OjGlobalOptions.rb:14:22:14:30 | json_data | Unsafe deserialization of $@. | OjGlobalOptions.rb:13:17:13:22 | call to params | user input |