Merge pull request #10512 from github/nickrolfe/hash_from_trusted_xml

Ruby: add Hash.from_trusted_xml as an unsafe deserialization sink

Author: nickrolfe

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

@@ -67,6 +67,16 @@ module UnsafeDeserialization {
     }
   }
 
+  /**
+   * The first argument in a call to `Hash.from_trusted_xml`, considered as a
+   * sink for unsafe deserialization.
+   */
+  class HashFromTrustedXmlArgument extends Sink {
+    HashFromTrustedXmlArgument() {
+      this = API::getTopLevelMember("Hash").getAMethodCall("from_trusted_xml").getArgument(0)
+    }
+  }
+
   private string getAKnownOjModeName(boolean isSafe) {
     result = ["compat", "custom", "json", "null", "rails", "strict", "wab"] and isSafe = true
     or

ruby/ql/src/change-notes/2022-09-21-hash-from-trusted-xml.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `rb/unsafe-deserialization` query now includes alerts for user-controlled data passed to `Hash.from_trusted_xml`, since that method can deserialize YAML embedded in the XML, which in turn can result in deserialization of arbitrary objects.

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

@@ -14,6 +14,8 @@ edges
 | UnsafeDeserialization.rb:51:17:51:28 | ...[...] :  | UnsafeDeserialization.rb:53:22:53:30 | json_data |
 | UnsafeDeserialization.rb:58:17:58:22 | call to params :  | UnsafeDeserialization.rb:58:17:58:28 | ...[...] :  |
 | UnsafeDeserialization.rb:58:17:58:28 | ...[...] :  | UnsafeDeserialization.rb:68:23:68:31 | json_data |
+| UnsafeDeserialization.rb:80:11:80:16 | call to params :  | UnsafeDeserialization.rb:80:11:80:22 | ...[...] :  |
+| UnsafeDeserialization.rb:80:11:80:22 | ...[...] :  | UnsafeDeserialization.rb:81:34:81:36 | xml |
 nodes
 | UnsafeDeserialization.rb:9:39:9:44 | call to params :  | semmle.label | call to params :  |
 | UnsafeDeserialization.rb:9:39:9:50 | ...[...] :  | semmle.label | ...[...] :  |
@@ -37,6 +39,9 @@ nodes
 | UnsafeDeserialization.rb:58:17:58:22 | call to params :  | semmle.label | call to params :  |
 | UnsafeDeserialization.rb:58:17:58:28 | ...[...] :  | semmle.label | ...[...] :  |
 | UnsafeDeserialization.rb:68:23:68:31 | json_data | semmle.label | json_data |
+| UnsafeDeserialization.rb:80:11:80:16 | call to params :  | semmle.label | call to params :  |
+| UnsafeDeserialization.rb:80:11:80:22 | ...[...] :  | semmle.label | ...[...] :  |
+| UnsafeDeserialization.rb:81:34:81:36 | xml | semmle.label | xml |
 subpaths
 #select
 | UnsafeDeserialization.rb:10:27:10:41 | serialized_data | UnsafeDeserialization.rb:9:39:9:44 | call to params :  | UnsafeDeserialization.rb:10:27:10:41 | serialized_data | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:9:39:9:44 | call to params | user-provided value |
@@ -47,3 +52,4 @@ subpaths
 | UnsafeDeserialization.rb:52:22:52:30 | json_data | UnsafeDeserialization.rb:51:17:51:22 | call to params :  | UnsafeDeserialization.rb:52:22:52:30 | json_data | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:51:17:51:22 | call to params | user-provided value |
 | UnsafeDeserialization.rb:53:22:53:30 | json_data | UnsafeDeserialization.rb:51:17:51:22 | call to params :  | UnsafeDeserialization.rb:53:22:53:30 | json_data | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:51:17:51:22 | call to params | user-provided value |
 | UnsafeDeserialization.rb:68:23:68:31 | json_data | UnsafeDeserialization.rb:58:17:58:22 | call to params :  | UnsafeDeserialization.rb:68:23:68:31 | json_data | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:58:17:58:22 | call to params | user-provided value |
+| UnsafeDeserialization.rb:81:34:81:36 | xml | UnsafeDeserialization.rb:80:11:80:16 | call to params :  | UnsafeDeserialization.rb:81:34:81:36 | xml | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:80:11:80:16 | call to params | user-provided value |

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

@@ -73,4 +73,11 @@ def route9
     json_data = params[:key]
     object = Oj.safe_load json_data
   end
+
+  # BAD - `Hash.from_trusted_xml` will deserialize elements with the
+  # `type="yaml"` attribute as YAML.
+  def route10
+    xml = params[:key]
+    hash = Hash.from_trusted_xml(xml)
+  end
 end