Skip to content

Java: Create new query Cleartext storage of sensitive information in Android filesystem #6576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jan 17, 2022
Merged

Java: Create new query Cleartext storage of sensitive information in Android filesystem #6576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c1ac09a
Added query for Cleartext Storage in Android Filesystem
atorralba Sep 3, 2021
79ddbd6
Fix QLDoc and the qhelp example
atorralba Sep 3, 2021
1e4840e
Fix predicate name
atorralba Nov 11, 2021
9bbba3c
Adjust UnsupportedExternalAPIs test
atorralba Nov 11, 2021
22aad17
Apply review suggestions
atorralba Nov 12, 2021
d9e6e5a
Apply suggestions from code review
atorralba Jan 17, 2022
500deac
Change query description
atorralba Jan 17, 2022
ba3a4fb
Rename filesystemStore predicate after d9e6e5aa04e550e7226c80ae468a2f…
atorralba Jan 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 1 addition & 14 deletions java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ private module Frameworks {
private import semmle.code.java.security.AndroidIntentRedirection
private import semmle.code.java.security.ResponseSplitting
private import semmle.code.java.security.InformationLeak
private import semmle.code.java.security.Files
private import semmle.code.java.security.GroovyInjection
private import semmle.code.java.security.JexlInjectionSinkModels
private import semmle.code.java.security.JndiInjection
Expand Down Expand Up @@ -258,20 +259,6 @@ private predicate sinkModelCsv(string row) {
"java.net;URLClassLoader;false;URLClassLoader;(String,URL[],ClassLoader);;Argument[1];open-url",
"java.net;URLClassLoader;false;URLClassLoader;(String,URL[],ClassLoader,URLStreamHandlerFactory);;Argument[1];open-url",
"java.net;URLClassLoader;false;newInstance;;;Argument[0];open-url",
// Create file
"java.io;FileOutputStream;false;FileOutputStream;;;Argument[0];create-file",
"java.io;RandomAccessFile;false;RandomAccessFile;;;Argument[0];create-file",
"java.io;FileWriter;false;FileWriter;;;Argument[0];create-file",
"java.nio.file;Files;false;move;;;Argument[1];create-file",
"java.nio.file;Files;false;copy;;;Argument[1];create-file",
"java.nio.file;Files;false;newOutputStream;;;Argument[0];create-file",
"java.nio.file;Files;false;newBufferedReader;;;Argument[0];create-file",
"java.nio.file;Files;false;createDirectory;;;Argument[0];create-file",
"java.nio.file;Files;false;createFile;;;Argument[0];create-file",
"java.nio.file;Files;false;createLink;;;Argument[0];create-file",
"java.nio.file;Files;false;createSymbolicLink;;;Argument[0];create-file",
"java.nio.file;Files;false;createTempDirectory;;;Argument[0];create-file",
"java.nio.file;Files;false;createTempFile;;;Argument[0];create-file",
// Bean validation
"javax.validation;ConstraintValidatorContext;true;buildConstraintViolationWithTemplate;;;Argument[0];bean-validation",
// Set hostname
Expand Down
101 changes: 101 additions & 0 deletions java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/**
* Provides classes and predicates to reason about cleartext storage in the Android filesystem
* (external or internal storage).
*/

import java
import semmle.code.java.dataflow.DataFlow
import semmle.code.java.dataflow.ExternalFlow
import semmle.code.java.security.CleartextStorageQuery
import semmle.code.java.security.Files
import semmle.code.xml.AndroidManifest

private class AndroidFilesystemCleartextStorageSink extends CleartextStorageSink {
AndroidFilesystemCleartextStorageSink() {
filesystemInput(_, this.asExpr()) and
// Make sure we are in an Android application.
exists(AndroidManifestXmlFile manifest)
}
}

/** A call to a method or constructor that may write to files to the local filesystem. */
class LocalFileOpenCall extends Storable {
LocalFileOpenCall() {
this = any(DataFlow::Node sink | sinkNode(sink, "create-file")).asExpr().(Argument).getCall()
}

override Expr getAnInput() {
exists(FilesystemFlowConfig conf, DataFlow::Node n |
filesystemInput(n, result) and
conf.hasFlow(DataFlow::exprNode(this), n)
)
}

override Expr getAStore() {
exists(FilesystemFlowConfig conf, DataFlow::Node n |
closesFile(n, result) and
conf.hasFlow(DataFlow::exprNode(this), n)
)
}
}

/** Holds if `input` is written into `file`. */
private predicate filesystemInput(DataFlow::Node file, Argument input) {
exists(DataFlow::Node write | sinkNode(write, "write-file") |
input = write.asExpr() or
isVarargs(input, write)
) and
if input.getCall().getCallee().isStatic()
then file.asExpr() = input.getCall()
else file.asExpr() = input.getCall().getQualifier()
}

/** Holds if `arg` is part of `varargs`. */
private predicate isVarargs(Argument arg, DataFlow::ImplicitVarargsArray varargs) {
arg.isVararg() and arg.getCall() = varargs.getCall()
}

/** Holds if `store` closes `file`. */
private predicate closesFile(DataFlow::Node file, Call closeCall) {
closeCall.getCallee() instanceof CloseFileMethod and
if closeCall.getCallee().isStatic()
then file.asExpr() = closeCall
else file.asExpr() = closeCall.getQualifier()
or
// try-with-resources automatically closes the file
any(TryStmt try).getAResource() = closeCall.(LocalFileOpenCall).getEnclosingStmt() and
closeCall = file.asExpr()
}

/** A method that closes a file, perhaps after writing some data. */
private class CloseFileMethod extends Method {
CloseFileMethod() {
this.hasQualifiedName("java.io", ["RandomAccessFile", "FileOutputStream", "PrintStream"],
"close")
or
this.getDeclaringType().getASupertype*().hasQualifiedName("java.io", "Writer") and
this.hasName("close")
or
this.hasQualifiedName("java.nio.file", "Files", ["write", "writeString"])
}
}

private class FilesystemFlowConfig extends DataFlow::Configuration {
FilesystemFlowConfig() { this = "FilesystemFlowConfig" }

override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof LocalFileOpenCall }

override predicate isSink(DataFlow::Node sink) {
filesystemInput(sink, _) or
closesFile(sink, _)
}

override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
// Add nested Writer constructors as extra data flow steps
exists(ClassInstanceExpr cie |
cie.getConstructedType().getASupertype*().hasQualifiedName("java.io", "Writer") and
node1.asExpr() = cie.getArgument(0) and
node2.asExpr() = cie
)
}
}
72 changes: 72 additions & 0 deletions java/ql/lib/semmle/code/java/security/Files.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/** Provides classes and predicates to work with File objects. */

