Add full path reconstruction from RemoteFlowSource to sink

Author: atorralba

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

@@ -6,13 +6,32 @@ import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.TaintTracking2
 import semmle.code.java.security.AndroidIntentRedirection
 
+/**
+ * Holds if taint may flow from `source` to `sink` for `IntentRedirectionConfiguration`.
+ *
+ * It ensures that `ChangeIntentComponent` is an intermediate step in the flow.
+ */
+predicate hasIntentRedirectionFlowPath(DataFlow::PathNode source, DataFlow::PathNode sink) {
+  exists(IntentRedirectionConfiguration conf, DataFlow::PathNode intermediate |
+    intermediate.getNode().(ChangeIntentComponent).hasFlowFrom(source.getNode()) and
+    conf.hasFlowPath(intermediate, sink)
+  )
+}
+
 /**
  * A taint tracking configuration for tainted Intents being used to start Android components.
  */
-class IntentRedirectionConfiguration extends TaintTracking::Configuration {
+private class IntentRedirectionConfiguration extends TaintTracking::Configuration {
   IntentRedirectionConfiguration() { this = "IntentRedirectionConfiguration" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof IntentRedirectionSource }
+  override predicate isSource(DataFlow::Node source) {
+    exists(ChangeIntentComponent c |
+      c = source
+      or
+      // This is needed for PathGraph to be able to build the full flow
+      c.hasFlowFrom(source)
+    )
+  }
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof IntentRedirectionSink }
 
@@ -26,11 +45,16 @@ class IntentRedirectionConfiguration extends TaintTracking::Configuration {
 }
 
 /** An expression modifying an `Intent` component with tainted data. */
-private class IntentRedirectionSource extends DataFlow::Node {
-  IntentRedirectionSource() {
+private class ChangeIntentComponent extends DataFlow::Node {
+  DataFlow::Node src;
+
+  ChangeIntentComponent() {
     changesIntentComponent(this.asExpr()) and
-    exists(TaintedIntentComponentConf conf | conf.hasFlowTo(this))
+    exists(TaintedIntentComponentConf conf | conf.hasFlow(src, this))
   }
+
+  /** Holds if `source` flows to `this`. */
+  predicate hasFlowFrom(DataFlow::Node source) { source = src }
 }
 
 /**
@@ -46,7 +70,7 @@ private class TaintedIntentComponentConf extends TaintTracking2::Configuration {
 
 /** Holds if `expr` modifies the component of an `Intent`. */
 private predicate changesIntentComponent(Expr expr) {
-  any(IntentGetParcelableExtra igpe).getQualifier() = expr or
+  any(IntentGetParcelableExtra igpe) = expr or
   any(IntentSetComponent isc).getSink() = expr
 }
 

java/ql/src/Security/CWE/CWE-940/AndroidIntentRedirection.ql

@@ -18,8 +18,8 @@ import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.security.AndroidIntentRedirectionQuery
 import DataFlow::PathGraph
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, IntentRedirectionConfiguration conf
-where conf.hasFlowPath(source, sink)
+from DataFlow::PathNode source, DataFlow::PathNode sink
+where hasIntentRedirectionFlowPath(source, sink)
 select sink.getNode(), source, sink,
   "Arbitrary Android activities or services can be started from $@.", source.getNode(),
   "this user input"

java/ql/test/query-tests/security/CWE-940/AndroidIntentRedirectionTest.java

@@ -11,19 +11,6 @@ public class AndroidIntentRedirectionTest extends Activity {
     public void onCreate(Bundle savedInstanceState) {
         Intent intent = (Intent) getIntent().getParcelableExtra("forward_intent");
 
-        if (intent.getComponent().getPackageName().equals("something")) {
-            startActivity(intent); // Safe - sanitized
-        } else {
-            startActivity(intent); // $ hasAndroidIntentRedirection
-        }
-        if (intent.getComponent().getClassName().equals("something")) {
-            startActivity(intent); // Safe - sanitized
-        } else {
-            startActivity(intent); // $ hasAndroidIntentRedirection
-        }
-
-        startActivity(getIntent()); // Safe - not an intent obtained from the Extras
-
         // @formatter:off
         startActivities(new Intent[] {intent}); // $ hasAndroidIntentRedirection
         startActivities(new Intent[] {intent}, null); // $ hasAndroidIntentRedirection
@@ -56,6 +43,17 @@ public void onCreate(Bundle savedInstanceState) {
         sendStickyOrderedBroadcastAsUser(intent, null, null, null, 0, null, null); // $ hasAndroidIntentRedirection
         // @formatter:on
 
+        if (intent.getComponent().getPackageName().equals("something")) {
+            startActivity(intent); // Safe - sanitized
+        } else {
+            startActivity(intent); // $ hasAndroidIntentRedirection
+        }
+        if (intent.getComponent().getClassName().equals("something")) {
+            startActivity(intent); // Safe - sanitized
+        } else {
+            startActivity(intent); // $ hasAndroidIntentRedirection
+        }
+
         try {
             {
                 Intent fwdIntent = new Intent();
@@ -134,6 +132,25 @@ public void onCreate(Bundle savedInstanceState) {
                 fwdIntent.setComponent(component);
                 startActivity(fwdIntent); // $ hasAndroidIntentRedirection
             }
+            {
+                Intent originalIntent = getIntent();
+                Intent fwdIntent = (Intent) originalIntent.getParcelableExtra("forward_intent");
+                startActivity(originalIntent); // Safe - not an Intent obtained from the Extras
+            }
+            {
+                Intent originalIntent = getIntent();
+                ComponentName cp = new ComponentName(originalIntent.getStringExtra("packageName"),
+                        originalIntent.getStringExtra("className"));
+                Intent anotherIntent = new Intent();
+                anotherIntent.setComponent(cp);
+                startActivity(originalIntent); // Safe - not a tainted Intent
+            }
+            {
+                // Delayed cast
+                Object obj = getIntent().getParcelableExtra("forward_intent");
+                Intent fwdIntent = (Intent) obj;
+                startActivity(fwdIntent); // $ hasAndroidIntentRedirection
+            }
         } catch (Exception e) {
         }
     }

java/ql/test/query-tests/security/CWE-940/AndroidIntentRedirectionTest.ql

@@ -9,10 +9,10 @@ class HasAndroidIntentRedirectionTest extends InlineExpectationsTest {
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {
     tag = "hasAndroidIntentRedirection" and
-    exists(DataFlow::Node src, DataFlow::Node sink, IntentRedirectionConfiguration conf |
-      conf.hasFlow(src, sink)
+    exists(DataFlow::PathNode src, DataFlow::PathNode sink |
+      hasIntentRedirectionFlowPath(src, sink)
     |
-      sink.getLocation() = location and
+      sink.getNode().getLocation() = location and
       element = sink.toString() and
       value = ""
     )