v4: make variable names camelCase, some inhancement, remove some duplicates

Author: am0o0

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

@@ -1,30 +1,43 @@
 /**
- * add additional steps for to_ruby method of YAML/Psych library
+ * 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 yaml_parser_methods |
-      yaml_parser_methods =
-        API::getTopLevelMember(["YAML", "Psych"]).getAMethodCall(["parse", "parse_stream"]) and
+    exists(DataFlow::CallNode yamlParserMethod |
+      yamlParserMethod = yamlNode().getAMethodCall(["parse", "parse_stream"]) and
       (
-        pred = yaml_parser_methods.getArgument(0) or
-        pred = yaml_parser_methods.getKeywordArgument("yaml")
+        pred = yamlParserMethod.getArgument(0) or
+        pred = yamlParserMethod.getKeywordArgument("yaml")
       ) and
-      succ = yaml_parser_methods.getAMethodCall("to_ruby")
-    )
-    or
-    exists(DataFlow::CallNode yaml_parser_methods |
-      yaml_parser_methods = API::getTopLevelMember(["YAML", "Psych"]).getAMethodCall("parse_file") and
+      succ = yamlParserMethod.getAMethodCall("to_ruby")
+      or
+      yamlParserMethod = yamlNode().getAMethodCall("parse_file") and
       (
-        pred = yaml_parser_methods.getArgument(0) or
-        pred = yaml_parser_methods.getKeywordArgument("filename")
+        pred = yamlParserMethod.getArgument(0) or
+        pred = yamlParserMethod.getKeywordArgument("filename")
       ) and
-      succ = yaml_parser_methods.getAMethodCall("to_ruby")
+      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

@@ -78,26 +78,27 @@ module UnsafeDeserialization {
    * 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 recent versions of YAML library, so it will be removed in future.
    */
   class YamlLoadArgument extends Sink {
     YamlLoadArgument() {
-      this =
-        API::getTopLevelMember(["YAML", "Psych"])
-            .getAMethodCall(["unsafe_load_file", "unsafe_load", "load_stream"])
-            .getArgument(0)
+      this = yamlNode().getAMethodCall("load").getArgument(0)
       or
       this =
-        API::getTopLevelMember(["YAML", "Psych"])
-            .getAMethodCall(["unsafe_load", "load_stream"])
-            .getKeywordArgument("yaml")
+        yamlNode().getAMethodCall(["unsafe_load_file", "unsafe_load", "load_stream"]).getArgument(0)
       or
-      this =
-        API::getTopLevelMember(["YAML", "Psych"])
-            .getAMethodCall("unsafe_load_file")
-            .getKeywordArgument("filename")
+      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,
@@ -107,9 +108,7 @@ module UnsafeDeserialization {
   class YamlParseArgument extends Sink {
     YamlParseArgument() {
       this =
-        API::getTopLevelMember(["YAML", "Psych"])
-            .getAMethodCall(["parse", "parse_stream", "parse_file"])
-            .getAMethodCall("to_ruby")
+        yamlNode().getAMethodCall(["parse", "parse_stream", "parse_file"]).getAMethodCall("to_ruby")
     }
   }
 
@@ -237,29 +236,20 @@ module UnsafeDeserialization {
     }
   }
 
-  /**
-   * check whether an input argument has desired "key: value" input or not.
-   */
-  predicate checkkeyValue(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.
    */
   class UnsafePlistParsexmlArgument extends Sink {
     UnsafePlistParsexmlArgument() {
-      exists(DataFlow::CallNode plistParsexml |
-        plistParsexml = API::getTopLevelMember("Plist").getAMethodCall("parse_xml")
+      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.
-        checkkeyValue(plistParsexml.getArgument(1).asExpr(), "marshal", "true")
+        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
+        this = [plistParseXml.getArgument(0), plistParseXml.getKeywordArgument("filename_or_xml")] and
+        plistParseXml.getNumberOfArguments() = 1
       )
     }
   }

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

@@ -9,7 +9,6 @@
 private import codeql.ruby.AST
 private import codeql.ruby.DataFlow
 private import codeql.ruby.TaintTracking
-private import codeql.ruby.ApiGraphs
 import UnsafeDeserializationCustomizations
 
 /**

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

@@ -8,18 +8,17 @@
     </overview>
     <recommendation>
         <p>
-            This vulnerability can be prevented by using <code>Plist.parse_xml</code>.
+        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 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>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="PlistUnsafeYamlDeserialization.rb" />
     </example>
     <references>
     <li>
-    Security considerations from library documentation
-    <a href="https://github.com/patsplat/plist#label-Security+considerations">patsplat/plist</a>.
+    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

@@ -6,7 +6,7 @@
  * @problem.severity warning
  * @security-severity 9.8
  * @precision high
- * @id rb/Plist-unsafe-deserialization
+ * @id rb/plist-unsafe-deserialization
  * @tags security
  *       experimental
  *       external/cwe/cwe-502
@@ -21,31 +21,20 @@ 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")
+    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")
+      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
+      this = [plistParseXml.getArgument(0), plistParseXml.getKeywordArgument("filename_or_xml")] and
+      plistParseXml.getNumberOfArguments() = 1
     )
   }
 }

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

@@ -1,12 +1,16 @@
-require 'yaml'
+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/queries/security/cwe-502/UnsafeDeserialization.qhelp

@@ -17,6 +17,17 @@ libraries that support it, such as the Ruby standard library's <code>JSON</code>
 module, ensure that the parser is configured to disable
 deserialization of arbitrary objects.
 </p>
+
+<p>
+YAML/Psych recommendation:
+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>
+
+<p>
+This vulnerability in Plist can be prevented by calling <code>Plist.parse_xml FileOrXmlString, marshal: false</code>.
+</p>
 </recommendation>
 
 <example>
@@ -27,6 +38,14 @@ 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>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="examples/YAMLUnsafeYamlDeserialization.rb"/>
+
+<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="examples/PlistUnsafeYamlDeserialization.rb"/>
+
 <p>
 Using <code>JSON.parse</code> and <code>YAML.safe_load</code> instead, as in the
 following example, removes the vulnerability. Similarly, calling
@@ -55,6 +74,10 @@ Ruby documentation: <a href="https://ruby-doc.org/stdlib-3.0.2/libdoc/json/rdoc/
 <li>
 Ruby documentation: <a href="https://ruby-doc.org/stdlib-3.0.2/libdoc/yaml/rdoc/YAML.html#module-YAML-label-Security">security guidance on the YAML library</a>.
 </li>
+<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/queries/security/cwe-502/examples/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/queries/security/cwe-502/examples/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
+
+