diff --git a/java/ql/lib/semmle/code/java/security/FragmentInjection.qll b/java/ql/lib/semmle/code/java/security/FragmentInjection.qll index 68946e8bee6..8e558878ab3 100644 --- a/java/ql/lib/semmle/code/java/security/FragmentInjection.qll +++ b/java/ql/lib/semmle/code/java/security/FragmentInjection.qll @@ -5,6 +5,32 @@ 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, BooleanLiteral bool | + retStmt.getEnclosingCallable() = this and + // Using taint tracking to handle logical expressions, like + // fragmentName.equals("safe") || true + TaintTracking::localExprTaint(bool, retStmt.getResult()) + | + bool.getBooleanValue() = true + ) + } +} + /** * A sink for Fragment injection vulnerabilities, * that is, method calls that dynamically add Fragments to Activities. diff --git a/java/ql/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.java b/java/ql/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.java new file mode 100644 index 00000000000..b74daf844a6 --- /dev/null +++ b/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); + } + +} + diff --git a/java/ql/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.qhelp b/java/ql/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.qhelp new file mode 100644 index 00000000000..86be8fa8e53 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.qhelp @@ -0,0 +1,4 @@ + + + + diff --git a/java/ql/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.ql b/java/ql/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.ql new file mode 100644 index 00000000000..5d76d14cbae --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-470/FragmentInjectionInPreferenceActivity.ql @@ -0,0 +1,21 @@ +/** + * @name Android Fragment injection in PreferenceActivity + * @description An insecure implementation of the isValidFragment method + * of the PreferenceActivity class may lead to Fragment injection. + * @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() diff --git a/java/ql/test/query-tests/security/CWE-470/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-470/AndroidManifest.xml index f1b73015afd..d275eb4ff08 100644 --- a/java/ql/test/query-tests/security/CWE-470/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-470/AndroidManifest.xml @@ -19,5 +19,8 @@ + + + diff --git a/java/ql/test/query-tests/security/CWE-470/FragmentInjectionInPreferenceActivityTest.expected b/java/ql/test/query-tests/security/CWE-470/FragmentInjectionInPreferenceActivityTest.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/java/ql/test/query-tests/security/CWE-470/FragmentInjectionInPreferenceActivityTest.ql b/java/ql/test/query-tests/security/CWE-470/FragmentInjectionInPreferenceActivityTest.ql new file mode 100644 index 00000000000..1e4d7b42cb5 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-470/FragmentInjectionInPreferenceActivityTest.ql @@ -0,0 +1,18 @@ +import java +import semmle.code.java.security.FragmentInjection +import TestUtilities.InlineExpectationsTest + +class FragmentInjectionInPreferenceActivityTest extends InlineExpectationsTest { + FragmentInjectionInPreferenceActivityTest() { this = "FragmentInjectionInPreferenceActivityTest" } + + override string getARelevantTag() { result = "hasPreferenceFragmentInjection" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasPreferenceFragmentInjection" and + exists(IsValidFragmentMethod isValidFragment | isValidFragment.isUnsafe() | + isValidFragment.getLocation() = location and + element = isValidFragment.toString() and + value = "" + ) + } +} diff --git a/java/ql/test/query-tests/security/CWE-470/SafePreferenceActivity.java b/java/ql/test/query-tests/security/CWE-470/SafePreferenceActivity.java new file mode 100644 index 00000000000..87ed6dd6168 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-470/SafePreferenceActivity.java @@ -0,0 +1,11 @@ +package com.example.myapp; + +import android.preference.PreferenceActivity; + +public class SafePreferenceActivity extends PreferenceActivity { + + @Override + protected boolean isValidFragment(String fragmentName) { // Safe: not all paths return true + return fragmentName.equals("MySafeFragment") || fragmentName.equals("MySafeFragment2"); + } +} diff --git a/java/ql/test/query-tests/security/CWE-470/UnexportedPreferenceActivity.java b/java/ql/test/query-tests/security/CWE-470/UnexportedPreferenceActivity.java new file mode 100644 index 00000000000..11f15098598 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-470/UnexportedPreferenceActivity.java @@ -0,0 +1,11 @@ +package com.example.myapp; + +import android.preference.PreferenceActivity; + +public class UnexportedPreferenceActivity extends PreferenceActivity { + + @Override + protected boolean isValidFragment(String fragmentName) { // Safe: not exported + return true; + } +} diff --git a/java/ql/test/query-tests/security/CWE-470/UnsafePreferenceActivity.java b/java/ql/test/query-tests/security/CWE-470/UnsafePreferenceActivity.java new file mode 100644 index 00000000000..330a4f29e77 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-470/UnsafePreferenceActivity.java @@ -0,0 +1,11 @@ +package com.example.myapp; + +import android.preference.PreferenceActivity; + +public class UnsafePreferenceActivity extends PreferenceActivity { + + @Override + protected boolean isValidFragment(String fragmentName) { // $ hasPreferenceFragmentInjection + return fragmentName.equals("MySafeClass") || true; + } +}