import java
import semmle.code.java.dataflow.ExternalFlow

private class CreateFileSinkModels extends SinkModelCsv {
override predicate row(string row) {
row =
[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also cover the java.io.File methods (unless they are covered somewhere else)?

  • createTempFile (directory argument)
  • createNewFile(), mkdir(), mkdirs(): Receiver is the sink
  • renameTo(File): Argument is the sink

"java.io;FileOutputStream;false;FileOutputStream;;;Argument[0];create-file",
"java.io;RandomAccessFile;false;RandomAccessFile;;;Argument[0];create-file",
"java.io;FileWriter;false;FileWriter;;;Argument[0];create-file",
"java.io;PrintStream;false;PrintStream;(File);;Argument[0];create-file",
"java.io;PrintStream;false;PrintStream;(File,String);;Argument[0];create-file",
"java.io;PrintStream;false;PrintStream;(File,Charset);;Argument[0];create-file",
"java.io;PrintStream;false;PrintStream;(String);;Argument[0];create-file",
"java.io;PrintStream;false;PrintStream;(String,String);;Argument[0];create-file",
"java.io;PrintStream;false;PrintStream;(String,Charset);;Argument[0];create-file",
"java.io;PrintWriter;false;PrintWriter;(File);;Argument[0];create-file",
"java.io;PrintWriter;false;PrintWriter;(File,String);;Argument[0];create-file",
"java.io;PrintWriter;false;PrintWriter;(File,Charset);;Argument[0];create-file",
"java.io;PrintWriter;false;PrintWriter;(String);;Argument[0];create-file",
"java.io;PrintWriter;false;PrintWriter;(String,String);;Argument[0];create-file",
"java.io;PrintWriter;false;PrintWriter;(String,Charset);;Argument[0];create-file",
"java.nio.file;Files;false;copy;;;Argument[1];create-file",
"java.nio.file;Files;false;createDirectories;;;Argument[0];create-file",
"java.nio.file;Files;false;createDirectory;;;Argument[0];create-file",
"java.nio.file;Files;false;createFile;;;Argument[0];create-file",
"java.nio.file;Files;false;createLink;;;Argument[0];create-file",
"java.nio.file;Files;false;createSymbolicLink;;;Argument[0];create-file",
"java.nio.file;Files;false;createTempDirectory;;;Argument[0];create-file",
"java.nio.file;Files;false;createTempFile;(Path,String,String,FileAttribute[]);;Argument[0];create-file",
"java.nio.file;Files;false;move;;;Argument[1];create-file",
"java.nio.file;Files;false;newBufferedWriter;;;Argument[0];create-file",
"java.nio.file;Files;false;newOutputStream;;;Argument[0];create-file",
"java.nio.file;Files;false;write;;;Argument[0];create-file",
"java.nio.file;Files;false;writeString;;;Argument[0];create-file"
]
}
}

