Introduce TaintInheritingContent instead of using parts of DataFlowPrivate

Author: smowton

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

@@ -103,3 +103,19 @@ private class NumberTaintPreservingCallable extends TaintPreservingCallable {
 
   override predicate returnsTaintFrom(int arg) { arg = argument }
 }
+
+/**
+ * A `Content` that should be implicitly regarded as tainted whenever an object with such `Content`
+ * is itself tainted.
+ *
+ * For example, if we had a type `class Container { Contained field; }`, then by default a tainted
+ * `Container` and a `Container` with a tainted `Contained` stored in its `field` are distinct.
+ *
+ * If `any(DataFlow::FieldContent fc | fc.getField().hasQualifiedName("Container", "field"))` was
+ * included in this type however, then a tainted `Container` would imply that its `field` is also
+ * tainted (but not vice versa).
+ *
+ * Note that `TaintTracking::Configuration` applies this behaviour by default to array, collection,
+ * map-key and map-value content, so that e.g. a tainted `Map` is assumed to have tainted keys and values.
+ */
+abstract class TaintInheritingContent extends DataFlow::Content { }

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

@@ -83,7 +83,11 @@ private module Cached {
       not sink.getTypeBound() instanceof PrimitiveType and
       not sink.getTypeBound() instanceof BoxedType and
       not sink.getTypeBound() instanceof NumberType and
-      containerContent(f)
+      (
+        containerContent(f)
+        or
+        f instanceof TaintInheritingContent
+      )
     )
     or
     FlowSummaryImpl::Private::Steps::summaryLocalStep(src, sink, false)

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

@@ -1,4 +1,5 @@
 import java
+private import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.FlowSteps
 import semmle.code.java.dataflow.ExternalFlow
 
@@ -54,6 +55,14 @@ class BundleGetterMethod extends Method, TaintPreservingCallable {
   override predicate returnsTaintFrom(int arg) { arg = -1 }
 }
 
+/**
+ * Specifies that if an `Intent` is tainted, then so are its synthetic fields.
+ */
+private class IntentFieldsInheritTaint extends DataFlow::SyntheticFieldContent,
+  TaintInheritingContent {
+  IntentFieldsInheritTaint() { this.getField().matches("android.content.Intent.%") }
+}
+
 private class IntentBundleFlowSteps extends SummaryModelCsv {
   override predicate row(string row) {
     row =

java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.ql

@@ -14,8 +14,6 @@ import semmle.code.java.controlflow.Guards
 import AndroidFileIntentSink
 import AndroidFileIntentSource
 import DataFlow::PathGraph
-// For readStep, to implement `isAdditionalTaintStep`
-private import semmle.code.java.dataflow.internal.DataFlowPrivate
 
 private class StartsWithSanitizer extends DataFlow::BarrierGuard {
   StartsWithSanitizer() { this.(MethodAccess).getMethod().hasName("startsWith") }
@@ -73,11 +71,6 @@ class AndroidFileLeakConfig extends TaintTracking::Configuration {
       prev.asExpr() = csma.getArgument(0) and
       succ.asParameter() = ssm.getParameter(0) // public int onStartCommand(Intent intent, int flags, int startId) {...} in FileUploader
     )
-    or
-    // When a whole Intent is tainted (e.g., due to this Configuration's source), treat its fields as tainted
-    readStep(prev,
-      any(DataFlow::SyntheticFieldContent c | c.getField().matches("android.content.Intent.%")),
-      succ)
   }
 
   override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {