Merge pull request #6923 from atorralba/atorralba/android-fragment-injection

Java: CWE-470  - Queries to detect Fragment Injection in Android applications

Author: atorralba

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

@@ -0,0 +1,16 @@
+/** Provides classes and predicates to track Android fragments. */
+
+import java
+
+/** The class `android.app.Fragment` */
+class Fragment extends Class {
+  Fragment() { this.hasQualifiedName("android.app", "Fragment") }
+}
+
+/** The method `instantiate` of the class `android.app.Fragment`. */
+class FragmentInstantiateMethod extends Method {
+  FragmentInstantiateMethod() {
+    this.getDeclaringType() instanceof Fragment and
+    this.hasName("instantiate")
+  }
+}

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

@@ -0,0 +1,83 @@
+/** Provides classes and predicates to reason about Android Fragment injection vulnerabilities. */
+
+import java
+private import semmle.code.java.dataflow.TaintTracking
+private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.frameworks.android.Android
+private import semmle.code.java.frameworks.android.Fragment
+private import semmle.code.java.Reflection
+
+/** The method `isValidFragment` of the class `android.preference.PreferenceActivity`. */
+class IsValidFragmentMethod extends Method {
+  IsValidFragmentMethod() {
+    this.getDeclaringType()
+        .getASupertype*()
+        .hasQualifiedName("android.preference", "PreferenceActivity") and
+    this.hasName("isValidFragment")
+  }
+
+  /**
+   * Holds if this method makes the Activity it is declared in vulnerable to Fragment injection,
+   * that is, all code paths in this method return `true` and the Activity is exported.
+   */
+  predicate isUnsafe() {
+    this.getDeclaringType().(AndroidActivity).isExported() and
+    forex(ReturnStmt retStmt | retStmt.getEnclosingCallable() = this |
+      retStmt.getResult().(BooleanLiteral).getBooleanValue() = true
+    )
+  }
+}
+
+/**
+ * A sink for Fragment injection vulnerabilities,
+ * that is, method calls that dynamically add fragments to activities.
+ */
+abstract class FragmentInjectionSink extends DataFlow::Node { }
+
+/** An additional taint step for flows related to Fragment injection vulnerabilites. */
+class FragmentInjectionAdditionalTaintStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a taint
+   * step in flows related to Fragment injection vulnerabilites.
+   */
+  abstract predicate step(DataFlow::Node n1, DataFlow::Node n2);
+}
+
+private class FragmentInjectionSinkModels extends SinkModelCsv {
+  override predicate row(string row) {
+    row =
+      ["android.app", "android.support.v4.app", "androidx.fragment.app"] +
+        ";FragmentTransaction;true;" +
+        [
+          "add;(Class,Bundle,String);;Argument[0]", "add;(Fragment,String);;Argument[0]",
+          "add;(int,Class,Bundle);;Argument[1]", "add;(int,Fragment);;Argument[1]",
+          "add;(int,Class,Bundle,String);;Argument[1]", "add;(int,Fragment,String);;Argument[1]",
+          "attach;(Fragment);;Argument[0]", "replace;(int,Class,Bundle);;Argument[1]",
+          "replace;(int,Fragment);;Argument[1]", "replace;(int,Class,Bundle,String);;Argument[1]",
+          "replace;(int,Fragment,String);;Argument[1]",
+        ] + ";fragment-injection"
+  }
+}
+
+private class DefaultFragmentInjectionSink extends FragmentInjectionSink {
+  DefaultFragmentInjectionSink() { sinkNode(this, "fragment-injection") }
+}
+
+private class DefaultFragmentInjectionAdditionalTaintStep extends FragmentInjectionAdditionalTaintStep {
+  override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
+    exists(ReflectiveClassIdentifierMethodAccess ma |
+      ma.getArgument(0) = n1.asExpr() and ma = n2.asExpr()
+    )
+    or
+    exists(NewInstance ni |
+      ni.getQualifier() = n1.asExpr() and
+      ni = n2.asExpr()
+    )
+    or
+    exists(MethodAccess ma |
+      ma.getMethod() instanceof FragmentInstantiateMethod and
+      ma.getArgument(1) = n1.asExpr() and
+      ma = n2.asExpr()
+    )
+  }
+}

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

@@ -0,0 +1,22 @@
+/** Provides classes and predicates to be used in queries related to Android Fragment injection. */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.security.FragmentInjection
+
+/**
+ * A taint-tracking configuration for unsafe user input
+ * that is used to create Android fragments dynamically.
+ */
+class FragmentInjectionTaintConf extends TaintTracking::Configuration {
+  FragmentInjectionTaintConf() { this = "FragmentInjectionTaintConf" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof FragmentInjectionSink }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
+    any(FragmentInjectionAdditionalTaintStep c).step(n1, n2)
+  }
+}

java/ql/src/Security/CWE/CWE-470/FragmentInjection.inc.qhelp

