add Ruby YAML unsafe_* methods as sink

ruby/ql/lib/codeql/ruby/Frameworks.qll

@@ -28,3 +28,4 @@ private import codeql.ruby.frameworks.PosixSpawn
 private import codeql.ruby.frameworks.StringFormatters
 private import codeql.ruby.frameworks.Json
 private import codeql.ruby.frameworks.Twirp
+private import codeql.ruby.frameworks.Yaml

ruby/ql/lib/codeql/ruby/frameworks/Yaml.qll

@@ -0,0 +1,43 @@
+/**
+ * Provides modeling for the `YAML` and `Psych` libraries.
+ */
+
+private import codeql.ruby.dataflow.FlowSteps
+private import codeql.ruby.DataFlow
+private import codeql.ruby.ApiGraphs
+
+/**
+ * A taint step related to the result of `YAML.parse` calls, or similar.
+ *In the following example, this step will propagate taint from
+ *`source` to `sink`:
+ *
+ *```rb
+ *x = source
+ *result = YAML.parse(x)
+ *sink result.to_ruby # Unsafe call
+ * ```
+ */
+private class YamlParseStep extends AdditionalTaintStep {
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(DataFlow::CallNode yamlParserMethod |
+      yamlParserMethod = yamlNode().getAMethodCall(["parse", "parse_stream"]) and
+      (
+        pred = yamlParserMethod.getArgument(0) or
+        pred = yamlParserMethod.getKeywordArgument("yaml")
+      ) and
+      succ = yamlParserMethod.getAMethodCall("to_ruby")
+      or
+      yamlParserMethod = yamlNode().getAMethodCall("parse_file") and
+      (
+        pred = yamlParserMethod.getArgument(0) or
+        pred = yamlParserMethod.getKeywordArgument("filename")
+      ) and
+      succ = yamlParserMethod.getAMethodCall("to_ruby")
+    )
+  }
+}
+
+/**
+ * YAML/Psych Top level Class member
+ */
+private API::Node yamlNode() { result = API::getTopLevelMember(["YAML", "Psych"]) }

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

@@ -75,13 +75,40 @@ module UnsafeDeserialization {
   }
 
   /**
-   * An argument in a call to `YAML.load`, considered a sink
+   * An argument in a call to `YAML.unsafe_*` and `YAML.load_stream` , considered sinks
    * for unsafe deserialization. The `YAML` module is an alias of `Psych` in
    * recent versions of Ruby.
+   * the `this = yamlNode().getAMethodCall("load").getArgument(0)` is safe
+   *  in psych/yaml library after [v4.0.0](https://github.com/ruby/psych/releases/tag/v4.0.0), so it will be removed in future.
    */
   class YamlLoadArgument extends Sink {
     YamlLoadArgument() {
-      this = API::getTopLevelMember(["YAML", "Psych"]).getAMethodCall("load").getArgument(0)
+      this = yamlNode().getAMethodCall("load").getArgument(0)
+      or
+      this =
+        yamlNode().getAMethodCall(["unsafe_load_file", "unsafe_load", "load_stream"]).getArgument(0)
+      or
+      this = yamlNode().getAMethodCall(["unsafe_load", "load_stream"]).getKeywordArgument("yaml")
+      or
+      this = yamlNode().getAMethodCall("unsafe_load_file").getKeywordArgument("filename")
+    }
+  }
+
+  /**
+   * YAML/Psych Top level Class member
+   */
+  private API::Node yamlNode() { result = API::getTopLevelMember(["YAML", "Psych"]) }
+
+  /**
+   * An argument in a call to `YAML.parse*`, considered sinks
+   * for unsafe deserialization if there is a call to `to_ruby` on returned value of them,
+   * so this need some additional taint steps. The `YAML` module is an alias of `Psych` in
+   * recent versions of Ruby.
+   */
+  class YamlParseArgument extends Sink {
+    YamlParseArgument() {
+      this =
+        yamlNode().getAMethodCall(["parse", "parse_stream", "parse_file"]).getAMethodCall("to_ruby")
     }
   }
 
@@ -208,4 +235,22 @@ module UnsafeDeserialization {
       )
     }
   }
+
+  /**
+   * An argument in a call to `Plist.parse_xml` where the marshal is `true` (which is
+   * the default), considered a sink for unsafe deserialization.
+   */
+  class UnsafePlistParsexmlArgument extends Sink {
+    UnsafePlistParsexmlArgument() {
+      exists(DataFlow::CallNode plistParseXml |
+        plistParseXml = API::getTopLevelMember("Plist").getAMethodCall("parse_xml")
+      |
+        this = [plistParseXml.getArgument(0), plistParseXml.getKeywordArgument("filename_or_xml")] and
+        plistParseXml.getKeywordArgument("marshal").getConstantValue().isBoolean(true)
+        or
+        this = [plistParseXml.getArgument(0), plistParseXml.getKeywordArgument("filename_or_xml")] and
+        plistParseXml.getNumberOfArguments() = 1
+      )
+    }
+  }
 }

