v2 add plist.parse_xml as a dangerous sink and enhancements on documents

Author: am0o0

ruby/ql/src/experimental/CWE-502/YAMLUnsafeYamlDeserialization.qhelp

@@ -0,0 +1,25 @@
+<!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 can be prevented by using <code>Plist.parse_xml</code>.
+        </p>
+    </recommendation>
+    <example>
+        <p>In the example below, you can see safe and unsafe call of this dangerous method 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="PlistUnsafeYamlDeserialization.rb" />
+    </example>
+    <references>
+    <li>
+    Security considerations from library documentation
+    <a href="https://github.com/patsplat/plist#label-Security+considerations">patsplat/plist</a>.
+    </li>
+    </references>
+</qhelp>

ruby/ql/src/experimental/CWE-502/YAMLUnsafeYamlDeserialization.ql

@@ -0,0 +1,68 @@
+/**
+ * @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 { }
+
+/**
+ * check whether an input argument has desired "key: value" input or not.
+ * borrowed from UnsafeDeserialization module with some changes
+ */
+predicate checkkeyBalue(CfgNodes::ExprNodes::PairCfgNode p, string key, string value) {
+  p.getKey().getConstantValue().isStringlikeValue(key) and
+  DataFlow::exprNode(p.getValue()).getALocalSource().getConstantValue().toString() = value
+}
+
+/**
+ * An argument in a call to `Plist.parse_xml` where the marshal is `true` (which is
+ * the default), considered a sink for unsafe deserialization.
+ * borrowed from UnsafeDeserialization module with some changes
+ */
+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
+      // Exclude calls that explicitly pass a safe mode option.
+      checkkeyBalue(plistParsexml.getArgument(1).asExpr(), "marshal", "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/YAMLUnsafeYamlDeserialization.rb

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

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

@@ -17,4 +17,10 @@
         <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="YAMLUnsafeYamlDeserialization.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/PlistUnsafeYamlDeserialization.ql

@@ -1,5 +1,5 @@
 /**
- * @name Deserialization of user-controlled data by YAML
+ * @name Unsafe Deserialization of user-controlled data by YAML
  * @description Deserializing user-controlled data may allow attackers to
  *              execute arbitrary code.
  * @kind path-problem
@@ -18,10 +18,10 @@ import codeql.ruby.TaintTracking
 import DataFlow::PathGraph
 import codeql.ruby.security.UnsafeDeserializationCustomizations
 
-abstract class YamlSink extends DataFlow::Node { }
+abstract class YamlUnsafeSinks extends DataFlow::Node { }
 
-class YamlUnsafeLoadArgument extends YamlSink {
-  YamlUnsafeLoadArgument() {
+class YamlUnsafeArgument extends YamlUnsafeSinks {
+  YamlUnsafeArgument() {
     this =
       API::getTopLevelMember(["YAML", "Psych"])
           .getAMethodCall(["unsafe_load_file", "unsafe_load", "load_stream"])
@@ -45,7 +45,7 @@ class YamlUnsafeLoadArgument extends YamlSink {
 }
 
 class Configuration extends TaintTracking::Configuration {
-  Configuration() { this = "UnsafeYAMLDeserialization" }
+  Configuration() { this = "YamlUnsafeDeserialization" }
 
   override predicate isSource(DataFlow::Node source) {
     // to detect CVE-2022-32224, we should uncomment following line instead of current UnsafeDeserialization::Source
@@ -57,7 +57,7 @@ class Configuration extends TaintTracking::Configuration {
     // 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 YamlSink
+    sink instanceof YamlUnsafeSinks
   }
 
   override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
@@ -84,5 +84,5 @@ class Configuration extends TaintTracking::Configuration {
 
 from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
 where config.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "This file extraction depends on a $@.", source.getNode(),
+select sink.getNode(), source, sink, "Unsafe deserialization depends on a $@.", source.getNode(),
   "potentially untrusted source"