Java: Refactor out flow steps for more frameworks.

Author: joefarebrother

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

@@ -2,10 +2,37 @@
  * Provides classes representing various flow steps for taint tracking.
  */
 
-import java
+private import java
+private import semmle.code.java.dataflow.DataFlow
 
 /**
- * A method that returns tainted data when one of its inputs (an argument or the qualifier) are tainted.
+ * A module importing the frameworks that implement additional flow steps,
+ * ensuring that they are visible to the taint tracking library.
+ */
+module Frameworks {
+  private import semmle.code.java.frameworks.jackson.JacksonSerializability
+  private import semmle.code.java.frameworks.android.Intent
+  private import semmle.code.java.frameworks.android.SQLite
+  private import semmle.code.java.frameworks.Guice
+  private import semmle.code.java.frameworks.Protobuf
+}
+
+/**
+ * A unit class for adding additional taint steps.
+ *
+ * Extend this class to add additional taint steps that should apply to all
+ * taint configurations.
+ */
+class AdditionalTaintStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a taint
+   * step for all configurations.
+   */
+  abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
+}
+
+/**
+ * A method that returns tainted data when one of its inputs (an argument or the qualifier) is tainted.
  *
  * Extend this class to add additional taint steps through a method that should
  * apply to all taint configurations.

java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingFrameworks.qll

@@ -7,8 +7,9 @@ private import semmle.code.java.security.SecurityTests
 private import semmle.code.java.security.Validation
 private import semmle.code.java.Maps
 private import semmle.code.java.dataflow.internal.ContainerFlow
-private import semmle.code.java.dataflow.FlowSteps
-private import semmle.code.java.dataflow.internal.TaintTrackingFrameworks
+private import semmle.code.java.frameworks.spring.SpringController
+private import semmle.code.java.frameworks.spring.SpringHttp
+import semmle.code.java.dataflow.FlowSteps
 
 /**
  * Holds if taint can flow from `src` to `sink` in zero or more
@@ -50,20 +51,6 @@ predicate localAdditionalTaintStep(DataFlow::Node src, DataFlow::Node sink) {
   )
 }
 
-/**
- * A unit class for adding additional taint steps.
- *
- * Extend this class to add additional taint steps that should apply to all
- * taint configurations.
- */
-class AdditionalTaintStep extends Unit {
-  /**
-   * Holds if the step from `node1` to `node2` should be considered a taint
-   * step for all configurations.
-   */
-  abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
-}
-
 /**
  * Holds if the additional step from `src` to `sink` should be included in all
  * global taint flow configurations.
@@ -354,8 +341,6 @@ private predicate taintPreservingQualifierToMethod(Method m) {
   m.getDeclaringType().hasQualifiedName("javax.xml.transform.stream", "StreamSource") and
   m.hasName("getInputStream")
   or
-  m instanceof IntentGetExtraMethod
-  or
   m.getDeclaringType().hasQualifiedName("java.nio", "ByteBuffer") and
   m.hasName("get")
   or
@@ -365,10 +350,6 @@ private predicate taintPreservingQualifierToMethod(Method m) {
   m.getDeclaringType().hasQualifiedName("java.net", "URI") and
   m.hasName("toURL")
   or
-  m = any(GuiceProvider gp).getAnOverridingGetMethod()
-  or
-  m = any(ProtobufMessageLite p).getAGetterMethod()
-  or
   m instanceof GetterMethod and m.getDeclaringType() instanceof SpringUntrustedDataType
   or
   m.getDeclaringType() instanceof SpringHttpEntity and
@@ -525,25 +506,10 @@ private predicate taintPreservingArgumentToMethod(Method method, int arg) {
   method.hasName("sourceToInputSource") and
   arg = 0
   or
-  exists(ProtobufParser p | method = p.getAParseFromMethod()) and
-  arg = 0
-  or
-  exists(ProtobufMessageLite m | method = m.getAParseFromMethod()) and
-  arg = 0
-  or
   method.getDeclaringType().hasQualifiedName("java.io", "StringWriter") and
   method.hasName("append") and
   arg = 0
   or
-  (
-    method.getDeclaringType() instanceof AndroidContentProvider or
-    method.getDeclaringType() instanceof AndroidContentResolver
-  ) and
-  // Cursor query(Uri uri, String[] projection, String selection, String[] selectionArgs, String sortOrder, CancellationSignal cancellationSignal)
-  // Cursor query(Uri uri, String[] projection, String selection, String[] selectionArgs, String sortOrder)
-  method.hasName("query") and
-  arg = 0
-  or
   method.(TaintPreservingMethod).returnsTaint(arg)
 }
 

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

@@ -3,6 +3,7 @@
  */
 
 import java
