mirror of
https://github.com/github/codeql.git
synced 2025-12-21 19:26:31 +01:00
Merge branch 'main' into atorralba/android-implicit-pending-intents
This commit is contained in:
Tony Torralba
@@ -0,0 +1,36 @@
|
||||
public void fileSystemStorageUnsafe(String name, String password) {
|
||||
// BAD - sensitive data stored in cleartext
|
||||
FileWriter fw = new FileWriter("some_file.txt");
|
||||
fw.write(name + ":" + password);
|
||||
fw.close();
|
||||
}
|
||||
|
||||
public void filesystemStorageEncryptedFileSafe(Context context, String name, String password) {
|
||||
// GOOD - the whole file is encrypted with androidx.security.crypto.EncryptedFile
|
||||
File file = new File("some_file.txt");
|
||||
String masterKeyAlias = MasterKeys.getOrCreate(MasterKeys.AES256_GCM_SPEC);
|
||||
EncryptedFile encryptedFile = new EncryptedFile.Builder(
|
||||
file,
|
||||
context,
|
||||
masterKeyAlias,
|
||||
EncryptedFile.FileEncryptionScheme.AES256_GCM_HKDF_4KB
|
||||
).build();
|
||||
FileOutputStream encryptedOutputStream = encryptedFile.openFileOutput();
|
||||
encryptedOutputStream.write(name + ":" + password);
|
||||
}
|
||||
|
||||
public void fileSystemStorageSafe(String name, String password) {
|
||||
// GOOD - sensitive data is encrypted using a custom method
|
||||
FileWriter fw = new FileWriter("some_file.txt");
|
||||
fw.write(name + ":" + encrypt(password));
|
||||
fw.close();
|
||||
}
|
||||
|
||||
private static String encrypt(String cleartext) {
|
||||
// Use an encryption or strong hashing algorithm in the real world.
|
||||
// The example below just returns a SHA-256 hash.
|
||||
MessageDigest digest = MessageDigest.getInstance("SHA-256");
|
||||
byte[] hash = digest.digest(cleartext.getBytes(StandardCharsets.UTF_8));
|
||||
String encoded = Base64.getEncoder().encodeToString(hash);
|
||||
return encoded;
|
||||
}
|
||||
@@ -0,0 +1,35 @@
|
||||
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>
|
||||
Android applications with the appropriate permissions can write files either to the device external storage or the application internal storage, depending on the application's needs. However, sensitive information should not be saved in cleartext. Otherwise it can be accessed by any process or user in rooted devices, or can be disclosed through chained vulnerabilities, like unexpected access to the private storage through exposed components.
|
||||
</p>
|
||||
</overview>
|
||||
|
||||
<recommendation>
|
||||
<p>
|
||||
Consider using the <code>EncryptedFile</code> class to work with files containing sensitive data. Alternatively, use encryption algorithms to encrypt the sensitive data being stored.
|
||||
</p>
|
||||
</recommendation>
|
||||
|
||||
<example>
|
||||
<p>
|
||||
In the first example, sensitive user information is stored in cleartext using a local file.
|
||||
</p>
|
||||
<p>
|
||||
In the second and third examples, the code encrypts sensitive information before saving it to the filesystem.
|
||||
</p>
|
||||
<sample src="CleartextStorageAndroidFilesystem.java" />
|
||||
</example>
|
||||
|
||||
<references>
|
||||
<li>
|
||||
Android Developers:
|
||||
<a href="https://developer.android.com/topic/security/data">Work with data more securely</a>
|
||||
</li>
|
||||
<li>
|
||||
Android Developers:
|
||||
<a href="https://developer.android.com/reference/androidx/security/crypto/EncryptedFile">EncryptedFile</a>
|
||||
</li>
|
||||
</references>
|
||||
</qhelp>
|
||||
@@ -0,0 +1,23 @@
|
||||
/**
|
||||
* @name Cleartext storage of sensitive information in the Android filesystem
|
||||
* @description Cleartext storage of sensitive information in the Android filesystem
|
||||
* allows access for users with root privileges or unexpected exposure
|
||||
* from chained vulnerabilities.
|
||||
* @kind problem
|
||||
* @problem.severity warning
|
||||
* @precision medium
|
||||
* @id java/android/cleartext-storage-filesystem
|
||||
* @tags security
|
||||
* external/cwe/cwe-312
|
||||
*/
|
||||
|
||||
import java
|
||||
import semmle.code.java.security.CleartextStorageAndroidFilesystemQuery
|
||||
|
||||
from SensitiveSource data, LocalFileOpenCall s, Expr input, Expr store
|
||||
where
|
||||
input = s.getAnInput() and
|
||||
store = s.getAStore() and
|
||||
data.flowsTo(input)
|
||||
select store, "Local file $@ containing $@ is stored $@. Data was added $@.", s, s.toString(), data,
|
||||
"sensitive data", store, "here", input, "here"
|
||||
60
java/ql/src/Security/CWE/CWE-470/FragmentInjection.inc.qhelp
Normal file
60
java/ql/src/Security/CWE/CWE-470/FragmentInjection.inc.qhelp
Normal file
@@ -0,0 +1,60 @@
|
||||
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
|
||||
<qhelp>
|
||||
|
||||
<overview>
|
||||
<p>
|
||||
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.
|
||||
This can bypass access controls and expose the application to unintended effects.
|
||||
</p>
|
||||
<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, 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
|
||||
unless the name has been properly validated.
|
||||
|
||||
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.
|
||||
</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.
|
||||
</p>
|
||||
<sample src="FragmentInjection.java" />
|
||||
|
||||
<p>
|
||||
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>
|
||||
|
||||
<references>
|
||||
<li> Google Help:
|
||||
<a href="https://support.google.com/faqs/answer/7188427?hl=en">How to fix Fragment Injection vulnerability</a>.
|
||||
</li>
|
||||
<li>
|
||||
IBM Security Systems:
|
||||
<a href="https://securityintelligence.com/wp-content/uploads/2013/12/android-collapses-into-fragments.pdf">Android collapses into Fragments</a>.
|
||||
</li>
|
||||
<li>
|
||||
Android Developers:
|
||||
<a href="https://developer.android.com/guide/fragments">Fragments</a>
|
||||
</li>
|
||||
</references>
|
||||
</qhelp>
|
||||
22
java/ql/src/Security/CWE/CWE-470/FragmentInjection.java
Normal file
22
java/ql/src/Security/CWE/CWE-470/FragmentInjection.java
Normal file
@@ -0,0 +1,22 @@
|
||||
public class MyActivity extends FragmentActivity {
|
||||
|
||||
@Override
|
||||
protected void onCreate(Bundle savedInstance) {
|
||||
try {
|
||||
super.onCreate(savedInstance);
|
||||
// BAD: Fragment instantiated from user input without validation
|
||||
{
|
||||
String fName = getIntent().getStringExtra("fragmentName");
|
||||
getFragmentManager().beginTransaction().replace(com.android.internal.R.id.prefs,
|
||||
Fragment.instantiate(this, fName, null)).commit();
|
||||
}
|
||||
// GOOD: Fragment instantiated statically
|
||||
{
|
||||
getFragmentManager().beginTransaction()
|
||||
.replace(com.android.internal.R.id.prefs, new MyFragment()).commit();
|
||||
}
|
||||
} catch (Exception e) {
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
4
java/ql/src/Security/CWE/CWE-470/FragmentInjection.qhelp
Normal file
4
java/ql/src/Security/CWE/CWE-470/FragmentInjection.qhelp
Normal file
@@ -0,0 +1,4 @@
|
||||
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
|
||||
<qhelp>
|
||||
<include src="FragmentInjection.inc.qhelp"></include>
|
||||
</qhelp>
|
||||
21
java/ql/src/Security/CWE/CWE-470/FragmentInjection.ql
Normal file
21
java/ql/src/Security/CWE/CWE-470/FragmentInjection.ql
Normal file
@@ -0,0 +1,21 @@
|
||||
/**
|
||||
* @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
|
||||
* @precision high
|
||||
* @id java/android/fragment-injection
|
||||
* @tags security
|
||||
* external/cwe/cwe-470
|
||||
*/
|
||||
|
||||
import java
|
||||
import semmle.code.java.security.FragmentInjectionQuery
|
||||
import DataFlow::PathGraph
|
||||
|
||||
from DataFlow::PathNode source, DataFlow::PathNode sink
|
||||
where any(FragmentInjectionTaintConf conf).hasFlowPath(source, sink)
|
||||
select sink.getNode(), source, sink, "Fragment injection from $@.", source.getNode(),
|
||||
"this user input"
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
@@ -0,0 +1,4 @@
|
||||
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
|
||||
<qhelp>
|
||||
<include src="FragmentInjection.inc.qhelp"></include>
|
||||
</qhelp>
|
||||
@@ -0,0 +1,22 @@
|
||||
/**
|
||||
* @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
|
||||
* @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()
|
||||
Reference in New Issue
Block a user