Skip to content

Commit e967b8a

Browse files
atorralbaatorralba
authored
Merge pull request #6576 from atorralba/atorralba/android-cleartext-storage-filesystem
Java: Create new query Cleartext storage of sensitive information in Android filesystem
2 parents 2279295 + ba3a4fb commit e967b8a

File tree

13 files changed

+644
-15
lines changed

13 files changed

+644
-15
lines changed

‎java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ private module Frameworks {
115115
private import semmle.code.java.security.AndroidIntentRedirection
116116
private import semmle.code.java.security.ResponseSplitting
117117
private import semmle.code.java.security.InformationLeak
118+
private import semmle.code.java.security.Files
118119
private import semmle.code.java.security.GroovyInjection
119120
private import semmle.code.java.security.JexlInjectionSinkModels
120121
private import semmle.code.java.security.JndiInjection
@@ -258,20 +259,6 @@ private predicate sinkModelCsv(string row) {
258259
"java.net;URLClassLoader;false;URLClassLoader;(String,URL[],ClassLoader);;Argument[1];open-url",
259260
"java.net;URLClassLoader;false;URLClassLoader;(String,URL[],ClassLoader,URLStreamHandlerFactory);;Argument[1];open-url",
260261
"java.net;URLClassLoader;false;newInstance;;;Argument[0];open-url",
261-
// Create file
262-
"java.io;FileOutputStream;false;FileOutputStream;;;Argument[0];create-file",
263-
"java.io;RandomAccessFile;false;RandomAccessFile;;;Argument[0];create-file",
264-
"java.io;FileWriter;false;FileWriter;;;Argument[0];create-file",
265-
"java.nio.file;Files;false;move;;;Argument[1];create-file",
266-
"java.nio.file;Files;false;copy;;;Argument[1];create-file",
267-
"java.nio.file;Files;false;newOutputStream;;;Argument[0];create-file",
268-
"java.nio.file;Files;false;newBufferedReader;;;Argument[0];create-file",
269-
"java.nio.file;Files;false;createDirectory;;;Argument[0];create-file",
270-
"java.nio.file;Files;false;createFile;;;Argument[0];create-file",
271-
"java.nio.file;Files;false;createLink;;;Argument[0];create-file",
272-
"java.nio.file;Files;false;createSymbolicLink;;;Argument[0];create-file",
273-
"java.nio.file;Files;false;createTempDirectory;;;Argument[0];create-file",
274-
"java.nio.file;Files;false;createTempFile;;;Argument[0];create-file",
275262
// Bean validation
276263
"javax.validation;ConstraintValidatorContext;true;buildConstraintViolationWithTemplate;;;Argument[0];bean-validation",
277264
// Set hostname

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

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/**
2+
* Provides classes and predicates to reason about cleartext storage in the Android filesystem
3+
* (external or internal storage).
4+
*/
5+
6+
import java
7+
import semmle.code.java.dataflow.DataFlow
8+
import semmle.code.java.dataflow.ExternalFlow
9+
import semmle.code.java.security.CleartextStorageQuery
10+
import semmle.code.java.security.Files
11+
import semmle.code.xml.AndroidManifest
12+
13+
private class AndroidFilesystemCleartextStorageSink extends CleartextStorageSink {
14+
AndroidFilesystemCleartextStorageSink() {
15+
filesystemInput(_, this.asExpr()) and
16+
// Make sure we are in an Android application.
17+
exists(AndroidManifestXmlFile manifest)
18+
}
19+
}
20+
21+
/** A call to a method or constructor that may write to files to the local filesystem. */
22+
class LocalFileOpenCall extends Storable {
23+
LocalFileOpenCall() {
24+
this = any(DataFlow::Node sink | sinkNode(sink, "create-file")).asExpr().(Argument).getCall()
25+
}
26+
27+
override Expr getAnInput() {
28+
exists(FilesystemFlowConfig conf, DataFlow::Node n |
29+
filesystemInput(n, result) and
30+
conf.hasFlow(DataFlow::exprNode(this), n)
31+
)
32+
}
33+
34+
override Expr getAStore() {
35+
exists(FilesystemFlowConfig conf, DataFlow::Node n |
36+
closesFile(n, result) and
37+
conf.hasFlow(DataFlow::exprNode(this), n)
38+
)
39+
}
40+
}
41+
42+
/** Holds if `input` is written into `file`. */
43+
private predicate filesystemInput(DataFlow::Node file, Argument input) {
44+
exists(DataFlow::Node write | sinkNode(write, "write-file") |
45+
input = write.asExpr() or
46+
isVarargs(input, write)
47+
) and
48+
if input.getCall().getCallee().isStatic()
49+
then file.asExpr() = input.getCall()
50+
else file.asExpr() = input.getCall().getQualifier()
51+
}
52+
53+
/** Holds if `arg` is part of `varargs`. */
54+
private predicate isVarargs(Argument arg, DataFlow::ImplicitVarargsArray varargs) {
55+
arg.isVararg() and arg.getCall() = varargs.getCall()
56+
}
57+
58+
/** Holds if `store` closes `file`. */
59+
private predicate closesFile(DataFlow::Node file, Call closeCall) {
60+
closeCall.getCallee() instanceof CloseFileMethod and
61+
if closeCall.getCallee().isStatic()
62+
then file.asExpr() = closeCall
63+
else file.asExpr() = closeCall.getQualifier()
64+
or
65+
// try-with-resources automatically closes the file
66+
any(TryStmt try).getAResource() = closeCall.(LocalFileOpenCall).getEnclosingStmt() and
67+
closeCall = file.asExpr()
68+
}
69+
70+
/** A method that closes a file, perhaps after writing some data. */
71+
private class CloseFileMethod extends Method {
72+
CloseFileMethod() {
73+
this.hasQualifiedName("java.io", ["RandomAccessFile", "FileOutputStream", "PrintStream"],
74+
"close")
75+
or
76+
this.getDeclaringType().getASupertype*().hasQualifiedName("java.io", "Writer") and
77+
this.hasName("close")
78+
or
79+
this.hasQualifiedName("java.nio.file", "Files", ["write", "writeString"])
80+
}
81+
}
82+
83+
private class FilesystemFlowConfig extends DataFlow::Configuration {
84+
FilesystemFlowConfig() { this = "FilesystemFlowConfig" }
85+
86+
override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof LocalFileOpenCall }
87+
88+
override predicate isSink(DataFlow::Node sink) {
89+
filesystemInput(sink, _) or
90+
closesFile(sink, _)
91+
}
92+
93+
override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
94+
// Add nested Writer constructors as extra data flow steps
95+
exists(ClassInstanceExpr cie |
96+
cie.getConstructedType().getASupertype*().hasQualifiedName("java.io", "Writer") and
97+
node1.asExpr() = cie.getArgument(0) and
98+
node2.asExpr() = cie
99+
)
100+
}
101+
}

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

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
/** Provides classes and predicates to work with File objects. */
2+
3+
import java
4+
import semmle.code.java.dataflow.ExternalFlow
5+
6+
private class CreateFileSinkModels extends SinkModelCsv {
7+
override predicate row(string row) {
8+
row =
9+
[
10+
"java.io;FileOutputStream;false;FileOutputStream;;;Argument[0];create-file",
11+
"java.io;RandomAccessFile;false;RandomAccessFile;;;Argument[0];create-file",
12+
"java.io;FileWriter;false;FileWriter;;;Argument[0];create-file",
13+
"java.io;PrintStream;false;PrintStream;(File);;Argument[0];create-file",
14+
"java.io;PrintStream;false;PrintStream;(File,String);;Argument[0];create-file",
15+
"java.io;PrintStream;false;PrintStream;(File,Charset);;Argument[0];create-file",
16+
"java.io;PrintStream;false;PrintStream;(String);;Argument[0];create-file",
17+
"java.io;PrintStream;false;PrintStream;(String,String);;Argument[0];create-file",
18+
"java.io;PrintStream;false;PrintStream;(String,Charset);;Argument[0];create-file",
19+
"java.io;PrintWriter;false;PrintWriter;(File);;Argument[0];create-file",
20+
"java.io;PrintWriter;false;PrintWriter;(File,String);;Argument[0];create-file",
21+
"java.io;PrintWriter;false;PrintWriter;(File,Charset);;Argument[0];create-file",
22+
"java.io;PrintWriter;false;PrintWriter;(String);;Argument[0];create-file",
23+
"java.io;PrintWriter;false;PrintWriter;(String,String);;Argument[0];create-file",
24+
"java.io;PrintWriter;false;PrintWriter;(String,Charset);;Argument[0];create-file",
25+
"java.nio.file;Files;false;copy;;;Argument[1];create-file",
26+
"java.nio.file;Files;false;createDirectories;;;Argument[0];create-file",
27+
"java.nio.file;Files;false;createDirectory;;;Argument[0];create-file",
28+
"java.nio.file;Files;false;createFile;;;Argument[0];create-file",
29+
"java.nio.file;Files;false;createLink;;;Argument[0];create-file",
30+
"java.nio.file;Files;false;createSymbolicLink;;;Argument[0];create-file",
31+
"java.nio.file;Files;false;createTempDirectory;;;Argument[0];create-file",
32+
"java.nio.file;Files;false;createTempFile;(Path,String,String,FileAttribute[]);;Argument[0];create-file",
33+
"java.nio.file;Files;false;move;;;Argument[1];create-file",
34+
"java.nio.file;Files;false;newBufferedWriter;;;Argument[0];create-file",
35+
"java.nio.file;Files;false;newOutputStream;;;Argument[0];create-file",
36+
"java.nio.file;Files;false;write;;;Argument[0];create-file",
37+
"java.nio.file;Files;false;writeString;;;Argument[0];create-file"
38+
]
39+
}
40+
}
41+
42+
private class WriteFileSinkModels extends SinkModelCsv {
43+
override predicate row(string row) {
44+
row =
45+
[
46+
"java.io;FileOutputStream;false;write;;;Argument[0];write-file",
47+
"java.io;RandomAccessFile;false;write;;;Argument[0];write-file",
48+
"java.io;RandomAccessFile;false;writeBytes;;;Argument[0];write-file",
49+
"java.io;RandomAccessFile;false;writeChars;;;Argument[0];write-file",
50+
"java.io;RandomAccessFile;false;writeUTF;;;Argument[0];write-file",
51+
"java.io;Writer;true;append;;;Argument[0];write-file",
52+
"java.io;Writer;true;write;;;Argument[0];write-file",
53+
"java.io;PrintStream;true;append;;;Argument[0];write-file",
54+
"java.io;PrintStream;true;format;(String,Object[]);;Argument[0..1];write-file",
55+
"java.io;PrintStream;true;format;(Locale,String,Object[]);;Argument[1..2];write-file",
56+
"java.io;PrintStream;true;print;;;Argument[0];write-file",
57+
"java.io;PrintStream;true;printf;(String,Object[]);;Argument[0..1];write-file",
58+
"java.io;PrintStream;true;printf;(Locale,String,Object[]);;Argument[1..2];write-file",
59+
"java.io;PrintStream;true;println;;;Argument[0];write-file",
60+
"java.io;PrintStream;true;write;;;Argument[0];write-file",
61+
"java.io;PrintStream;true;writeBytes;;;Argument[0];write-file",
62+
"java.io;PrintWriter;false;format;(String,Object[]);;Argument[0..1];write-file",
63+
"java.io;PrintWriter;false;format;(Locale,String,Object[]);;Argument[1..2];write-file",
64+
"java.io;PrintWriter;false;print;;;Argument[0];write-file",
65+
"java.io;PrintWriter;false;printf;(String,Object[]);;Argument[0..1];write-file",
66+
"java.io;PrintWriter;false;printf;(Locale,String,Object[]);;Argument[1..2];write-file",
67+
"java.io;PrintWriter;false;println;;;Argument[0];write-file",
68+
"java.nio.file;Files;false;write;;;Argument[1];write-file",
69+
"java.nio.file;Files;false;writeString;;;Argument[1];write-file"
70+
]
71+
}
72+
}

