Merge pull request github#5843 from JLLeitschuh/feat/JLL/improve_kryo_support

[Java] Fix Kryo FP & Kryo 5 Support

Author: aschackmull

java/change-notes/2021-05-05-kryo-improvements.md

@@ -0,0 +1,3 @@
+lgtm,codescanning
+* Add support for version 5 of the Kryo serialization/deserialization framework.
+* Add support for detecting safe uses of Kryo utilizing `KryoPool.Builder`. [#4992](https://github.com/github/codeql/issues/4992)

java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll

@@ -292,6 +292,7 @@ private predicate summaryModelCsv(string row) {
       "java.util;StringTokenizer;false;StringTokenizer;;;Argument[0];Argument[-1];taint",
       "java.beans;XMLDecoder;false;XMLDecoder;;;Argument[0];Argument[-1];taint",
       "com.esotericsoftware.kryo.io;Input;false;Input;;;Argument[0];Argument[-1];taint",
+      "com.esotericsoftware.kryo5.io;Input;false;Input;;;Argument[0];Argument[-1];taint",
       "java.io;BufferedInputStream;false;BufferedInputStream;;;Argument[0];Argument[-1];taint",
       "java.io;DataInputStream;false;DataInputStream;;;Argument[0];Argument[-1];taint",
       "java.io;ByteArrayInputStream;false;ByteArrayInputStream;;;Argument[0];Argument[-1];taint",

java/ql/src/semmle/code/java/frameworks/Kryo.qll

@@ -3,19 +3,60 @@
  */
 
 import java
+private import semmle.code.java.dataflow.DataFlow
+private import semmle.code.java.dataflow.FlowSteps
 
 /**
  * The type `com.esotericsoftware.kryo.Kryo`.
  */
 class Kryo extends RefType {
-  Kryo() { this.hasQualifiedName("com.esotericsoftware.kryo", "Kryo") }
+  Kryo() {
+    hasQualifiedName("com.esotericsoftware.kryo", "Kryo") or
+    hasQualifiedName("com.esotericsoftware.kryo5", "Kryo")
+  }
 }
 
 /**
  * A Kryo input stream.
  */
 class KryoInput extends RefType {
-  KryoInput() { this.hasQualifiedName("com.esotericsoftware.kryo.io", "Input") }
+  KryoInput() {
+    hasQualifiedName("com.esotericsoftware.kryo.io", "Input") or
+    hasQualifiedName("com.esotericsoftware.kryo5.io", "Input")
+  }
+}
+
+/**
+ * A Kryo pool.
+ */
+class KryoPool extends RefType {
+  KryoPool() {
+    hasQualifiedName("com.esotericsoftware.kryo.pool", "KryoPool") or
+    hasQualifiedName("com.esotericsoftware.kryo5.pool", "KryoPool")
+  }
+}
+
+/**
+ * A Kryo pool builder.
+ */
+class KryoPoolBuilder extends RefType {
+  KryoPoolBuilder() {
+    hasQualifiedName("com.esotericsoftware.kryo.pool", "KryoPool$Builder") or
+    hasQualifiedName("com.esotericsoftware.kryo5.pool", "KryoPool$Builder")
+  }
+}
+
+/**
+ * A Kryo pool builder method used in a fluent API call chain.
+ */
+class KryoPoolBuilderMethod extends Method {
+  KryoPoolBuilderMethod() {
+    getDeclaringType() instanceof KryoPoolBuilder and
+    (
+      getReturnType() instanceof KryoPoolBuilder or
+      getReturnType() instanceof KryoPool
+    )
+  }
 }
 
 /**
@@ -45,3 +86,13 @@ class KryoEnableWhiteListing extends MethodAccess {
     )
   }
 }
+
+/**
+ * A KryoPool method that uses a Kryo instance.
+ */
+class KryoPoolRunMethod extends Method {
+  KryoPoolRunMethod() {
+    getDeclaringType() instanceof KryoPool and
+    hasName("run")
+  }
+}

java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll

@@ -48,6 +48,65 @@ class SafeKryo extends DataFlow2::Configuration {
       ma.getMethod() instanceof KryoReadObjectMethod
     )
   }
+
+  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    stepKryoPoolBuilderFactoryArgToConstructor(node1, node2) or
+    stepKryoPoolRunMethodAccessQualifierToFunctionalArgument(node1, node2) or
+    stepKryoPoolBuilderChainMethod(node1, node2) or
+    stepKryoPoolBorrowMethod(node1, node2)
+  }
+
+  /**
+   * Holds when a functional expression is used to create a `KryoPool.Builder`.
+   * Eg. `new KryoPool.Builder(() -> new Kryo())`
+   */
+  private predicate stepKryoPoolBuilderFactoryArgToConstructor(
+    DataFlow::Node node1, DataFlow::Node node2
+  ) {
+    exists(ConstructorCall cc, FunctionalExpr fe |
+      cc.getConstructedType() instanceof KryoPoolBuilder and
+      fe.asMethod().getBody().getAStmt().(ReturnStmt).getResult() = node1.asExpr() and
+      node2.asExpr() = cc and
+      cc.getArgument(0) = fe
+    )
+  }
+
+  /**
+   * Holds when a `KryoPool.run` is called to use a `Kryo` instance.
+   * Eg. `pool.run(kryo -> ...)`
+   */
+  private predicate stepKryoPoolRunMethodAccessQualifierToFunctionalArgument(
+    DataFlow::Node node1, DataFlow::Node node2
+  ) {
+    exists(MethodAccess ma |
+      ma.getMethod() instanceof KryoPoolRunMethod and
+      node1.asExpr() = ma.getQualifier() and
+      ma.getArgument(0).(FunctionalExpr).asMethod().getParameter(0) = node2.asParameter()
+    )
+  }
+
+  /**
+   * Holds when a `KryoPool.Builder` method is called fluently.
+   */
+  private predicate stepKryoPoolBuilderChainMethod(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(MethodAccess ma |
+      ma.getMethod() instanceof KryoPoolBuilderMethod and
+      ma = node2.asExpr() and
+      ma.getQualifier() = node1.asExpr()
+    )
+  }
+
+  /**
+   * Holds when a `KryoPool.borrow` method is called.
+   */
+  private predicate stepKryoPoolBorrowMethod(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(MethodAccess ma |
+      ma.getMethod() =
+        any(Method m | m.getDeclaringType() instanceof KryoPool and m.hasName("borrow")) and
+      node1.asExpr() = ma.getQualifier() and
+      node2.asExpr() = ma
+    )
+  }
 }
 
 predicate unsafeDeserialization(MethodAccess ma, Expr sink) {

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

@@ -0,0 +1,34 @@
+
+import java.io.*;
+import java.net.Socket;
+import com.esotericsoftware.kryo.Kryo;
+import com.esotericsoftware.kryo.pool.KryoPool;
+import com.esotericsoftware.kryo.io.Input;
+
+public class KryoTest {
+
+    private Kryo getSafeKryo() {
+        Kryo kryo = new Kryo();
+        kryo.setRegistrationRequired(true);
+        // ... kryo.register(A.class) ...
+        return kryo;
+    }
+
+    public void kryoDeserialize(Socket sock) throws java.io.IOException {
+        KryoPool kryoPool = new KryoPool.Builder(this::getSafeKryo).softReferences().build();
+        Input input = new Input(sock.getInputStream());
+        Object o = kryoPool.run(kryo -> kryo.readClassAndObject(input)); // OK
+    }
+
+    public void kryoDeserialize2(Socket sock) throws java.io.IOException {
+        KryoPool kryoPool = new KryoPool.Builder(this::getSafeKryo).softReferences().build();
+        Input input = new Input(sock.getInputStream());
+        Kryo k = kryoPool.borrow();
+        try {
+            Object o = k.readClassAndObject(input); // OK
+        } finally {
+            kryoPool.release(k);
+        }
+    }
+
+}

java/ql/test/stubs/kryo-4.0.2/com/esotericsoftware/kryo/pool/KryoCallback.java

@@ -0,0 +1,7 @@
+package com.esotericsoftware.kryo.pool;
+
+import com.esotericsoftware.kryo.Kryo;
+
+public interface KryoCallback<T> {
+	T execute (Kryo kryo);
+}

java/ql/test/stubs/kryo-4.0.2/com/esotericsoftware/kryo/pool/KryoFactory.java

@@ -0,0 +1,7 @@
+package com.esotericsoftware.kryo.pool;
+
+import com.esotericsoftware.kryo.Kryo;
+
+public interface KryoFactory {
+	Kryo create ();
+}

java/ql/test/stubs/kryo-4.0.2/com/esotericsoftware/kryo/pool/KryoPool.java

@@ -0,0 +1,30 @@
+package com.esotericsoftware.kryo.pool;
+
+import com.esotericsoftware.kryo.Kryo;
+import java.util.Queue;
+
+public interface KryoPool {
+
+    Kryo borrow ();
+
+    void release (Kryo kryo);
+
+    <T> T run (KryoCallback<T> callback);
+
+    static class Builder {
+        public Builder (KryoFactory factory) {
+        }
+    
+        public Builder queue (Queue<Kryo> queue) {
+            return null;
+        }
+    
+        public Builder softReferences () {
+            return null;
+        }
+    
+        public KryoPool build () {
+            return null;
+        }
+    }
+}