Java: Adapt unsafe deserialization to SnakeYaml 2.0, which is secure by default

java/ql/lib/semmle/code/java/frameworks/SnakeYaml.qll

@@ -4,6 +4,7 @@
 
 import java
 import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.TaintTracking
 
 /**
  * The class `org.yaml.snakeyaml.constructor.SafeConstructor`.
@@ -18,7 +19,9 @@ class SnakeYamlSafeConstructor extends RefType {
  * An instance of `SafeConstructor`.
  */
 class SafeSnakeYamlConstruction extends ClassInstanceExpr {
-  SafeSnakeYamlConstruction() { this.getConstructedType() instanceof SnakeYamlSafeConstructor }
+  SafeSnakeYamlConstruction() {
+    this.getConstructedType().getASourceSupertype*() instanceof SnakeYamlSafeConstructor
+  }
 }
 
 /**
@@ -28,60 +31,98 @@ class Yaml extends RefType {
   Yaml() { this.getAnAncestor().hasQualifiedName("org.yaml.snakeyaml", "Yaml") }
 }
 
-private DataFlow::ExprNode yamlClassInstanceExprArgument(ClassInstanceExpr cie) {
-  cie.getConstructedType() instanceof Yaml and
-  result.getExpr() = cie.getArgument(0)
+/** A call to a parse method of `Yaml`. */
+private class SnakeYamlParse extends MethodAccess {
+  SnakeYamlParse() {
+    exists(Method m |
+      m.getDeclaringType() instanceof Yaml and
+      m.hasName(["compose", "composeAll", "load", "loadAll", "loadAs", "parse"]) and
+      m = this.getMethod()
+    )
+  }
 }
 
-private module SafeYamlConstructionFlowConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeSnakeYamlConstruction }
+private class TagInspector extends Interface {
+  TagInspector() { this.hasQualifiedName("org.yaml.snakeyaml.inspector", "TagInspector") }
+}
 
-  predicate isSink(DataFlow::Node sink) { sink = yamlClassInstanceExprArgument(_) }
+private class LoaderOptions extends Class {
+  LoaderOptions() { this.hasQualifiedName("org.yaml.snakeyaml", "LoaderOptions") }
+}
 
