Skip to content

Commit 701d12f

Browse files
atorralbaatorralba
committed
Add Fragment injection query
1 parent efb4716 commit 701d12f

12 files changed

Lines changed: 275 additions & 0 deletions

File tree

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import java
2+
3+
/** The class `android.app.Fragment` */
4+
class Fragment extends Class {
5+
Fragment() { this.hasQualifiedName("android.app", "Fragment") }
6+
}
7+
8+
/** The method `instantiate` of the class `android.app.Fragment`. */
9+
class FragmentInstantiateMethod extends Method {
10+
FragmentInstantiateMethod() {
11+
this.getDeclaringType() instanceof Fragment and
12+
this.hasName("instantiate")
13+
}
14+
}

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

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import java
2+
private import semmle.code.java.dataflow.TaintTracking
3+
private import semmle.code.java.dataflow.ExternalFlow
4+
private import semmle.code.java.frameworks.android.Android
5+
private import semmle.code.java.frameworks.android.Fragment
6+
private import semmle.code.java.Reflection
7+
8+
/**
9+
* A sink for Fragment injection vulnerabilities,
10+
* that is, method calls that dynamically add Fragments to Activities.
11+
*/
12+
abstract class FragmentInjectionSink extends DataFlow::Node { }
13+
14+
/**
15+
* A unit class for adding additional taint steps.
16+
*
17+
* Extend this class to add additional taint steps that should apply to `FragmentInjectionTaintConf`.
18+
*/
19+
class FragmentInjectionAdditionalTaintStep extends Unit {
20+
abstract predicate step(DataFlow::Node n1, DataFlow::Node n2);
21+
}
22+
23+
private class FragmentInjectionSinkModels extends SinkModelCsv {
24+
override predicate row(string row) {
25+
row =
26+
["android.app", "android.support.v4.app", "androidx.fragment.app"] +
27+
";FragmentTransaction;true;" +
28+
[
29+
"add;(Class,Bundle,String);;Argument[0]", "add;(Fragment,String);;Argument[0]",
30+
"add;(int,Class,Bundle);;Argument[1]", "add;(int,Fragment);;Argument[1]",
31+
"add;(int,Class,Bundle,String);;Argument[1]", "add;(int,Fragment,String);;Argument[1]",
32+
"attach;(Fragment);;Argument[0]", "replace;(int,Class,Bundle);;Argument[1]",
33+
"replace;(int,Fragment);;Argument[1]", "replace;(int,Class,Bundle,String);;Argument[1]",
34+
"replace;(int,Fragment,String);;Argument[1]",
35+
] + ";fragment-injection"
36+
}
37+
}
38+
39+
private class DefaultFragmentInjectionSink extends FragmentInjectionSink {
40+
DefaultFragmentInjectionSink() { sinkNode(this, "fragment-injection") }
41+
}
42+
43+
private class DefaultFragmentInjectionAdditionalTaintStep extends FragmentInjectionAdditionalTaintStep {
44+
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
45+
exists(ReflectiveClassIdentifierMethodAccess ma |
46+
ma.getArgument(0) = n1.asExpr() and ma = n2.asExpr()
47+
)
48+
or
49+
exists(NewInstance ni |
50+
ni.getQualifier() = n1.asExpr() and
51+
ni = n2.asExpr()
52+
)
53+
or
54+
exists(MethodAccess ma |
55+
ma.getMethod() instanceof FragmentInstantiateMethod and
56+
ma.getArgument(1) = n1.asExpr() and
57+
ma = n2.asExpr()
58+
)
59+
}
60+
}

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import java
2+
import semmle.code.java.dataflow.FlowSources
3+
import semmle.code.java.dataflow.TaintTracking
4+
import semmle.code.java.security.FragmentInjection
5+
6+
/**
7+
* A taint-tracking configuration for unsafe user input
8+
* that is used to create Android Fragments dinamically.
9+
*/
10+
class FragmentInjectionTaintConf extends TaintTracking::Configuration {
11+
FragmentInjectionTaintConf() { this = "FragmentInjectionTaintConf" }
12+
13+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
14+
15+
override predicate isSink(DataFlow::Node sink) { sink instanceof FragmentInjectionSink }
16+
17+
override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
18+
any(FragmentInjectionAdditionalTaintStep c).step(n1, n2)
19+
}
20+
}

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

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>
6+
Fragments are reusable parts of an Android application's user interface.
7+
Even though a Fragment controls its own lifecycle and layout and handles its input events,
8+
it cannot exist on its own: it must be hosted either by an Activity or another fragment.
9+
This means that a Fragment will be accessible by third-party applications (that is, exported)
10+
only if its hosting Activity is itself exported.
11+
</p>
12+
<p>
13+
If an exported Activity dinamically creates and hosts Fragments instantiated with externally
14+
provided names, a malicious application could provide the name of an arbitrary Fragment, even
15+
one not designed to be externally accessible, and inject it into the Activity, effectively
16+
bypassing access controls and exposing the application to unintended effects.
17+
</p>
18+
</overview>
19+
20+
<recommendation>
21+
<p>
22+
In general, do not instantiate classes (including Fragments) with user-provided names
23+
without proper validation.
24+
25+
Also, if an exported Activity is extending the <code>PreferenceActivity</code> class, make sure that
26+
the <code>isValidFragment</code> method is overriden and only returns <code>true</code> when the provided
27+
<code>fragmentName</code> points to an intended Fragment.
28+
</p>
29+
</recommendation>
30+
31+
<example>
32+
<p>
33+
The following example shows two cases: in the first one, untrusted data is used to instantiate and
34+
add a Fragment to an Activity, while in the second one, a Fragment is safely added with a static name.
35+
</p>
36+
<sample src="FragmentInjection.java" />
37+
38+
<p>
39+
The next example shows two Activities extending <code>PreferenceActivity</code>. The first one overrides
40+
<code>isValidFragment</code>, but it wrongly returns <code>true</code> inconditionally. The second Activity
41+
correctly overrides <code>isValidFragment</code> to only return <code>true</code> when <code>fragmentName</code>
42+
is a trusted Fragment name.
43+
</p>
44+
<sample src="FragmentInjectionInPreferenceActivity.java" />
45+
</example>
46+
47+
<references>
48+
<li> Google Help:
49+
<a href="https://support.google.com/faqs/answer/7188427?hl=en">How to fix Fragment Injection vulnerability</a>.
50+
</li>
51+
<li>
52+
IBM Security Systems:
53+
<a href="https://securityintelligence.com/wp-content/uploads/2013/12/android-collapses-into-fragments.pdf">Android collapses into Fragments</a>.
54+
</li>
55+
<li>
56+
Android Developers:
57+
<a href="https://developer.android.com/guide/fragments">Fragments</a>
58+
</li>
59+
</references>
60+
</qhelp>

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
public class MyActivity extends FragmentActivity {
2+
3+
@Override
4+
protected void onCreate(Bundle savedInstance) {
5+
try {
6+
super.onCreate(savedInstance);
7+
// BAD: Fragment instantiated from user input
8+
{
9+
String fName = getIntent().getStringExtra("fragmentName");
10+
getFragmentManager().beginTransaction().replace(com.android.internal.R.id.prefs,
11+
Fragment.instantiate(this, fName, null)).commit();
12+
}
13+
// GOOD: Fragment instantiated statically
14+
{
15+
getFragmentManager().beginTransaction()
16+
.replace(com.android.internal.R.id.prefs, new MyFragment()).commit();
17+
}
18+
} catch (Exception e) {
19+
}
20+
}
21+
22+
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
<include src="FragmentInjection.inc.qhelp"></include>
4+
</qhelp>

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* @name Android Fragment injection
3+
* @description Instantiating an Android Fragment from a user-provided value
4+
* may lead to Fragment injection.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 9.8
8+
* @precision high
9+
* @id java/android/fragment-injection
10+
* @tags security
11+
* external/cwe/cwe-470
12+
*/
13+
14+
import java
15+
import semmle.code.java.security.FragmentInjectionQuery
16+
import DataFlow::PathGraph
17+
18+
from DataFlow::PathNode source, DataFlow::PathNode sink
19+
where any(FragmentInjectionTaintConf conf).hasFlowPath(source, sink)
20+
select sink.getNode(), source, sink, "Fragment injection from $@.", source.getNode(),
21+
"this user input"

‎java/ql/test/query-tests/security/CWE-470/AndroidManifest.xml‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<manifest
3+
xmlns:android="http://schemas.android.com/apk/res/android"
4+
android:versionCode="1"
5+
android:versionName="1.0"
6+
package="com.example.myapp">
7+
8+
<application
9+
android:allowBackup="true"
10+
android:icon="@mipmap/ic_launcher"
11+
android:roundIcon="@mipmap/ic_launcher_round"
12+
android:label="@string/app_name"
13+
android:supportsRtl="true"
14+
android:theme="@style/AppTheme">
15+
16+
<activity android:name=".MainActivity">
17+
<intent-filter>
18+
<action android:name="android.intent.action.MAIN" />
19+
<category android:name="android.intent.category.LAUNCHER" />
20+
</intent-filter>
21+
</activity>
22+
</application>
23+
</manifest>

‎java/ql/test/query-tests/security/CWE-470/FragmentInjectionTest.expected‎

Whitespace-only changes.

‎java/ql/test/query-tests/security/CWE-470/FragmentInjectionTest.ql‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import java
2+
import semmle.code.java.security.FragmentInjectionQuery
3+
import TestUtilities.InlineFlowTest
4+
5+
class Test extends InlineFlowTest {
6+
override DataFlow::Configuration getValueFlowConfig() { none() }
7+
8+
override TaintTracking::Configuration getTaintFlowConfig() {
9+
result instanceof FragmentInjectionTaintConf
10+
}
11+
}

0 commit comments

Comments
 (0)