Merge pull request #20025 from owen-mc/java/unsafe-deserialization

Java: add extra sink for `java/unsafe-deserialization`

Author: owen-mc

java/ql/lib/change-notes/2025-07-11-unsafe-deserialization-extra-sink.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The qualifiers of a calls to `readObject` on any classes that implement `java.io.ObjectInput` are now recognised as sinks for `java/unsafe-deserialization`. Previously this was only the case for classes which extend `java.io.ObjectInputStream`.

java/ql/lib/semmle/code/java/JDK.qll

@@ -211,6 +211,11 @@ class TypeObjectOutputStream extends RefType {
   TypeObjectOutputStream() { this.hasQualifiedName("java.io", "ObjectOutputStream") }
 }
 
+/** The type `java.io.ObjectInput`. */
+class TypeObjectInput extends RefType {
+  TypeObjectInput() { this.hasQualifiedName("java.io", "ObjectInput") }
+}
+
 /** The type `java.io.ObjectInputStream`. */
 class TypeObjectInputStream extends RefType {
   TypeObjectInputStream() { this.hasQualifiedName("java.io", "ObjectInputStream") }

java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll

@@ -23,10 +23,17 @@ private import semmle.code.java.frameworks.google.Gson
 private import semmle.code.java.frameworks.apache.Lang
 private import semmle.code.java.Reflection
 
-private class ObjectInputStreamReadObjectMethod extends Method {
-  ObjectInputStreamReadObjectMethod() {
+private class ObjectInputReadObjectMethod extends Method {
+  ObjectInputReadObjectMethod() {
+    this.getDeclaringType().getASourceSupertype*() instanceof TypeObjectInput and
+    this.hasName("readObject")
+  }
+}
+
+private class ObjectInputStreamReadUnsharedMethod extends Method {
+  ObjectInputStreamReadUnsharedMethod() {
     this.getDeclaringType().getASourceSupertype*() instanceof TypeObjectInputStream and
-    (this.hasName("readObject") or this.hasName("readUnshared"))
+    this.hasName("readUnshared")
   }
 }
 
@@ -147,12 +154,13 @@ private module SafeKryoFlow = DataFlow::Global<SafeKryoConfig>;
  */
 predicate unsafeDeserialization(MethodCall ma, Expr sink) {
   exists(Method m | m = ma.getMethod() |
-    m instanceof ObjectInputStreamReadObjectMethod and
+    m instanceof ObjectInputReadObjectMethod and
     sink = ma.getQualifier() and
-    not exists(DataFlow::ExprNode node |
-      node.getExpr() = sink and
-      node.getTypeBound() instanceof SafeObjectInputStreamType
-    )
+    not DataFlow::exprNode(sink).getTypeBound() instanceof SafeObjectInputStreamType
+    or
+    m instanceof ObjectInputStreamReadUnsharedMethod and
+    sink = ma.getQualifier() and
+    not DataFlow::exprNode(sink).getTypeBound() instanceof SafeObjectInputStreamType
     or
     m instanceof XmlDecoderReadObjectMethod and
     sink = ma.getQualifier()

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

@@ -1,6 +1,9 @@
+package unsafedeserialization;
+
 import java.io.*;
 import java.net.Socket;
 import java.beans.XMLDecoder;
+import com.example.MyObjectInput;
 import com.thoughtworks.xstream.XStream;
 import com.esotericsoftware.kryo.Kryo;
 import com.esotericsoftware.kryo.io.Input;
@@ -10,13 +13,23 @@
 import org.nibblesec.tools.SerialKiller;
 
 public class A {
-  public Object deserialize1(Socket sock) throws java.io.IOException, ClassNotFoundException {
+  public Object deserialize1a(Socket sock) throws java.io.IOException, ClassNotFoundException {
     InputStream inputStream = sock.getInputStream(); // $ Source
     ObjectInputStream in = new ObjectInputStream(inputStream);
     return in.readObject(); // $ Alert
   }
 
-  public Object deserialize2(Socket sock) throws java.io.IOException, ClassNotFoundException {
+  public Object deserialize2() throws java.io.IOException, ClassNotFoundException {
+    ObjectInput objectInput = A.getTaintedObjectInput(); // $ Source
+    return objectInput.readObject(); // $ Alert
+  }
+
+  public Object deserialize3() throws java.io.IOException, ClassNotFoundException {
+    MyObjectInput objectInput = A.getTaintedMyObjectInput(); // $ Source
+    return objectInput.readObject(); // $ Alert
+  }
+
+  public Object deserialize4(Socket sock) throws java.io.IOException, ClassNotFoundException {
     InputStream inputStream = sock.getInputStream(); // $ Source
     ObjectInputStream in = new ObjectInputStream(inputStream);
     return in.readUnshared(); // $ Alert
@@ -28,20 +41,20 @@ public Object deserializeWithSerialKiller(Socket sock) throws java.io.IOExceptio
     return in.readUnshared(); // OK
   }
 
-  public Object deserialize3(Socket sock) throws java.io.IOException {
+  public Object deserialize5(Socket sock) throws java.io.IOException {
     InputStream inputStream = sock.getInputStream(); // $ Source
     XMLDecoder d = new XMLDecoder(inputStream);
     return d.readObject(); // $ Alert
   }
 
-  public Object deserialize4(Socket sock) throws java.io.IOException {
+  public Object deserialize6(Socket sock) throws java.io.IOException {
     XStream xs = new XStream();
     InputStream inputStream = sock.getInputStream(); // $ Source
     Reader reader = new InputStreamReader(inputStream);
     return xs.fromXML(reader); // $ Alert
   }
 
-  public void deserialize5(Socket sock) throws java.io.IOException {
+  public void deserialize7(Socket sock) throws java.io.IOException {
     Kryo kryo = new Kryo();
     Input input = new Input(sock.getInputStream()); // $ Source
     A a1 = kryo.readObject(input, A.class); // $ Alert
@@ -56,7 +69,7 @@ private Kryo getSafeKryo() throws java.io.IOException {
     return kryo;
   }
 
-  public void deserialize6(Socket sock) throws java.io.IOException {
+  public void deserialize8(Socket sock) throws java.io.IOException {
     Kryo kryo = getSafeKryo();
     Input input = new Input(sock.getInputStream());
     Object o = kryo.readClassAndObject(input); // OK
@@ -101,4 +114,8 @@ public void deserializeSnakeYaml4(Socket sock) throws java.io.IOException {
     A o4 = yaml.loadAs(input, A.class); // $ Alert
     A o5 = yaml.loadAs(new InputStreamReader(input), A.class); // $ Alert
   }
+
+  static ObjectInput getTaintedObjectInput() { return null; }
+
+  static MyObjectInput getTaintedMyObjectInput() { return null; }
 }