Merge pull request #20067 from owen-mc/java/unsafe-deserialization-mad-sinks

Java: allow the definition of `java/unsafe-deserialization` sinks using data extensions

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

@@ -1,8 +1,10 @@
 /**
+ * DEPRECATED: Now modeled using data extensions instead.
+ *
  * Provides classes and predicates for working with the Castor framework.
  */
 overlay[local?]
-module;
+deprecated module;
 
 import java
 
@@ -13,7 +15,9 @@ class CastorUnmarshaller extends RefType {
   CastorUnmarshaller() { this.hasQualifiedName("org.exolab.castor.xml", "Unmarshaller") }
 }
 
-/** A method with the name `unmarshal` declared in `org.exolab.castor.xml.Unmarshaller`. */
+/**
+ * A method with the name `unmarshal` declared in `org.exolab.castor.xml.Unmarshaller`.
+ */
 class CastorUnmarshalMethod extends Method {
   CastorUnmarshalMethod() {
     this.getDeclaringType() instanceof CastorUnmarshaller and

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

@@ -17,10 +17,12 @@ class UnsafeHessianInput extends RefType {
 }
 
 /**
+ * DEPRECATED: Now modeled using data extensions instead.
+ *
  * A AbstractHessianInput or Hessian2StreamingInput subclass readObject method.
  * This is either `AbstractHessianInput.readObject` or `Hessian2StreamingInput.readObject`.
  */
-class UnsafeHessianInputReadObjectMethod extends Method {
+deprecated class UnsafeHessianInputReadObjectMethod extends Method {
   UnsafeHessianInputReadObjectMethod() {
     this.getDeclaringType().getAnAncestor() instanceof UnsafeHessianInput and
     this.getName() = "readObject"
@@ -34,8 +36,12 @@ class BurlapInput extends RefType {
   BurlapInput() { this.hasQualifiedName("com.caucho.burlap.io", "BurlapInput") }
 }
 
-/** A method with the name `readObject` declared in `com.caucho.burlap.io.BurlapInput`. */
-class BurlapInputReadObjectMethod extends Method {
+/**
+ * DEPRECATED: Now modeled using data extensions instead.
+ *
+ *  A method with the name `readObject` declared in `com.caucho.burlap.io.BurlapInput`.
+ */
+deprecated class BurlapInputReadObjectMethod extends Method {
   BurlapInputReadObjectMethod() {
     this.getDeclaringType() instanceof BurlapInput and
     this.getName() = "readObject"

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

@@ -1,8 +1,10 @@
 /**
+ * DEPRECATED: Now modeled using data extensions instead.
+ *
  * Provides classes and predicates for working with the JYaml framework.
  */
 overlay[local?]
-module;
+deprecated module;
 
 import java
 

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

@@ -19,8 +19,12 @@ class JabsorbUnmarshallMethod extends Method {
   }
 }
 
-/** The deserialization method `fromJSON`. */
-class JabsorbFromJsonMethod extends Method {
+/**
+ * DEPRECATED: Now modeled using data extensions instead.
+ *
+ * The deserialization method `fromJSON`.
+ */
+deprecated class JabsorbFromJsonMethod extends Method {
   JabsorbFromJsonMethod() {
     this.getDeclaringType().getAnAncestor() instanceof JabsorbSerializer and
     this.getName() = "fromJSON"

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

@@ -13,8 +13,12 @@ class YamlBeansReader extends RefType {
   YamlBeansReader() { this.hasQualifiedName("com.esotericsoftware.yamlbeans", "YamlReader") }
 }
 
-/** A method with the name `read` declared in `com.esotericsoftware.yamlbeans.YamlReader`. */
-class YamlBeansReaderReadMethod extends Method {
+/**
+ * DEPRECATED: Now modeled using data extensions instead.
+ *
+ * A method with the name `read` declared in `com.esotericsoftware.yamlbeans.YamlReader`.
+ */
+deprecated class YamlBeansReaderReadMethod extends Method {
   YamlBeansReaderReadMethod() {
     this.getDeclaringType() instanceof YamlBeansReader and
     this.getName() = "read"

java/ql/lib/semmle/code/java/frameworks/apache/Lang.qll

@@ -16,10 +16,12 @@ class TypeApacheRandomStringUtils extends Class {
 }
 
 /**
+ * DEPRECATED: Now modeled using data extensions instead.
+ *
  * The method `deserialize` in either `org.apache.commons.lang.SerializationUtils`
  * or `org.apache.commons.lang3.SerializationUtils`.
  */
-class MethodApacheSerializationUtilsDeserialize extends Method {
+deprecated class MethodApacheSerializationUtilsDeserialize extends Method {
   MethodApacheSerializationUtilsDeserialize() {
     this.getDeclaringType()
         .hasQualifiedName(["org.apache.commons.lang", "org.apache.commons.lang3"],

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

@@ -3,17 +3,16 @@
  */
 
 import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.dataflow.ExternalFlow
 private import semmle.code.java.dataflow.FlowSinks
 private import semmle.code.java.dispatch.VirtualDispatch
 private import semmle.code.java.frameworks.Kryo
 private import semmle.code.java.frameworks.XStream
 private import semmle.code.java.frameworks.SnakeYaml
 private import semmle.code.java.frameworks.FastJson
-private import semmle.code.java.frameworks.JYaml
 private import semmle.code.java.frameworks.JsonIo
 private import semmle.code.java.frameworks.YamlBeans
 private import semmle.code.java.frameworks.HessianBurlap
-private import semmle.code.java.frameworks.Castor
 private import semmle.code.java.frameworks.Jackson
 private import semmle.code.java.frameworks.Jabsorb
 private import semmle.code.java.frameworks.Jms
@@ -51,13 +50,6 @@ private class SafeObjectInputStreamType extends RefType {
   }
 }
 
-private class XmlDecoderReadObjectMethod extends Method {
-  XmlDecoderReadObjectMethod() {
-    this.getDeclaringType().hasQualifiedName("java.beans", "XMLDecoder") and
-    this.hasName("readObject")
-  }
-}
-
 private module SafeXStreamConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node src) {
     any(XStreamEnableWhiteListing ma).getQualifier().(VarAccess).getVariable().getAnAccess() =
@@ -149,8 +141,15 @@ private module SafeKryoConfig implements DataFlow::ConfigSig {
 
 private module SafeKryoFlow = DataFlow::Global<SafeKryoConfig>;
 
+private class DefaultUnsafeDeserializationSink extends DataFlow::Node {
+  DefaultUnsafeDeserializationSink() { sinkNode(this, "unsafe-deserialization") }
+}
+
 /**
  * Holds if `ma` is a call that deserializes data from `sink`.
+ *
+ * Note that this does not include deserialization methods that have been
+ * specified using models-as-data.
  */
 predicate unsafeDeserialization(MethodCall ma, Expr sink) {
   exists(Method m | m = ma.getMethod() |
@@ -162,9 +161,6 @@ predicate unsafeDeserialization(MethodCall ma, Expr sink) {
     sink = ma.getQualifier() and
     not DataFlow::exprNode(sink).getTypeBound() instanceof SafeObjectInputStreamType
     or
-    m instanceof XmlDecoderReadObjectMethod and
-    sink = ma.getQualifier()
-    or
     m instanceof XStreamReadObjectMethod and
     sink = ma.getAnArgument() and
     not SafeXStreamFlow::flowToExpr(ma.getQualifier())
@@ -173,9 +169,6 @@ predicate unsafeDeserialization(MethodCall ma, Expr sink) {
     sink = ma.getAnArgument() and
     not SafeKryoFlow::flowToExpr(ma.getQualifier())
     or
-    m instanceof MethodApacheSerializationUtilsDeserialize and
-    sink = ma.getArgument(0)
-    or
     ma instanceof UnsafeSnakeYamlParse and
     sink = ma.getArgument(0)
     or
@@ -183,23 +176,6 @@ predicate unsafeDeserialization(MethodCall ma, Expr sink) {
     not fastJsonLooksSafe() and
     sink = ma.getArgument(0)
     or
-    ma.getMethod() instanceof JYamlLoaderUnsafeLoadMethod and
-    sink = ma.getArgument(0)
-    or
-    ma.getMethod() instanceof JsonIoJsonToJavaMethod and
-    sink = ma.getArgument(0)
-    or
-    ma.getMethod() instanceof JsonIoReadObjectMethod and
-    sink = ma.getQualifier()
-    or
-    ma.getMethod() instanceof YamlBeansReaderReadMethod and sink = ma.getQualifier()
-    or
-    ma.getMethod() instanceof UnsafeHessianInputReadObjectMethod and sink = ma.getQualifier()
-    or
-    ma.getMethod() instanceof CastorUnmarshalMethod and sink = ma.getAnArgument()
-    or
-    ma.getMethod() instanceof BurlapInputReadObjectMethod and sink = ma.getQualifier()
-    or
     ma.getMethod() instanceof ObjectMapperReadMethod and
     sink = ma.getArgument(0) and
     (
@@ -215,9 +191,6 @@ predicate unsafeDeserialization(MethodCall ma, Expr sink) {
     sink = ma.getArgument(2) and
     UnsafeTypeFlow::flowToExpr(ma.getArgument(1))
     or
-    m instanceof JabsorbFromJsonMethod and
-    sink = ma.getArgument(0)
-    or
     m instanceof JoddJsonParseMethod and
     sink = ma.getArgument(0) and
     (
@@ -244,10 +217,17 @@ predicate unsafeDeserialization(MethodCall ma, Expr sink) {
 
 /** A sink for unsafe deserialization. */
 class UnsafeDeserializationSink extends ApiSinkNode, DataFlow::ExprNode {
-  UnsafeDeserializationSink() { unsafeDeserialization(_, this.getExpr()) }
+  MethodCall mc;
+
+  UnsafeDeserializationSink() {
+    unsafeDeserialization(mc, this.getExpr())
+    or
+    this instanceof DefaultUnsafeDeserializationSink and
+    this.getExpr() = [mc.getQualifier(), mc.getAnArgument()]
+  }
 
   /** Gets a call that triggers unsafe deserialization. */
-  MethodCall getMethodCall() { unsafeDeserialization(result, this.getExpr()) }
+  MethodCall getMethodCall() { result = mc }
 }
 
 /** Holds if `node` is a sanitizer for unsafe deserialization */