‎java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.java

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
public void fileSystemStorageUnsafe(String name, String password) {
2+
// BAD - sensitive data stored in cleartext
3+
FileWriter fw = new FileWriter("some_file.txt");
4+
fw.write(name + ":" + password);
5+
fw.close();
6+
}
7+
8+
public void filesystemStorageEncryptedFileSafe(Context context, String name, String password) {
9+
// GOOD - the whole file is encrypted with androidx.security.crypto.EncryptedFile
10+
File file = new File("some_file.txt");
11+
String masterKeyAlias = MasterKeys.getOrCreate(MasterKeys.AES256_GCM_SPEC);
12+
EncryptedFile encryptedFile = new EncryptedFile.Builder(
13+
file,
14+
context,
15+
masterKeyAlias,
16+
EncryptedFile.FileEncryptionScheme.AES256_GCM_HKDF_4KB
17+
).build();
18+
FileOutputStream encryptedOutputStream = encryptedFile.openFileOutput();
19+
encryptedOutputStream.write(name + ":" + password);
20+
}
21+
22+
public void fileSystemStorageSafe(String name, String password) {
23+
// GOOD - sensitive data is encrypted using a custom method
24+
FileWriter fw = new FileWriter("some_file.txt");
25+
fw.write(name + ":" + encrypt(password));
26+
fw.close();
27+
}
28+
29+
private static String encrypt(String cleartext) {
30+
// Use an encryption or strong hashing algorithm in the real world.
31+
// The example below just returns a SHA-256 hash.
32+
MessageDigest digest = MessageDigest.getInstance("SHA-256");
33+
byte[] hash = digest.digest(cleartext.getBytes(StandardCharsets.UTF_8));
34+
String encoded = Base64.getEncoder().encodeToString(hash);
35+
return encoded;
36+
}