private class WriteFileSinkModels extends SinkModelCsv {
override predicate row(string row) {
row =
[
"java.io;FileOutputStream;false;write;;;Argument[0];write-file",
"java.io;RandomAccessFile;false;write;;;Argument[0];write-file",
"java.io;RandomAccessFile;false;writeBytes;;;Argument[0];write-file",
"java.io;RandomAccessFile;false;writeChars;;;Argument[0];write-file",
"java.io;RandomAccessFile;false;writeUTF;;;Argument[0];write-file",
"java.io;Writer;true;append;;;Argument[0];write-file",
"java.io;Writer;true;write;;;Argument[0];write-file",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems inconsistent that Writer.write is modelled as a file-write but FileOutputStream is special-cased rather than inheriting from OutputStream.write?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning for this was that, if constructed properly, several kinds of Writer can be used to write to a file, e.g.

FileOutputStream os = new FileOutputStream("some_file.txt");
Writer writer = new BufferedWriter(new OutputStreamWriter(os, "utf-8"));
Writer writer2 = new OutputStreamWriter(os, "utf-8");

Whereas, AFAIK, the same cannot be said about OutputStream, i.e. it needs to be a FileOutputStream if you want to write to a file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sure, I thought you were handling cases where you're not sure where the OutputStream came from. Worth checking if we track taint well enough that the typical BufferedOuputStream(FileOutputStream(...)) works?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm you're right, the case for OutputStreams is actually very similar to Writer since you can always wrap it in another stream.

I don't think it currently works for that case, because BufferedOutputStream.write isn't defined as a sink, and AFAIK we don't support this kind of wrapping when calculating virtual dispatch.

So, for consistency, if you agree I could change the FileOutputStream sink to OutputStream, and see if it generates too many FPs. Because I'm afraid that the opposite (modeling only FileWriter) would cause too many FNs.

"java.io;PrintStream;true;append;;;Argument[0];write-file",
"java.io;PrintStream;true;format;(String,Object[]);;Argument[0..1];write-file",
"java.io;PrintStream;true;format;(Locale,String,Object[]);;Argument[1..2];write-file",
"java.io;PrintStream;true;print;;;Argument[0];write-file",
"java.io;PrintStream;true;printf;(String,Object[]);;Argument[0..1];write-file",
"java.io;PrintStream;true;printf;(Locale,String,Object[]);;Argument[1..2];write-file",
"java.io;PrintStream;true;println;;;Argument[0];write-file",
"java.io;PrintStream;true;write;;;Argument[0];write-file",
"java.io;PrintStream;true;writeBytes;;;Argument[0];write-file",
"java.io;PrintWriter;false;format;(String,Object[]);;Argument[0..1];write-file",
"java.io;PrintWriter;false;format;(Locale,String,Object[]);;Argument[1..2];write-file",
"java.io;PrintWriter;false;print;;;Argument[0];write-file",
"java.io;PrintWriter;false;printf;(String,Object[]);;Argument[0..1];write-file",
"java.io;PrintWriter;false;printf;(Locale,String,Object[]);;Argument[1..2];write-file",
"java.io;PrintWriter;false;println;;;Argument[0];write-file",
"java.nio.file;Files;false;write;;;Argument[1];write-file",
"java.nio.file;Files;false;writeString;;;Argument[1];write-file"
]
}
}
36 changes: 36 additions & 0 deletions java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}
35 changes: 35 additions & 0 deletions java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.qhelp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>
23 changes: 23 additions & 0 deletions java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
4 changes: 4 additions & 0 deletions java/ql/src/change-notes/2021-09-01-cleartext-storage-filesystem-query.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: newQuery
---
* 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.
1 change: 0 additions & 1 deletion java/ql/test/query-tests/Telemetry/UnsupportedExternalAPIs/UnsupportedExternalAPIs.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
| java.io.PrintStream#println(Object) | 3 |
| java.lang.Class#isAssignableFrom(Class) | 1 |
| java.lang.String#length() | 1 |
| java.time.Duration#ofMillis(long) | 1 |
Expand Down
13 changes: 13 additions & 0 deletions java/ql/test/query-tests/security/CWE-312/AndroidManifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="com.example.app"
android:installLocation="auto"
android:versionCode="1"
android:versionName="0.1" >

<application>
<activity android:name=".CleartextStorageAndroidDatabaseTest"></activity>
<activity android:name=".CleartextStorageAndroidFileSystemTest"></activity>
<activity android:name=".CleartextStorageSharedPrefsTest"></activity>
</application>

</manifest>
Empty file added java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.expected
View file
Open in desktop
Empty file.
Loading