Merge pull request github#6849 from Marcono1234/marcono1234/improvements

Java: Serialization query improvements

Author: aschackmull

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

@@ -156,6 +156,11 @@ class TypeObjectOutputStream extends RefType {
   TypeObjectOutputStream() { hasQualifiedName("java.io", "ObjectOutputStream") }
 }
 
+/** The type `java.io.ObjectInputStream`. */
+class TypeObjectInputStream extends RefType {
+  TypeObjectInputStream() { hasQualifiedName("java.io", "ObjectInputStream") }
+}
+
 /** The class `java.nio.file.Paths`. */
 class TypePaths extends Class {
   TypePaths() { this.hasQualifiedName("java.nio.file", "Paths") }
@@ -275,7 +280,7 @@ class WriteObjectMethod extends Method {
  */
 class ReadObjectMethod extends Method {
   ReadObjectMethod() {
-    this.getDeclaringType().hasQualifiedName("java.io", "ObjectInputStream") and
+    this.getDeclaringType() instanceof TypeObjectInputStream and
     (
       this.hasName("readObject") or
       this.hasName("readObjectOverride") or

java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll

@@ -269,7 +269,7 @@ private predicate taintPreservingQualifierToMethod(Method m) {
     m.getName() = "toString"
   )
   or
-  m.getDeclaringType().hasQualifiedName("java.io", "ObjectInputStream") and
+  m.getDeclaringType() instanceof TypeObjectInputStream and
   m.getName().matches("read%")
   or
   m instanceof GetterMethod and

java/ql/lib/semmle/code/java/frameworks/javaee/ejb/EJBRestrictions.qll

@@ -299,10 +299,7 @@ class RuntimeExitOrHaltMethod extends Method {
     (this.hasName("exit") or this.hasName("halt")) and
     this.getNumberOfParameters() = 1 and
     this.getParameter(0).getType().(PrimitiveType).hasName("int") and
-    this.getDeclaringType()
-        .getASupertype*()
-        .getSourceDeclaration()
-        .hasQualifiedName("java.lang", "Runtime")
+    this.getDeclaringType().getASupertype*().getSourceDeclaration() instanceof TypeRuntime
   }
 }
 
@@ -315,10 +312,7 @@ class RuntimeAddOrRemoveShutdownHookMethod extends Method {
     (this.hasName("addShutdownHook") or this.hasName("removeShutdownHook")) and
     this.getNumberOfParameters() = 1 and
     this.getParameter(0).getType().(RefType).hasQualifiedName("java.lang", "Thread") and
-    this.getDeclaringType()
-        .getASupertype*()
-        .getSourceDeclaration()
-        .hasQualifiedName("java.lang", "Runtime")
+    this.getDeclaringType().getASupertype*().getSourceDeclaration() instanceof TypeRuntime
   }
 }
 
@@ -414,33 +408,29 @@ class ForbiddenSerializationMethod extends Method {
 
 /**
  * A method named `enableReplaceObject` declared in
- * the class `java.io.ObjectInputStream` or a subclass thereof.
+ * the class `java.io.ObjectOutputStream` or a subclass thereof.
  */
 class EnableReplaceObjectMethod extends Method {
   EnableReplaceObjectMethod() {
     this.hasName("enableReplaceObject") and
     this.getNumberOfParameters() = 1 and
     this.getParameter(0).getType().(PrimitiveType).hasName("boolean") and
-    this.getDeclaringType()
-        .getASupertype*()
-        .getSourceDeclaration()
-        .hasQualifiedName("java.io", "ObjectOutputStream")
+    this.getDeclaringType().getASupertype*().getSourceDeclaration() instanceof
+      TypeObjectOutputStream
   }
 }
 
 /**
  * A method named `replaceObject` declared in
- * the class `java.io.ObjectInputStream` or a subclass thereof.
+ * the class `java.io.ObjectOutputStream` or a subclass thereof.
  */
 class ReplaceObjectMethod extends Method {
   ReplaceObjectMethod() {
     this.hasName("replaceObject") and
     this.getNumberOfParameters() = 1 and
     this.getParameter(0).getType() instanceof TypeObject and
-    this.getDeclaringType()
-        .getASupertype*()
-        .getSourceDeclaration()
-        .hasQualifiedName("java.io", "ObjectOutputStream")
+    this.getDeclaringType().getASupertype*().getSourceDeclaration() instanceof
+      TypeObjectOutputStream
   }
 }
 
@@ -453,10 +443,7 @@ class EnableResolveObjectMethod extends Method {
     this.hasName("enableResolveObject") and
     this.getNumberOfParameters() = 1 and
     this.getParameter(0).getType().(PrimitiveType).hasName("boolean") and
-    this.getDeclaringType()
-        .getASupertype*()
-        .getSourceDeclaration()
-        .hasQualifiedName("java.io", "ObjectInputStream")
+    this.getDeclaringType().getASupertype*().getSourceDeclaration() instanceof TypeObjectInputStream
   }
 }
 
@@ -469,10 +456,7 @@ class ResolveObjectMethod extends Method {
     this.hasName("resolveObject") and
     this.getNumberOfParameters() = 1 and
     this.getParameter(0).getType() instanceof TypeObject and
-    this.getDeclaringType()
-        .getASupertype*()
-        .getSourceDeclaration()
-        .hasQualifiedName("java.io", "ObjectInputStream")
+    this.getDeclaringType().getASupertype*().getSourceDeclaration() instanceof TypeObjectInputStream
   }
 }
 
@@ -485,10 +469,7 @@ class ResolveClassMethod extends Method {
     this.hasName("resolveClass") and
     this.getNumberOfParameters() = 1 and
     this.getParameter(0).getType().(RefType).hasQualifiedName("java.io", "ObjectStreamClass") and
-    this.getDeclaringType()
-        .getASupertype*()
-        .getSourceDeclaration()
-        .hasQualifiedName("java.io", "ObjectInputStream")
+    this.getDeclaringType().getASupertype*().getSourceDeclaration() instanceof TypeObjectInputStream
   }
 }
 
@@ -500,16 +481,8 @@ class ResolveProxyClassMethod extends Method {
   ResolveProxyClassMethod() {
     this.hasName("resolveProxyClass") and
     this.getNumberOfParameters() = 1 and
-    this.getParameter(0)
-        .getType()
-        .(Array)
-        .getComponentType()
-        .(RefType)
-        .hasQualifiedName("java.lang", "String") and
-    this.getDeclaringType()
-        .getASupertype*()
-        .getSourceDeclaration()
-        .hasQualifiedName("java.io", "ObjectInputStream")
+    this.getParameter(0).getType().(Array).getComponentType() instanceof TypeString and
+    this.getDeclaringType().getASupertype*().getSourceDeclaration() instanceof TypeObjectInputStream
   }
 }
 
@@ -598,16 +571,13 @@ class SystemOrRuntimeLoadLibraryMethod extends Method {
   SystemOrRuntimeLoadLibraryMethod() {
     (this.hasName("load") or this.hasName("loadLibrary")) and
     this.getNumberOfParameters() = 1 and
-    this.getParameter(0).getType().(RefType).hasQualifiedName("java.lang", "String") and
+    this.getParameter(0).getType() instanceof TypeString and
     (
       this.getDeclaringType()
           .getASupertype*()
           .getSourceDeclaration()
           .hasQualifiedName("java.lang", "System") or
-      this.getDeclaringType()
-          .getASupertype*()
-          .getSourceDeclaration()
-          .hasQualifiedName("java.lang", "Runtime")
+      this.getDeclaringType().getASupertype*().getSourceDeclaration() instanceof TypeRuntime
     )
   }
 }
@@ -619,9 +589,6 @@ class SystemOrRuntimeLoadLibraryMethod extends Method {
 class RuntimeExecMethod extends Method {
   RuntimeExecMethod() {
     this.hasName("exec") and
-    this.getDeclaringType()
-        .getASupertype*()
-        .getSourceDeclaration()
-        .hasQualifiedName("java.lang", "Runtime")
+    this.getDeclaringType().getASupertype*().getSourceDeclaration() instanceof TypeRuntime
   }
 }

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

@@ -22,7 +22,7 @@ private import semmle.code.java.Reflection
 
 private class ObjectInputStreamReadObjectMethod extends Method {
   ObjectInputStreamReadObjectMethod() {
-    this.getDeclaringType().getASourceSupertype*().hasQualifiedName("java.io", "ObjectInputStream") and
+    this.getDeclaringType().getASourceSupertype*() instanceof TypeObjectInputStream and
     (this.hasName("readObject") or this.hasName("readUnshared"))
   }
 }

java/ql/src/Likely Bugs/Concurrency/SynchWriteObject.ql

@@ -20,7 +20,7 @@ where
   m.getDeclaringType().getASupertype*() instanceof TypeSerializable and
   m.hasName("writeObject") and
   m.getNumberOfParameters() = 1 and
-  m.getAParamType().(Class).hasQualifiedName("java.io", "ObjectOutputStream") and
+  m.getAParamType() instanceof TypeObjectOutputStream and
   m.isSynchronized() and
   not exists(Method s |
     m.getDeclaringType().inherits(s) and

java/ql/src/Likely Bugs/Reflection/AnnotationPresentCheck.ql

@@ -19,9 +19,8 @@ where
   m.getNumberOfParameters() = 1 and
   c.getArgument(0).getType() = p and
   p.getATypeArgument() = t and
-  not exists(Annotation a |
+  not exists(RetentionAnnotation a |
     t.getAnAnnotation() = a and
-    a.getType().hasQualifiedName("java.lang.annotation", "Retention") and
     a.getAValue().(VarAccess).getVariable().hasName("RUNTIME")
   )
 select c, "Call to isAnnotationPresent where no annotation has the RUNTIME retention policy."

java/ql/src/Likely Bugs/Serialization/IncorrectSerializableMethods.java

@@ -5,6 +5,12 @@ void readObject(ObjectInputStream in) {
 		//...
 	}
 
+	// BAD: Does not match the exact signature required for a custom 
+	// deserialization protocol. Will not be called during deserialization.
+	void readObjectNoData() {
+		//...
+	}
+	
 	// BAD: Does not match the exact signature required for a custom 
 	// serialization protocol. Will not be called during serialization.
 	protected void writeObject(ObjectOutputStream out) {
@@ -18,6 +24,11 @@ private void readObject(ObjectInputStream in) {
 		//...
 	}
 
+	// GOOD: Signature for a custom deserialization implementation.
+	private void readObjectNoData() {
+		//...
+	}
+	
 	// GOOD: Signature for a custom serialization implementation.
 	private void writeObject(ObjectOutputStream out) {
 		//...

java/ql/src/Likely Bugs/Serialization/IncorrectSerializableMethods.qhelp

@@ -7,25 +7,26 @@
 <overview>
 <p>
 A serializable object that defines its own serialization protocol using the methods
-<code>readObject</code> and <code>writeObject</code> must use the signature that is expected by the 
-Java serialization framework. Otherwise, the default serialization mechanism is used.
+<code>readObject</code>, <code>readObjectNoData</code> or <code>writeObject</code> must use
+the signature that is expected by the Java serialization framework. Otherwise, the default
+serialization mechanism is used.
 </p>
 
 </overview>
 <recommendation>
 <p>
-Make sure that the signatures of <code>readObject</code> and <code>writeObject</code> on 
-serializable classes use these exact signatures:
+Make sure that the signatures of <code>readObject</code>, <code>readObjectNoData</code> and
+<code>writeObject</code> on serializable classes match these expected signatures:
 </p>
 
 <sample src="IncorrectSerializableMethodsSig.java" />
 
 </recommendation>
 <example>
 
-<p>In the following example, <code>WrongNetRequest</code> defines <code>readObject</code> and 
-<code>writeObject</code> using the wrong signatures. However, <code>NetRequest</code> defines them
-correctly.</p>
+<p>In the following example, <code>WrongNetRequest</code> defines <code>readObject</code>,
+<code>readObjectNoData</code> and <code>writeObject</code> using the wrong signatures. However,
+<code>NetRequest</code> defines them correctly.</p>
 
 <sample src="IncorrectSerializableMethods.java" />
 

java/ql/src/Likely Bugs/Serialization/IncorrectSerializableMethods.ql

@@ -1,7 +1,7 @@
 /**
  * @name Serialization methods do not match required signature
- * @description A serialized class that implements 'readObject' or 'writeObject' but does not use
- *              the correct signatures causes the default serialization mechanism to be used.
+ * @description A serialized class that implements 'readObject', 'readObjectNoData' or 'writeObject' but
+ *              does not use the correct signatures causes the default serialization mechanism to be used.
  * @kind problem
  * @problem.severity warning
  * @precision medium
@@ -13,11 +13,20 @@
 
 import java
 
-from Method m, TypeSerializable serializable
+from Method m, TypeSerializable serializable, string reason
 where
+  m.fromSource() and
   m.getDeclaringType().hasSupertype+(serializable) and
-  m.getNumberOfParameters() = 1 and
-  m.getAParameter().getType().(RefType).hasQualifiedName("java.io", "ObjectOutputStream") and
-  (m.hasName("readObject") or m.hasName("writeObject")) and
-  not m.isPrivate()
-select m, "readObject and writeObject should be private methods."
+  (
+    m.hasStringSignature("readObject(ObjectInputStream)") or
+    m.hasStringSignature("readObjectNoData()") or
+    m.hasStringSignature("writeObject(ObjectOutputStream)")
+  ) and
+  (
+    not m.isPrivate() and reason = "Method must be private"
+    or
+    m.isStatic() and reason = "Method must not be static"
+    or
+    not m.getReturnType() instanceof VoidType and reason = "Return type must be void"
+  )
+select m, "Not recognized by Java serialization framework: " + reason

java/ql/src/Likely Bugs/Serialization/IncorrectSerializableMethodsSig.java

@@ -1,4 +1,6 @@
 private void readObject(java.io.ObjectInputStream in)
      throws IOException, ClassNotFoundException;
+private void readObjectNoData()
+     throws ObjectStreamException;
 private void writeObject(java.io.ObjectOutputStream out)
      throws IOException;