+import semmle.code.java.dataflow.FlowSteps
 
 /**
  * A `@com.google.inject.servlet.RequestParameters` annotation.
@@ -33,3 +34,9 @@ class GuiceProvider extends Interface {
     exists(Method m | m.getSourceDeclaration() = getGetMethod() | result.overrides*(m))
   }
 }
+
+private class OverridingGetMethod extends TaintPreservingMethod {
+  OverridingGetMethod() { this = any(GuiceProvider gp).getAnOverridingGetMethod() }
+
+  override predicate returnsTaint(int arg) { arg = -1 }
+}

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

@@ -3,6 +3,7 @@
  */
 
 import java
+import semmle.code.java.dataflow.FlowSteps
 
 /**
  * The interface `com.google.protobuf.Parser`.
@@ -54,3 +55,19 @@ class ProtobufMessageLite extends Interface {
     )
   }
 }
+
+private class TaintPreservingGetterMethod extends TaintPreservingMethod {
+  TaintPreservingGetterMethod() { this = any(ProtobufMessageLite p).getAGetterMethod() }
+
+  override predicate returnsTaint(int arg) { arg = -1 }
+}
+
+private class TaintPreservingParseFromMethod extends TaintPreservingMethod {
+  TaintPreservingParseFromMethod() {
+    exists(ProtobufParser p | this = p.getAParseFromMethod())
+    or
+    exists(ProtobufMessageLite m | this = m.getAParseFromMethod())
+  }
+
+  override predicate returnsTaint(int arg) { arg = 0 }
+}

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

@@ -1,4 +1,5 @@
 import java
+import semmle.code.java.dataflow.FlowSteps
 
 class TypeIntent extends Class {
   TypeIntent() { hasQualifiedName("android.content", "Intent") }
@@ -33,9 +34,11 @@ class ContextStartActivityMethod extends Method {
   }
 }
 
-class IntentGetExtraMethod extends Method {
+class IntentGetExtraMethod extends Method, TaintPreservingMethod {
   IntentGetExtraMethod() {
     (getName().regexpMatch("get\\w+Extra") or hasName("getExtras")) and
     getDeclaringType() instanceof TypeIntent
   }
+
+  override predicate returnsTaint(int arg) { arg = -1 }
 }

java/ql/src/semmle/code/java/frameworks/android/Intent.qll

@@ -283,3 +283,17 @@ private class UnsafeAppendUtilMethod extends TaintPreservingMethod {
 
   override predicate returnsTaint(int arg) { arg = [0 .. getNumberOfParameters()] }
 }
+
+private class TaintPreservingQueryMethod extends TaintPreservingMethod {
+  TaintPreservingQueryMethod() {
+    (
+      this.getDeclaringType() instanceof AndroidContentProvider or
+      this.getDeclaringType() instanceof AndroidContentResolver
+    ) and
+    // Cursor query(Uri uri, String[] projection, String selection, String[] selectionArgs, String sortOrder, CancellationSignal cancellationSignal)
+    // Cursor query(Uri uri, String[] projection, String selection, String[] selectionArgs, String sortOrder)
+    this.hasName("query")
+  }
+
+  override predicate returnsTaint(int arg) { arg = 0 }
+}