ruby/ql/src/experimental/cwe-502/PlistUnsafeDeserialization.qhelp

@@ -0,0 +1,24 @@
+<!DOCTYPE qhelp SYSTEM "qhelp.dtd">
+<qhelp>
+    <overview>
+        <p>
+            Processing an unvalidated user input can allow an attacker to execute arbitrary code in your application.
+            Unsafe deserializing the malicious serialized xml document through the Plist library, making it possible to execute some code or execute arbitrary code with the help of a complete gadget chain.
+        </p>
+    </overview>
+    <recommendation>
+        <p>
+        This vulnerability in Plist can be prevented by calling <code>Plist.parse_xml FileOrXmlString, marshal: false</code>.
+        </p>
+    </recommendation>
+    <example>
+        <p>In the example below, you can see safe and unsafe Plist dangerous method calls that can be abused by a remote user input. You can use "marshal: false" as an arugument for <code>Plist.parse_xml</code> to use it safe.
+        </p>
+        <sample src="PlistUnsafeDeserialization.rb" />
+    </example>
+    <references>
+    <li>
+    Security considerations from library documentation: <a href="https://github.com/patsplat/plist#label-Security+considerations">patsplat/plist Repository</a>.
+    </li>
+    </references>
+</qhelp>

ruby/ql/src/experimental/cwe-502/PlistUnsafeDeserialization.ql

@@ -0,0 +1,57 @@
+/**
+ * @name Unsafe Deserialization of user-controlled data by Plist
+ * @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/plist-unsafe-deserialization
+ * @tags security
+ *       experimental
+ *       external/cwe/cwe-502
+ */
+
+import codeql.ruby.ApiGraphs
+import codeql.ruby.DataFlow
+import codeql.ruby.TaintTracking
+import codeql.ruby.CFG
+import DataFlow::PathGraph
+import codeql.ruby.security.UnsafeDeserializationCustomizations
+
+abstract class PlistUnsafeSinks extends DataFlow::Node { }
+
+/**
+ * An argument in a call to `Plist.parse_xml` where the marshal is `true` (which is
+ * the default), considered a sink for unsafe deserialization.
+ */
+class UnsafePlistParsexmlArgument extends PlistUnsafeSinks {
+  UnsafePlistParsexmlArgument() {
+    exists(DataFlow::CallNode plistParseXml |
+      plistParseXml = API::getTopLevelMember("Plist").getAMethodCall("parse_xml")
+    |
+      this = [plistParseXml.getArgument(0), plistParseXml.getKeywordArgument("filename_or_xml")] and
+      plistParseXml.getKeywordArgument("marshal").getConstantValue().isBoolean(true)
+      or
+      this = [plistParseXml.getArgument(0), plistParseXml.getKeywordArgument("filename_or_xml")] and
+      plistParseXml.getNumberOfArguments() = 1
+    )
+  }
+}
+
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "PlistUnsafeDeserialization" }
+
+  override predicate isSource(DataFlow::Node source) {
+    // to detect CVE-2021-33575, we should uncomment following line instead of current UnsafeDeserialization::Source
+    // source instanceof DataFlow::LocalSourceNode
+    source instanceof UnsafeDeserialization::Source
+  }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof PlistUnsafeSinks }
+}
+
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Unsafe deserialization depends on a $@.", source.getNode(),
+  "potentially untrusted source"

ruby/ql/src/experimental/cwe-502/PlistUnsafeDeserialization.rb

@@ -0,0 +1,17 @@
+require 'plist'
+class UsersController < ActionController::Base
+  def example
+    # not safe
+    config = true
+    result = Plist.parse_xml(params[:yaml_string])
+    result = Plist.parse_xml(params[:yaml_string], marshal: config)
+    result = Plist.parse_xml(params[:yaml_string], marshal: true)
+
+    # safe
+    config = false
+    result = Plist.parse_xml(params[:yaml_string], marshal: false)
+    result = Plist.parse_xml(params[:yaml_string], marshal: config)
+  end
+end
+
+

ruby/ql/src/experimental/cwe-502/YAMLUnsafeDeserialization.qhelp

@@ -0,0 +1,26 @@
+<!DOCTYPE qhelp SYSTEM "qhelp.dtd">
+<qhelp>
+    <overview>
+        <p>
+            Processing an unvalidated user input can allow an attacker to execute arbitrary code in your application.
+            Unsafe deserializing the malicious serialized yaml document through the Psych (YAML) library, making it possible to execute some code or execute arbitrary code with the help of a complete gadget chain.
+        </p>
+    </overview>
+    <recommendation>
+        <p>
+            After Psych(YAML) 4.0.0, the load method is same as safe_load method.
+            This vulnerability can be prevented by using YAML.load (same as <code>YAML.safe_load</code>), <code>YAML.load_file</code> (same as <code>YAML.safe_load_file</code>) instead of <code>YAML.unsafe_*</code> methods.
+            Be careful that <code>YAML.load_stream</code> don't use safe_load method, Also Be careful the <code>to_ruby</code> method of Psych get called on a trusted parsed (<code>YAML.parse*</code>) yaml document.
+        </p>
+    </recommendation>
+    <example>
+        <p>In the example below, you can see safe and unsafe methods get called by a remote user input. You can give correct authorization to users, or you can use safe methods for loading yaml documents.</p>
+        <sample src="YAMLUnsafeDeserialization.rb" />
+    </example>
+    <references>
+    <li>
+    You can read that how unsafe yaml load methods can lead to code executions.
+    <a href="https://devcraft.io/2021/01/07/universal-deserialisation-gadget-for-ruby-2-x-3-x.html">Universal Deserialisation Gadget for Ruby 2.x-3.x </a>.
+    </li>
+    </references>
+</qhelp>

