Skip to content

Commit 2279295

Browse files
atorralbaatorralba
authored
Merge pull request #6923 from atorralba/atorralba/android-fragment-injection
Java: CWE-470 - Queries to detect Fragment Injection in Android applications
2 parents 3c837c3 + 7beab7c commit 2279295

35 files changed

+4536
-662
lines changed

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

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

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

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
/** Provides classes and predicates to reason about Android Fragment injection vulnerabilities. */
2+
3+
import java
4+
private import semmle.code.java.dataflow.TaintTracking
5+
private import semmle.code.java.dataflow.ExternalFlow
6+
private import semmle.code.java.frameworks.android.Android
7+
private import semmle.code.java.frameworks.android.Fragment
8+
private import semmle.code.java.Reflection
9+
10+
/** The method `isValidFragment` of the class `android.preference.PreferenceActivity`. */
11+
class IsValidFragmentMethod extends Method {
12+
IsValidFragmentMethod() {
13+
this.getDeclaringType()
14+
.getASupertype*()
15+
.hasQualifiedName("android.preference", "PreferenceActivity") and
16+
this.hasName("isValidFragment")
17+
}
18+
19+
/**
20+
* Holds if this method makes the Activity it is declared in vulnerable to Fragment injection,
21+
* that is, all code paths in this method return `true` and the Activity is exported.
22+
*/
23+
predicate isUnsafe() {
24+
this.getDeclaringType().(AndroidActivity).isExported() and
25+
forex(ReturnStmt retStmt | retStmt.getEnclosingCallable() = this |
26+
retStmt.getResult().(BooleanLiteral).getBooleanValue() = true
27+
)
28+
}
29+
}
30+
31+
/**
32+
* A sink for Fragment injection vulnerabilities,
33+
* that is, method calls that dynamically add fragments to activities.
34+
*/
35+
abstract class FragmentInjectionSink extends DataFlow::Node { }
36+
37+
/** An additional taint step for flows related to Fragment injection vulnerabilites. */
38+
class FragmentInjectionAdditionalTaintStep extends Unit {
39+
/**
40+
* Holds if the step from `node1` to `node2` should be considered a taint
41+
* step in flows related to Fragment injection vulnerabilites.
42+
*/
43+
abstract predicate step(DataFlow::Node n1, DataFlow::Node n2);
44+
}
45+
46+
private class FragmentInjectionSinkModels extends SinkModelCsv {
47+
override predicate row(string row) {
48+
row =
49+
["android.app", "android.support.v4.app", "androidx.fragment.app"] +
50+
";FragmentTransaction;true;" +
51+
[
52+
"add;(Class,Bundle,String);;Argument[0]", "add;(Fragment,String);;Argument[0]",
53+
"add;(int,Class,Bundle);;Argument[1]", "add;(int,Fragment);;Argument[1]",
54+
"add;(int,Class,Bundle,String);;Argument[1]", "add;(int,Fragment,String);;Argument[1]",
55+
"attach;(Fragment);;Argument[0]", "replace;(int,Class,Bundle);;Argument[1]",
56+
"replace;(int,Fragment);;Argument[1]", "replace;(int,Class,Bundle,String);;Argument[1]",
57+
"replace;(int,Fragment,String);;Argument[1]",
58+
] + ";fragment-injection"
59+
}
60+
}
61+
62+
private class DefaultFragmentInjectionSink extends FragmentInjectionSink {
63+
DefaultFragmentInjectionSink() { sinkNode(this, "fragment-injection") }
64+
}
65+
66+
private class DefaultFragmentInjectionAdditionalTaintStep extends FragmentInjectionAdditionalTaintStep {
67+
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
68+
exists(ReflectiveClassIdentifierMethodAccess ma |
69+
ma.getArgument(0) = n1.asExpr() and ma = n2.asExpr()
70+
)
71+
or
72+
exists(NewInstance ni |
73+
ni.getQualifier() = n1.asExpr() and
74+
ni = n2.asExpr()
75+
)
76+
or
77+
exists(MethodAccess ma |
78+
ma.getMethod() instanceof FragmentInstantiateMethod and
79+
ma.getArgument(1) = n1.asExpr() and
80+
ma = n2.asExpr()
81+
)
82+
}
83+
}

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

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

‎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+
When fragments are instantiated with externally provided names, this exposes any exported activity that dynamically
7+
creates and hosts the fragment to fragment injection. A malicious application could provide the
8+
name of an arbitrary fragment, even one not designed to be externally accessible, and inject it into the activity.
9+
This can bypass access controls and expose the application to unintended effects.
10+
</p>
11+
<p>
12+
Fragments are reusable parts of an Android application's user interface.
13+
Even though a fragment controls its own lifecycle and layout, and handles its input events,
14+
it cannot exist on its own: it must be hosted either by an activity or another fragment.
15+
This means that, normally, a fragment will be accessible by third-party applications (that is, exported)
16+
only if its hosting activity is itself exported.
17+
</p>
18+
</overview>
19+
20+
<recommendation>
21+
<p>
22+
In general, do not instantiate classes (including fragments) with user-provided names
23+
unless the name has been properly validated.
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 that extend <code>PreferenceActivity</code>. The first activity overrides
40+
<code>isValidFragment</code>, but it wrongly returns <code>true</code> unconditionally. The second activity
41+
correctly overrides <code>isValidFragment</code> so that it only returns <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 without validation
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 allow a malicious application to bypass access controls, exposing the application to unintended effects.
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/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.java

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
class UnsafeActivity extends PreferenceActivity {
2+
3+
@Override
4+
protected boolean isValidFragment(String fragmentName) {
5+
// BAD: any Fragment name can be provided.
6+
return true;
7+
}
8+
}
9+
10+
11+
class SafeActivity extends PreferenceActivity {
12+
@Override
13+
protected boolean isValidFragment(String fragmentName) {
14+
// Good: only trusted Fragment names are allowed.
15+
return SafeFragment1.class.getName().equals(fragmentName)
16+
|| SafeFragment2.class.getName().equals(fragmentName)
17+
|| SafeFragment3.class.getName().equals(fragmentName);
18+
}
19+
20+
}
21+

‎java/ql/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.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/FragmentInjectionInPreferenceActivity.ql

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* @name Android fragment injection in PreferenceActivity
3+
* @description An insecure implementation of the 'isValidFragment' method
4+
* of the 'PreferenceActivity' class may allow a malicious application to bypass access controls,
5+
* exposing the application to unintended effects.
6+
* @kind problem
7+
* @problem.severity error
8+
* @security-severity 9.8
9+
* @precision high
10+
* @id java/android/fragment-injection-preference-activity
11+
* @tags security
12+
* external/cwe/cwe-470
13+
*/
14+
15+
import java
16+
import semmle.code.java.security.FragmentInjection
17+
18+
from IsValidFragmentMethod m
19+
where m.isUnsafe()
20+
select m,
21+
"The 'isValidFragment' method always returns true. This makes the exported Activity $@ vulnerable to Fragment Injection.",
22+
m.getDeclaringType(), m.getDeclaringType().getName()

0 commit comments

Comments
 (0)