diff --git a/java/change-notes/2021-05-05-kryo-improvements.md b/java/change-notes/2021-05-05-kryo-improvements.md new file mode 100644 index 000000000000..dbacb10099b4 --- /dev/null +++ b/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) diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 7073c57ff9c7..b62018dc2c45 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -291,6 +291,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", diff --git a/java/ql/src/semmle/code/java/frameworks/Kryo.qll b/java/ql/src/semmle/code/java/frameworks/Kryo.qll index 049be97ea9cb..317148d56b55 100644 --- a/java/ql/src/semmle/code/java/frameworks/Kryo.qll +++ b/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") + } +} diff --git a/java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll b/java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll index ab809f07d6d1..a1561c0e6cd9 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll +++ b/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) { diff --git a/java/ql/test/query-tests/security/CWE-502/KryoTest.java b/java/ql/test/query-tests/security/CWE-502/KryoTest.java new file mode 100644 index 000000000000..8890bad91b9f --- /dev/null +++ b/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); + } + } + +} diff --git a/java/ql/test/stubs/kryo-4.0.2/com/esotericsoftware/kryo/pool/KryoCallback.java b/java/ql/test/stubs/kryo-4.0.2/com/esotericsoftware/kryo/pool/KryoCallback.java new file mode 100644 index 000000000000..729426aba626 --- /dev/null +++ b/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 execute (Kryo kryo); +} diff --git a/java/ql/test/stubs/kryo-4.0.2/com/esotericsoftware/kryo/pool/KryoFactory.java b/java/ql/test/stubs/kryo-4.0.2/com/esotericsoftware/kryo/pool/KryoFactory.java new file mode 100644 index 000000000000..4dcda1445dfc --- /dev/null +++ b/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 (); +} diff --git a/java/ql/test/stubs/kryo-4.0.2/com/esotericsoftware/kryo/pool/KryoPool.java b/java/ql/test/stubs/kryo-4.0.2/com/esotericsoftware/kryo/pool/KryoPool.java new file mode 100644 index 000000000000..c005442c9e8a --- /dev/null +++ b/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 run (KryoCallback callback); + + static class Builder { + public Builder (KryoFactory factory) { + } + + public Builder queue (Queue queue) { + return null; + } + + public Builder softReferences () { + return null; + } + + public KryoPool build () { + return null; + } + } +}