ruby/ql/src/experimental/cwe-502/YAMLUnsafeDeserialization.ql

@@ -0,0 +1,88 @@
+/**
+ * @name Unsafe Deserialization of user-controlled data by YAML
+ * @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/YAML-unsafe-deserialization
+ * @tags security
+ *       experimental
+ *       external/cwe/cwe-502
+ */
+
+import codeql.ruby.ApiGraphs
+import codeql.ruby.DataFlow
+import codeql.ruby.TaintTracking
+import DataFlow::PathGraph
+import codeql.ruby.security.UnsafeDeserializationCustomizations
+
+abstract class YamlUnsafeSinks extends DataFlow::Node { }
+
+class YamlUnsafeArgument extends YamlUnsafeSinks {
+  YamlUnsafeArgument() {
+    this =
+      API::getTopLevelMember(["YAML", "Psych"])
+          .getAMethodCall(["unsafe_load_file", "unsafe_load", "load_stream"])
+          .getArgument(0)
+    or
+    this =
+      API::getTopLevelMember(["YAML", "Psych"])
+          .getAMethodCall(["unsafe_load", "load_stream"])
+          .getKeywordArgument("yaml")
+    or
+    this =
+      API::getTopLevelMember(["YAML", "Psych"])
+          .getAMethodCall("unsafe_load_file")
+          .getKeywordArgument("filename")
+    or
+    this =
+      API::getTopLevelMember(["YAML", "Psych"])
+          .getAMethodCall(["parse", "parse_stream", "parse_file"])
+          .getAMethodCall("to_ruby")
+  }
+}
+
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "YamlUnsafeDeserialization" }
+
+  override predicate isSource(DataFlow::Node source) {
+    // to detect CVE-2022-32224, we should uncomment following line instead of current UnsafeDeserialization::Source
+    // source instanceof DataFlow::LocalSourceNode
+    source instanceof UnsafeDeserialization::Source
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    // after changing the isSource for detecting CVE-2022-32224
+    // uncomment the following line only see the CVE sink not other files similar sinks
+    // sink.getLocation().getFile().toString().matches("%yaml_column%") and
+    sink instanceof YamlUnsafeSinks
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    exists(DataFlow::CallNode yaml_parser_methods |
+      yaml_parser_methods =
+        API::getTopLevelMember(["YAML", "Psych"]).getAMethodCall(["parse", "parse_stream"]) and
+      (
+        nodeFrom = yaml_parser_methods.getArgument(0) or
+        nodeFrom = yaml_parser_methods.getKeywordArgument("yaml")
+      ) and
+      nodeTo = yaml_parser_methods.getAMethodCall("to_ruby")
+    )
+    or
+    exists(DataFlow::CallNode yaml_parser_methods |
+      yaml_parser_methods = API::getTopLevelMember(["YAML", "Psych"]).getAMethodCall("parse_file") and
+      (
+        nodeFrom = yaml_parser_methods.getArgument(0) or
+        nodeFrom = yaml_parser_methods.getKeywordArgument("filename")
+      ) and
+      nodeTo = yaml_parser_methods.getAMethodCall("to_ruby")
+    )
+  }
+}
+
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Unsafe deserialization depends on a $@.", source.getNode(),
+  "potentially untrusted source"

ruby/ql/src/experimental/cwe-502/YAMLUnsafeDeserialization.rb

@@ -0,0 +1,22 @@
+require 'yaml'
+class UsersController < ActionController::Base
+  def example
+    # safe
+    Psych.load(params[:yaml_string])
+    Psych.load_file(params[:yaml_file])
+    Psych.parse_stream(params[:yaml_string])
+    Psych.parse(params[:yaml_string])
+    Psych.parse_file(params[:yaml_file])
+    # unsafe
+    Psych.unsafe_load(params[:yaml_string])
+    Psych.unsafe_load_file(params[:yaml_file])
+    Psych.load_stream(params[:yaml_string])
+    parse_output = Psych.parse_stream(params[:yaml_string])
+    parse_output.to_ruby
+    Psych.parse(params[:yaml_string]).to_ruby
+    Psych.parse_file(params[:yaml_file]).to_ruby
+
+  end
+end
+
+