Ruby: Consolidate unsafe deserialization queries

Merge the experimental YAMLUnsafeDeserialization and
PlistUnsafeDeserialization queries into the generate
UnsafeDeserialization query in the default suite.

These queries look for some specific sinks that we now find in the
general query.

Also apply some small code and comment refactors.

Author: hmac

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

@@ -8,36 +8,28 @@ 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`:
+ * 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
+ * ```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
+      succ = yamlParserMethod.getAMethodCall("to_ruby") 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")
+        yamlParserMethod = yamlNode().getAMethodCall(["parse", "parse_stream"]) and
+        pred = [yamlParserMethod.getArgument(0), yamlParserMethod.getKeywordArgument("yaml")]
+        or
+        yamlParserMethod = yamlNode().getAMethodCall("parse_file") and
+        pred = [yamlParserMethod.getArgument(0), yamlParserMethod.getKeywordArgument("filename")]
+      )
     )
   }
 }
 
-/**
- * YAML/Psych Top level Class member
- */
 private API::Node yamlNode() { result = API::getTopLevelMember(["YAML", "Psych"]) }

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

@@ -75,14 +75,13 @@ module UnsafeDeserialization {
   }
 
   /**
-   * An argument in a call to `YAML.unsafe_*` and `YAML.load_stream` , considered sinks
+   * An argument in a call to `YAML.unsafe_*` and `YAML.load_stream` , considered a sink
    * 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() {
+      // Note: this is safe in psych/yaml >= 4.0.0.
       this = yamlNode().getAMethodCall("load").getArgument(0)
       or
       this =
@@ -94,16 +93,11 @@ module UnsafeDeserialization {
     }
   }
 
-  /**
-   * 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.
+   * An argument in a call to `YAML.parse*`, considered a sink for unsafe deserialization
+   * if there is a call to `to_ruby` on the returned value.
    */
   class YamlParseArgument extends Sink {
     YamlParseArgument() {
@@ -237,7 +231,7 @@ module UnsafeDeserialization {
   }
 
   /**
-   * An argument in a call to `Plist.parse_xml` where the marshal is `true` (which is
+   * An argument in a call to `Plist.parse_xml` where `marshal` is `true` (which is
    * the default), considered a sink for unsafe deserialization.
    */
   class UnsafePlistParsexmlArgument extends Sink {
@@ -246,10 +240,11 @@ module UnsafeDeserialization {
         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
+        (
+          plistParseXml.getKeywordArgument("marshal").getConstantValue().isBoolean(true)
+          or
+          plistParseXml.getNumberOfArguments() = 1
+        )
       )
     }
   }

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

@@ -19,14 +19,19 @@ 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.
+If deserializing an untrusted YAML document using the <code>psych</code> gem
+prior to version 4.0.0, the <code>load</code> method is vulnerable. Use
+<code>safe_load</code> instead. With <code>psych</code> version 4.0.0 and later,
+the <code>load</code> is safe. The same applies to <code>load_file</code>.
+<code>load_stream</code> is vulnerable in all versions. The safe versions of these
+methods (<code>safe_load</code> and <code>safe_load_file</code>) are not vulnerable
+in any known version.
 </p>
 
 <p>
-This vulnerability in Plist can be prevented by calling <code>Plist.parse_xml FileOrXmlString, marshal: false</code>.
+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>
+when calling <code>Plist.parse_xml</code>.
 </p>
 </recommendation>
 
@@ -39,13 +44,6 @@ 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/YAMLUnsafeDeserialization.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/PlistUnsafeDeserialization.rb"/>
-
 <p>
 Using <code>JSON.parse</code> and <code>YAML.safe_load</code> instead, as in the
 following example, removes the vulnerability. Similarly, calling