@@ -0,0 +1,60 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+When fragments are instantiated with externally provided names, this exposes any exported activity that dynamically
+creates and hosts the fragment to fragment injection. A malicious application could provide the 
+name of an arbitrary fragment, even one not designed to be externally accessible, and inject it into the activity. 
+This can bypass access controls and expose the application to unintended effects.
+</p>
+<p>
+Fragments are reusable parts of an Android application's user interface.
+Even though a fragment controls its own lifecycle and layout, and handles its input events,
+it cannot exist on its own: it must be hosted either by an activity or another fragment.
+This means that, normally, a fragment will be accessible by third-party applications (that is, exported)
+only if its hosting activity is itself exported.
+</p>
+</overview>
+
+<recommendation>
+<p>
+In general, do not instantiate classes (including fragments) with user-provided names
+unless the name has been properly validated.
+
+Also, if an exported activity is extending the <code>PreferenceActivity</code> class, make sure that
+the <code>isValidFragment</code> method is overriden and only returns <code>true</code> when the provided
+<code>fragmentName</code> points to an intended fragment.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows two cases: in the first one, untrusted data is used to instantiate and
+add a fragment to an activity, while in the second one, a fragment is safely added with a static name.
+</p>
+<sample src="FragmentInjection.java" />
+
+<p>
+The next example shows two activities that extend <code>PreferenceActivity</code>. The first activity overrides
+<code>isValidFragment</code>, but it wrongly returns <code>true</code> unconditionally. The second activity
+correctly overrides <code>isValidFragment</code> so that it only returns <code>true</code> when <code>fragmentName</code>
+is a trusted fragment name.
+</p>
+<sample src="FragmentInjectionInPreferenceActivity.java" />
+</example>
+
+<references>
+<li> Google Help:
+  <a href="https://support.google.com/faqs/answer/7188427?hl=en">How to fix Fragment Injection vulnerability</a>.
+</li>
+<li>
+  IBM Security Systems:
+  <a href="https://securityintelligence.com/wp-content/uploads/2013/12/android-collapses-into-fragments.pdf">Android collapses into Fragments</a>.
+</li>
+<li>
+  Android Developers:
+  <a href="https://developer.android.com/guide/fragments">Fragments</a>
+</li>
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-470/FragmentInjection.java

@@ -0,0 +1,22 @@
+public class MyActivity extends FragmentActivity {
+
+    @Override
+    protected void onCreate(Bundle savedInstance) {
+        try {
+            super.onCreate(savedInstance);
+            // BAD: Fragment instantiated from user input without validation
+            {
+                String fName = getIntent().getStringExtra("fragmentName");
+                getFragmentManager().beginTransaction().replace(com.android.internal.R.id.prefs,
+                        Fragment.instantiate(this, fName, null)).commit();
+            }
+            // GOOD: Fragment instantiated statically
+            {
+                getFragmentManager().beginTransaction()
+                        .replace(com.android.internal.R.id.prefs, new MyFragment()).commit();
+            }
+        } catch (Exception e) {
+        }
+    }
+
+}

java/ql/src/Security/CWE/CWE-470/FragmentInjection.qhelp

@@ -0,0 +1,4 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+<include src="FragmentInjection.inc.qhelp"></include>
+</qhelp>

java/ql/src/Security/CWE/CWE-470/FragmentInjection.ql

@@ -0,0 +1,21 @@
+/**
+ * @name Android fragment injection
+ * @description Instantiating an Android fragment from a user-provided value
+ *              may allow a malicious application to bypass access controls,  exposing the application to unintended effects.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 9.8
+ * @precision high
+ * @id java/android/fragment-injection
+ * @tags security
+ *       external/cwe/cwe-470
+ */
+
+import java
+import semmle.code.java.security.FragmentInjectionQuery
+import DataFlow::PathGraph
+
+from DataFlow::PathNode source, DataFlow::PathNode sink
+where any(FragmentInjectionTaintConf conf).hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Fragment injection from $@.", source.getNode(),
+  "this user input"

java/ql/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.java

@@ -0,0 +1,21 @@
+class UnsafeActivity extends PreferenceActivity {
+
+    @Override
+    protected boolean isValidFragment(String fragmentName) {
+        // BAD: any Fragment name can be provided.
+        return true;
+    }
+}
+
+
+class SafeActivity extends PreferenceActivity {
+    @Override
+    protected boolean isValidFragment(String fragmentName) {
+        // Good: only trusted Fragment names are allowed.
+        return SafeFragment1.class.getName().equals(fragmentName)
+                || SafeFragment2.class.getName().equals(fragmentName)
+                || SafeFragment3.class.getName().equals(fragmentName);
+    }
+
+}
+

java/ql/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.qhelp

@@ -0,0 +1,4 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+<include src="FragmentInjection.inc.qhelp"></include>
+</qhelp>

java/ql/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.ql

@@ -0,0 +1,22 @@
+/**
+ * @name Android fragment injection in PreferenceActivity
+ * @description An insecure implementation of the 'isValidFragment' method
+ *              of the 'PreferenceActivity' class may allow a malicious application to bypass access controls,
+ *              exposing the application to unintended effects.
+ * @kind problem
+ * @problem.severity error
+ * @security-severity 9.8
+ * @precision high
+ * @id java/android/fragment-injection-preference-activity
+ * @tags security
+ *       external/cwe/cwe-470
+ */
+
+import java
+import semmle.code.java.security.FragmentInjection
+
+from IsValidFragmentMethod m
+where m.isUnsafe()
+select m,
+  "The 'isValidFragment' method always returns true. This makes the exported Activity $@ vulnerable to Fragment Injection.",
+  m.getDeclaringType(), m.getDeclaringType().getName()