Apply suggestions from code review

Co-authored-by: Felicity Chapman <felicitymay@github.com>
This commit is contained in:
Tony Torralba
2021-12-15 15:59:40 +01:00
parent 65b6c16254
commit f0e9b768f2
8 changed files with 31 additions and 30 deletions

View File

@@ -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>

View File

@@ -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,

View File

@@ -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

View File

@@ -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