-  additional ClassInstanceExpr getSafeYaml() {
-    SafeYamlConstructionFlow::flowTo(yamlClassInstanceExprArgument(result))
+private class SnakeYamlBaseConstructor extends Class {
+  SnakeYamlBaseConstructor() {
+    this.hasQualifiedName("org.yaml.snakeyaml.constructor", "BaseConstructor")
   }
 }
 
-private module SafeYamlConstructionFlow = DataFlow::Global<SafeYamlConstructionFlowConfig>;
+private class IsGlobalTagAllowed extends Method {
+  IsGlobalTagAllowed() {
+    this.getDeclaringType().getASourceSupertype*() instanceof TagInspector and
+    this.hasName("isGlobalTagAllowed")
+  }
+}
 
 /**
- * An instance of `Yaml` that does not allow arbitrary constructor to be called.
+ * Holds if `m` always returns `true` ignoring any exceptional flow.
  */
-private class SafeYaml extends ClassInstanceExpr {
-  SafeYaml() { SafeYamlConstructionFlowConfig::getSafeYaml() = this }
+private predicate alwaysAllowsGlobalTags(IsGlobalTagAllowed m) {
+  forex(ReturnStmt rs | rs.getEnclosingCallable() = m |
+    rs.getResult().(CompileTimeConstantExpr).getBooleanValue() = true
+  )
 }
 
-/** A call to a parse method of `Yaml`. */
-private class SnakeYamlParse extends MethodAccess {
-  SnakeYamlParse() {
-    exists(Method m |
-      m.getDeclaringType() instanceof Yaml and
-      m.hasName(["compose", "composeAll", "load", "loadAll", "loadAs", "parse"]) and
-      m = this.getMethod()
-    )
+/**
+ * A class that overrides the `org.yaml.snakeyaml.inspector.TagInspector.IsGlobalTagAllowed` method and **always** returns `true` (though it could also exit due to an uncaught exception),
+ * thus allowing arbitrary code execution when untrusted data is deserialized.
+ */
+private class UnsafeTagInspector extends RefType {
+  UnsafeTagInspector() {
+    this.getAnAncestor() instanceof TagInspector and
+    alwaysAllowsGlobalTags(this.getAMethod())
   }
 }
 
-private module SafeYamlFlowConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeYaml }
-
-  predicate isSink(DataFlow::Node sink) { sink = yamlParseQualifier(_) }
-
-  additional DataFlow::ExprNode yamlParseQualifier(SnakeYamlParse syp) {
-    result.getExpr() = syp.getQualifier()
+/**
+ * A configuration to model the flow of a `UnsafeTagInspector` to the qualifier of a `SnakeYamlParse` call.
+ */
+private module UnsafeTagInspectorConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source.asExpr().(ClassInstanceExpr).getConstructedType() instanceof UnsafeTagInspector
   }
 
-  additional SnakeYamlParse getASafeSnakeYamlParse() {
-    SafeYamlFlow::flowTo(yamlParseQualifier(result))
+  predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(MethodAccess ma, Method m |
+      ma.getMethod() = m and
+      m.getDeclaringType() instanceof LoaderOptions and
+      m.hasName("setTagInspector")
+    |
+      n1.asExpr() = ma.getArgument(0) and
+      n2.asExpr() = ma.getQualifier()
+    )
+    or
+    exists(ConstructorCall cc, Constructor c, int argIdx |
+      c.getDeclaringType().getAnAncestor() instanceof SnakeYamlBaseConstructor and
+      c.getParameterType(argIdx) instanceof LoaderOptions
+      or
+      c.getDeclaringType() instanceof Yaml and
+      (
+        c.getParameterType(argIdx) instanceof LoaderOptions or
+        c.getParameterType(argIdx).(RefType).getAnAncestor() instanceof SnakeYamlBaseConstructor
+      )
+    |
+      cc.getConstructor() = c and n1.asExpr() = cc.getArgument(argIdx) and n2.asExpr() = cc
+    )
   }
+
+  predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(SnakeYamlParse p).getQualifier() }
 }
 
-private module SafeYamlFlow = DataFlow::Global<SafeYamlFlowConfig>;
+private module UnsafeTagInspectorFlow = TaintTracking::Global<UnsafeTagInspectorConfig>;
 
 /**
  * A call to a parse method of `Yaml` that allows arbitrary constructor to be called.
  */
 class UnsafeSnakeYamlParse extends SnakeYamlParse {
-  UnsafeSnakeYamlParse() { not SafeYamlFlowConfig::getASafeSnakeYamlParse() = this }
+  UnsafeSnakeYamlParse() { UnsafeTagInspectorFlow::flowToExpr(this.getQualifier()) }
 }

java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.qhelp

@@ -64,8 +64,9 @@ Recommendations specific to particular frameworks supported by this query:
 <p></p>
 <p><b>SnakeYAML</b> - <code>org.yaml:snakeyaml</code></p>
     <ul>
-        <li><b>Secure by Default</b>: No</li>
-        <li><b>Recommendation</b>: Pass an instance of <code>org.yaml.snakeyaml.constructor.SafeConstructor</code> to  <code>org.yaml.snakeyaml.Yaml</code>'s constructor before using it to deserialize untrusted data.</li>
+        <li><b>Secure by Default</b>: Yes since version 2.0.</li>
+        <li><b>Recommendation</b>: Don't deserialize untrusted data with an instance of <code>org.yaml.snakeyaml.Yaml</code> with unsafe <code>org.yaml.snakeyaml.LoaderOptions</code>,
+        that is, using a <code>org.yaml.snakeyaml.inspector.TagInspector</code> that allows global tags with <code>isGlobalTagAllowed</code>.</li>
     </ul>
 <p></p>
 <p><b>XML Decoder</b> - <code>Standard Java Library</code></p>

java/ql/src/change-notes/2023-06-02-snakeyaml-update.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The query `java/unsafe-deserialization` has been updated to take the release of SnakeYaml 2.0 into account. Starting in that version, deserialization of untrusted YAML is protected against remote code execution by default. Deserializers configured with an unsafe `org.yaml.snakeyaml.inspector.TagInspector` are still vulnerable and detected by the query.

java/ql/test/query-tests/security/CWE-502/A.java

@@ -6,6 +6,9 @@
 import com.esotericsoftware.kryo.io.Input;
 import org.yaml.snakeyaml.constructor.SafeConstructor;
 import org.yaml.snakeyaml.constructor.Constructor;
+import org.yaml.snakeyaml.inspector.TagInspector;
+import org.yaml.snakeyaml.nodes.Tag;
+import org.yaml.snakeyaml.LoaderOptions;
 import org.yaml.snakeyaml.Yaml;
 
 public class A {
@@ -58,36 +61,67 @@ public void deserialize6(Socket sock) throws java.io.IOException {
   public void deserializeSnakeYaml(Socket sock) throws java.io.IOException {
     Yaml yaml = new Yaml();
     InputStream input = sock.getInputStream();
-    Object o = yaml.load(input); // $unsafeDeserialization
-    Object o2 = yaml.loadAll(input); // $unsafeDeserialization
-    Object o3 = yaml.parse(new InputStreamReader(input)); // $unsafeDeserialization
-    A o4 = yaml.loadAs(input, A.class); // $unsafeDeserialization
-    A o5 = yaml.loadAs(new InputStreamReader(input), A.class); // $unsafeDeserialization
+    Object o = yaml.load(input); // OK
+    Object o2 = yaml.loadAll(input); // OK
+    Object o3 = yaml.parse(new InputStreamReader(input)); // OK
+    A o4 = yaml.loadAs(input, A.class); // OK
+    A o5 = yaml.loadAs(new InputStreamReader(input), A.class); // OK
   }
 
   public void deserializeSnakeYaml2(Socket sock) throws java.io.IOException {
     Yaml yaml = new Yaml(new Constructor());
     InputStream input = sock.getInputStream();
-    Object o = yaml.load(input); // $unsafeDeserialization
-    Object o2 = yaml.loadAll(input); // $unsafeDeserialization
-    Object o3 = yaml.parse(new InputStreamReader(input)); // $unsafeDeserialization
-    A o4 = yaml.loadAs(input, A.class); // $unsafeDeserialization
-    A o5 = yaml.loadAs(new InputStreamReader(input), A.class); // $unsafeDeserialization
+    Object o = yaml.load(input); // OK
+    Object o2 = yaml.loadAll(input); // OK
+    Object o3 = yaml.parse(new InputStreamReader(input)); // OK
+    A o4 = yaml.loadAs(input, A.class); // OK
+    A o5 = yaml.loadAs(new InputStreamReader(input), A.class); // OK
   }
 
   public void deserializeSnakeYaml3(Socket sock) throws java.io.IOException {
     Yaml yaml = new Yaml(new SafeConstructor());
     InputStream input = sock.getInputStream();
-    Object o = yaml.load(input); //OK
-    Object o2 = yaml.loadAll(input); //OK
-    Object o3 = yaml.parse(new InputStreamReader(input)); //OK
-    A o4 = yaml.loadAs(input, A.class); //OK
-    A o5 = yaml.loadAs(new InputStreamReader(input), A.class); //OK
+    Object o = yaml.load(input); // OK
+    Object o2 = yaml.loadAll(input); // OK
+    Object o3 = yaml.parse(new InputStreamReader(input)); // OK
+    A o4 = yaml.loadAs(input, A.class); // OK
+    A o5 = yaml.loadAs(new InputStreamReader(input), A.class); // OK
   }
 
   public void deserializeSnakeYaml4(Socket sock) throws java.io.IOException {
     Yaml yaml = new Yaml(new Constructor(A.class));
     InputStream input = sock.getInputStream();
+    Object o = yaml.load(input); // OK
+    Object o2 = yaml.loadAll(input); // OK
+    Object o3 = yaml.parse(new InputStreamReader(input)); // OK
+    A o4 = yaml.loadAs(input, A.class); // OK
+    A o5 = yaml.loadAs(new InputStreamReader(input), A.class); // OK
+  }
+
+  private static class CustomSafeConstructor extends SafeConstructor {
+  }
+
+  public void deserializeSnakeYaml5(Socket sock) throws java.io.IOException {
+    Yaml yaml = new Yaml(new CustomSafeConstructor());
+    InputStream input = sock.getInputStream();
+    Object o = yaml.load(input); // OK
+    Object o2 = yaml.loadAll(input); // OK
+    Object o3 = yaml.parse(new InputStreamReader(input)); // OK
+    A o4 = yaml.loadAs(input, A.class); // OK
+    A o5 = yaml.loadAs(new InputStreamReader(input), A.class); // OK
+  }
+
+  public void deserializeSnakeYaml6(Socket sock) throws java.io.IOException {
+    LoaderOptions options = new LoaderOptions();
+    TagInspector allowedTags = new TagInspector() {
+      @Override
+      public boolean isGlobalTagAllowed(Tag tag) {
+        return true;
+      }
+    };
+    options.setTagInspector(allowedTags);
+    Yaml yaml = new Yaml(new Constructor(options));
+    InputStream input = sock.getInputStream();
     Object o = yaml.load(input); // $unsafeDeserialization
     Object o2 = yaml.loadAll(input); // $unsafeDeserialization
     Object o3 = yaml.parse(new InputStreamReader(input)); // $unsafeDeserialization