Apply suggestions from code review

Co-authored-by: Felicity Chapman <[email protected]>

Author: atorralba

java/change-notes/2021-10-20-android-fragment-injection-query.md

@@ -1,3 +1,3 @@
 lgtm,codescanning
-* Two new queries, "Android Fragment injection" (`java/android/fragment-injection`) and "Android Fragment injection in PreferenceActivity" (`java/android/fragment-injection-preference-activity`) have been added.
-These queries find exported Android Activities that instantiate and host Fragments created from user-provided data, which can lead to access control bypass and exposes the application to unintended effects.
+* Two new queries, "Android fragment injection" (`java/android/fragment-injection`) and "Android fragment injection in PreferenceActivity" (`java/android/fragment-injection-preference-activity`) have been added.
+These queries find exported Android activities that instantiate and host fragments created from user-provided data. Such activities are vulnerable to access control bypass and expose the Android application to unintended effects.

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

@@ -1,4 +1,4 @@
-/** Provides classes and predicates related to Android Fragments. */
+/** Provides classes and predicates to track Android fragments. */
 
 import java
 

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

@@ -35,7 +35,7 @@ class IsValidFragmentMethod extends Method {
 
 /**
  * A sink for Fragment injection vulnerabilities,
- * that is, method calls that dynamically add Fragments to Activities.
+ * that is, method calls that dynamically add fragments to activities.
  */
 abstract class FragmentInjectionSink extends DataFlow::Node { }
 

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

@@ -7,7 +7,7 @@ import semmle.code.java.security.FragmentInjection
 
 /**
  * A taint-tracking configuration for unsafe user input
- * that is used to create Android Fragments dinamically.
+ * that is used to create Android fragments dynamically.
  */
 class FragmentInjectionTaintConf extends TaintTracking::Configuration {
   FragmentInjectionTaintConf() { this = "FragmentInjectionTaintConf" }

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

@@ -3,43 +3,43 @@
 
 <overview>
 <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 a Fragment will be accessible by third-party applications (that is, exported)
-only if its hosting Activity is itself exported.
+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. 
+Thus, effectively bypassing access controls and exposing the application to unintended effects.
 </p>
 <p>
-If an exported Activity dinamically creates and hosts Fragments instantiated with externally
-provided names, 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, effectively
-bypassing access controls and exposing the application to unintended effects.
+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
-without proper validation.
+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
+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.
+<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.
+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 extending <code>PreferenceActivity</code>. The first one overrides
-<code>isValidFragment</code>, but it wrongly returns <code>true</code> inconditionally. The second Activity
-correctly overrides <code>isValidFragment</code> to only return <code>true</code> when <code>fragmentName</code>
-is a trusted Fragment name.
+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>

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

@@ -4,7 +4,7 @@ public class MyActivity extends FragmentActivity {
     protected void onCreate(Bundle savedInstance) {
         try {
             super.onCreate(savedInstance);
-            // BAD: Fragment instantiated from user input
+            // BAD: Fragment instantiated from user input without validation
             {
                 String fName = getIntent().getStringExtra("fragmentName");
                 getFragmentManager().beginTransaction().replace(com.android.internal.R.id.prefs,

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

@@ -1,7 +1,7 @@
 /**
- * @name Android Fragment injection
- * @description Instantiating an Android Fragment from a user-provided value
- *              may lead to Fragment injection.
+ * @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

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

@@ -1,7 +1,8 @@
 /**
- * @name Android Fragment injection in PreferenceActivity
- * @description An insecure implementation of the isValidFragment method
- *              of the PreferenceActivity class may lead to Fragment injection.
+ * @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