Merge pull request #8956 from atorralba/atorralba/intent-redirection-sanitizer-fix

Java: Fix Intent Redirection sanitizer

Author: atorralba

java/ql/lib/change-notes/2022-04-29-intent-redirection-sanitizer-fix.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+Fixed a sanitizer of the query `java/android/intent-redirection`. Now, for an intent to be considered
+safe against intent redirection, both its package name and class name must be checked.

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

@@ -65,16 +65,24 @@ private class DefaultIntentRedirectionSink extends IntentRedirectionSink {
 }
 
 /**
- * A default sanitizer for nodes dominated by calls to `ComponentName.getPackageName`
- * or `ComponentName.getClassName`. These are used to check whether the origin or destination
+ * A default sanitizer for `Intent` nodes dominated by calls to `ComponentName.getPackageName`
+ * and `ComponentName.getClassName`. These are used to check whether the origin or destination
  * components are trusted.
  */
 private class DefaultIntentRedirectionSanitizer extends IntentRedirectionSanitizer {
   DefaultIntentRedirectionSanitizer() {
+    this.getType() instanceof TypeIntent and
     exists(MethodAccess ma, Method m, Guard g, boolean branch |
       ma.getMethod() = m and
       m.getDeclaringType() instanceof TypeComponentName and
-      m.hasName(["getPackageName", "getClassName"]) and
+      m.hasName("getPackageName") and
+      g.isEquality(ma, _, branch) and
+      g.controls(this.asExpr().getBasicBlock(), branch)
+    ) and
+    exists(MethodAccess ma, Method m, Guard g, boolean branch |
+      ma.getMethod() = m and
+      m.getDeclaringType() instanceof TypeComponentName and
+      m.hasName("getClassName") and
       g.isEquality(ma, _, branch) and
       g.controls(this.asExpr().getBasicBlock(), branch)
     )

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

@@ -40,13 +40,23 @@ public void onCreate(Bundle savedInstanceState) {
         sendStickyOrderedBroadcastAsUser(intent, null, null, null, 0, null, null); // $ hasAndroidIntentRedirection
         // @formatter:on
 
+        // Sanitizing only the package or the class still allows redirecting
+        // to non-exported activities in the same package
+        // or activities with the same name in other packages, respectively.
         if (intent.getComponent().getPackageName().equals("something")) {
-            startActivity(intent); // Safe - sanitized
+            startActivity(intent); // $ hasAndroidIntentRedirection
         } else {
             startActivity(intent); // $ hasAndroidIntentRedirection
         }
         if (intent.getComponent().getClassName().equals("something")) {
-            startActivity(intent); // Safe - sanitized
+            startActivity(intent); // $ hasAndroidIntentRedirection
+        } else {
+            startActivity(intent); // $ hasAndroidIntentRedirection
+        }
+
+        if (intent.getComponent().getPackageName().equals("something")
+                && intent.getComponent().getClassName().equals("something")) {
+            startActivity(intent); // Safe
         } else {
             startActivity(intent); // $ hasAndroidIntentRedirection
         }
@@ -94,8 +104,7 @@ public void onCreate(Bundle savedInstanceState) {
             }
             {
                 Intent fwdIntent = new Intent();
-                ComponentName component =
-                        new ComponentName("", intent.getStringExtra("className"));
+                ComponentName component = new ComponentName("", intent.getStringExtra("className"));
                 fwdIntent.setComponent(component);
                 startActivity(fwdIntent); // $ hasAndroidIntentRedirection
             }