‎java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.qhelp

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
<overview>
4+
<p>
5+
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.
6+
</p>
7+
</overview>
8+
9+
<recommendation>
10+
<p>
11+
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.
12+
</p>
13+
</recommendation>
14+
15+
<example>
16+
<p>
17+
In the first example, sensitive user information is stored in cleartext using a local file.
18+
</p>
19+
<p>
20+
In the second and third examples, the code encrypts sensitive information before saving it to the filesystem.
21+
</p>
22+
<sample src="CleartextStorageAndroidFilesystem.java" />
23+
</example>
24+
25+
<references>
26+
<li>
27+
Android Developers:
28+
<a href="https://developer.android.com/topic/security/data">Work with data more securely</a>
29+
</li>
30+
<li>
31+
Android Developers:
32+
<a href="https://developer.android.com/reference/androidx/security/crypto/EncryptedFile">EncryptedFile</a>
33+
</li>
34+
</references>
35+
</qhelp>

‎java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Cleartext storage of sensitive information in the Android filesystem
3+
* @description Cleartext storage of sensitive information in the Android filesystem
4+
* allows access for users with root privileges or unexpected exposure
5+
* from chained vulnerabilities.
6+
* @kind problem
7+
* @problem.severity warning
8+
* @precision medium
9+
* @id java/android/cleartext-storage-filesystem
10+
* @tags security
11+
* external/cwe/cwe-312
12+
*/
13+
14+
import java
15+
import semmle.code.java.security.CleartextStorageAndroidFilesystemQuery
16+
17+
from SensitiveSource data, LocalFileOpenCall s, Expr input, Expr store
18+
where
19+
input = s.getAnInput() and
20+
store = s.getAStore() and
21+
data.flowsTo(input)
22+
select store, "Local file $@ containing $@ is stored $@. Data was added $@.", s, s.toString(), data,
23+
"sensitive data", store, "here", input, "here"

‎java/ql/src/change-notes/2021-09-01-cleartext-storage-filesystem-query.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* A new query "Cleartext storage of sensitive information in the Android filesystem" (`java/android/cleartext-storage-filesystem`) has been added. This query finds instances of sensitive data being stored in local files without encryption, which may expose it to attackers or malicious applications.

‎java/ql/test/query-tests/Telemetry/UnsupportedExternalAPIs/UnsupportedExternalAPIs.expected

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
| java.io.PrintStream#println(Object) | 3 |
21
| java.lang.Class#isAssignableFrom(Class) | 1 |
32
| java.lang.String#length() | 1 |
43
| java.time.Duration#ofMillis(long) | 1 |

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
2+
package="com.example.app"
3+
android:installLocation="auto"
4+
android:versionCode="1"
5+
android:versionName="0.1" >
6+
7+
<application>
8+
<activity android:name=".CleartextStorageAndroidDatabaseTest"></activity>
9+
<activity android:name=".CleartextStorageAndroidFileSystemTest"></activity>
10+
<activity android:name=".CleartextStorageSharedPrefsTest"></activity>
11+
</application>
12+
13+
</manifest>

‎java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.expected

Whitespace-only changes.

0 commit comments

Comments
 (0)