From c1ac09a063d46db7d6eb653a49ca274379d666a3 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 3 Sep 2021 13:24:31 +0200 Subject: [PATCH 1/8] Added query for Cleartext Storage in Android Filesystem --- .../code/java/dataflow/ExternalFlow.qll | 15 +- ...CleartextStorageAndroidFilesystemQuery.qll | 101 +++++++ .../lib/semmle/code/java/security/Files.qll | 70 +++++ .../CleartextStorageAndroidFilesystem.java | 36 +++ .../CleartextStorageAndroidFilesystem.qhelp | 35 +++ .../CleartextStorageAndroidFilesystem.ql | 23 ++ ...9-01-cleartext-storage-filesystem-query.md | 4 + .../security/CWE-312/AndroidManifest.xml | 13 + ...rtextStorageAndroidFilesystemTest.expected | 0 ...CleartextStorageAndroidFilesystemTest.java | 277 ++++++++++++++++++ .../CleartextStorageAndroidFilesystemTest.ql | 22 ++ .../security/crypto/EncryptedFile.java | 60 ++++ 12 files changed, 642 insertions(+), 14 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll create mode 100644 java/ql/lib/semmle/code/java/security/Files.qll create mode 100644 java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.java create mode 100644 java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql create mode 100644 java/ql/src/change-notes/2021-09-01-cleartext-storage-filesystem-query.md create mode 100644 java/ql/test/query-tests/security/CWE-312/AndroidManifest.xml create mode 100644 java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.expected create mode 100644 java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.java create mode 100644 java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.ql create mode 100644 java/ql/test/stubs/google-android-9.0.0/androidx/security/crypto/EncryptedFile.java diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index 430c3df816c7..158b8b924273 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -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 @@ -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 diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll new file mode 100644 index 000000000000..abdfd98e4838 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll @@ -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) + } +} + +/** The creation of an object that can be used to 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 | + filesystemStore(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 filesystemStore(DataFlow::Node file, Call store) { + store.getCallee() instanceof CloseFileMethod and + if store.getCallee().isStatic() + then file.asExpr() = store + else file.asExpr() = store.getQualifier() + or + // try-with-resources automatically closes the file + any(TryStmt try).getAResource() = store.(LocalFileOpenCall).getEnclosingStmt() and + store = file.asExpr() +} + +/** A method that closes a file. */ +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 + filesystemStore(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 + ) + } +} diff --git a/java/ql/lib/semmle/code/java/security/Files.qll b/java/ql/lib/semmle/code/java/security/Files.qll new file mode 100644 index 000000000000..35764cc45511 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/Files.qll @@ -0,0 +1,70 @@ +import java +import semmle.code.java.dataflow.ExternalFlow + +private class CreateFileSinkModels extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "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", + "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" + ] + } +} diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.java b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.java new file mode 100644 index 000000000000..5b4911333f66 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.java @@ -0,0 +1,36 @@ +public void fileSystemStorageUnsafe(String name, String password) { + // BAD - sensitive data stored in plaintext + 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; +} diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.qhelp b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.qhelp new file mode 100644 index 000000000000..b14d34539cb7 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.qhelp @@ -0,0 +1,35 @@ + + + +

+ 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. +

+ + + +

+ Consider using the EncryptedFile class to work with files containing sensitive data. Alternatively, use encryption algorithms to encrypt the sensitive data being stored. +

+ + + +

+ In the first example, sensitive user information is stored in cleartext using a local file. +

+

+ In the second and third examples, the code encrypts sensitive information before saving it to the filesystem. +

+ + + + +
  • + Android Developers: + Work with data more securely +
    • +
  • + Android Developers: + EncryptedFile +
    • +     +     diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql new file mode 100644 index 000000000000..63efe303b090 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql @@ -0,0 +1,23 @@ +/** + * @name Cleartext storage of sensitive information in the Android filesystem + * @description Cleartext Storage of Sensitive Information 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.flowsToCached(input) +select store, "Local file $@ containing $@ is stored $@. Data was added $@.", s, s.toString(), data, + "sensitive data", store, "here", input, "here" diff --git a/java/ql/src/change-notes/2021-09-01-cleartext-storage-filesystem-query.md b/java/ql/src/change-notes/2021-09-01-cleartext-storage-filesystem-query.md new file mode 100644 index 000000000000..7c60c03ebf44 --- /dev/null +++ b/java/ql/src/change-notes/2021-09-01-cleartext-storage-filesystem-query.md @@ -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. diff --git a/java/ql/test/query-tests/security/CWE-312/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-312/AndroidManifest.xml new file mode 100644 index 000000000000..d8af1947bd71 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-312/AndroidManifest.xml @@ -0,0 +1,13 @@ + + + + + + + + + diff --git a/java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.expected b/java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.java b/java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.java new file mode 100644 index 000000000000..6c3de16af88f --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.java @@ -0,0 +1,277 @@ +import java.io.BufferedWriter; +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.FileOutputStream; +import java.io.FileWriter; +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.PrintStream; +import java.io.PrintWriter; +import java.io.RandomAccessFile; +import java.io.Writer; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.security.MessageDigest; +import java.util.Base64; +import java.util.List; +import java.util.Locale; +import android.app.Activity; +import android.content.Context; +import androidx.security.crypto.EncryptedFile; +import androidx.security.crypto.EncryptedFile.FileEncryptionScheme; + +public class CleartextStorageAndroidFilesystemTest extends Activity { + + public void testWriteLocalFile(Context context, String name, String password) throws Exception { + + // FileOutputStream + { + // java.io;FileOutputStream;false;FileOutputStream;;;Argument[0];create-file + FileOutputStream os = new FileOutputStream("some_file.txt"); + // Nested writers + Writer writer = new BufferedWriter(new OutputStreamWriter(os, "utf-8")); + // java.io;FileOutputStream;false;write;;;Argument[0];write-file + writer.write(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + writer.close(); + } + + // RandomAccessFile + { + // java.io;RandomAccessFile;false;RandomAccessFile;;;Argument[0];create-file + RandomAccessFile f = new RandomAccessFile("some_file.txt", "r"); + String contents = name + ":" + password; + // java.io;RandomAccessFile;false;write;;;Argument[0];write-file + f.write(contents.getBytes()); // $ hasCleartextStorageAndroidFilesystem + f.close(); + } + { + // try-with-resources + try (RandomAccessFile f = new RandomAccessFile(new File("some_file.txt"), "r")) { + // java.io;RandomAccessFile;false;writeBytes;;;Argument[0];write-file + f.writeBytes(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + } + } + { + RandomAccessFile f = new RandomAccessFile("some_file.txt", "r"); + // java.io;RandomAccessFile;false;writeChars;;;Argument[0];write-file + f.writeChars(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + f.close(); + } + { + RandomAccessFile f = new RandomAccessFile("some_file.txt", "r"); + // java.io;RandomAccessFile;false;writeUTF;;;Argument[0];write-file + f.writeUTF(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + f.close(); + } + + // FileWriter + { + // java.io;FileWriter;false;FileWriter;;;Argument[0];create-file + FileWriter fw = new FileWriter("some_file.txt"); + // java.io;Writer;true;append;;;Argument[0];write-file + fw.append(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + fw.close(); + } + { + // try-with-resources + try (FileWriter fw = new FileWriter("some_file.txt")) { + // java.io;Writer;true;write;;;Argument[0];write-file + fw.write(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + } + } + + // PrintStream + { + // java.io;PrintStream;false;PrintStream;(File);;Argument[0];create-file" + PrintStream ps = new PrintStream(new File("some_file.txt")); + // java.io;PrintStream;true;append;;;Argument[0];write-file + ps.append(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + ps.close(); + } + { + // java.io;PrintStream;false;PrintStream;(File,String);;Argument[0];create-file + PrintStream ps = new PrintStream(new File("some_file.txt"), "utf-8"); + // java.io;PrintStream;true;format;(String,Object[]);;Argument[0..1];write-file + ps.format("%s:" + password, name); // $ hasCleartextStorageAndroidFilesystem + ps.format("%s:%s", name, password); // $ hasCleartextStorageAndroidFilesystem + ps.close(); + } + { + // java.io;PrintStream;false;PrintStream;(File,Charset);;Argument[0];create-file + PrintStream ps = new PrintStream(new File("some_file.txt"), Charset.defaultCharset()); + // java.io;PrintStream;true;format;(Locale,String,Object[]);;Argument[1..2];write-file + ps.format(Locale.US, "%s:" + password, name); // $ hasCleartextStorageAndroidFilesystem + ps.format(Locale.US, "%s:%s", name, password); // $ hasCleartextStorageAndroidFilesystem + ps.close(); + } + { + // java.io;PrintStream;false;PrintStream;(String);;Argument[0];create-file + PrintStream ps = new PrintStream("some_file.txt"); + // java.io;PrintStream;true;print;;;Argument[0];write-file + ps.print(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + ps.close(); + } + { + // java.io;PrintStream;false;PrintStream;(String,String);;Argument[0];create-file + PrintStream ps = new PrintStream("some_file.txt", "utf-8"); + // java.io;PrintStream;true;printf;(String,Object[]);;Argument[0..1];write-file + ps.printf("%s:" + password, name); // $ hasCleartextStorageAndroidFilesystem + ps.printf("%s:%s", name, password); // $ hasCleartextStorageAndroidFilesystem + ps.close(); + } + { + // java.io;PrintStream;false;PrintStream;(String,Charset);;Argument[0];create-file + PrintStream ps = new PrintStream("some_file.txt", Charset.defaultCharset()); + // java.io;PrintStream;true;printf;(Locale,String,Object[]);;Argument[1..2];write-file + ps.printf(Locale.US, "%s:" + password, name); // $ hasCleartextStorageAndroidFilesystem + ps.printf(Locale.US, "%s:%s", name, password); // $ hasCleartextStorageAndroidFilesystem + ps.close(); + } + { + PrintStream ps = new PrintStream("some_file.txt"); + // java.io;PrintStream;true;println;;;Argument[0];write-file + ps.println(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + ps.close(); + } + { + PrintStream ps = new PrintStream("some_file.txt"); + String contents = name + ":" + password; + // java.io;PrintStream;true;write;;;Argument[0];write-file + ps.write(contents.getBytes()); // $ hasCleartextStorageAndroidFilesystem + ps.close(); + } + { + PrintStream ps = new PrintStream("some_file.txt"); + String contents = name + ":" + password; + // java.io;PrintStream;true;writeBytes;;;Argument[0];write-file + ps.writeBytes(contents.getBytes()); // $ hasCleartextStorageAndroidFilesystem + ps.close(); + } + + // PrintWriter + { + // java.io;PrintWriter;false;PrintWriter;(File);;Argument[0];create-file + PrintWriter pw = new PrintWriter(new File("some_file.txt")); + // java.io;Writer;true;append;;;Argument[0];write-file + pw.append(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + pw.close(); + } + { + // try-with-resources + try (PrintWriter pw = new PrintWriter(new File("some_file.txt"))) { + // java.io;PrintWriter;false;format;(String,Object[]);;Argument[0..1];write-file + pw.format("%s:" + password, name); // $ hasCleartextStorageAndroidFilesystem + pw.format("%s:%s", name, password); // $ hasCleartextStorageAndroidFilesystem + } + } + { + // java.io;PrintWriter;false;PrintWriter;(File,String);;Argument[0];create-file + PrintWriter pw = new PrintWriter(new File("some_file.txt"), "utf-8"); + // java.io;PrintWriter;false;format;(Locale,String,Object[]);;Argument[1..2];write-file + pw.format(Locale.US, "%s:" + password, name); // $ hasCleartextStorageAndroidFilesystem + pw.format(Locale.US, "%s:%s", name, password); // $ hasCleartextStorageAndroidFilesystem + pw.close(); + } + { + // java.io;PrintWriter;false;PrintWriter;(File,Charset);;Argument[0];create-file + PrintWriter pw = new PrintWriter(new File("some_file.txt"), Charset.defaultCharset()); + // java.io;PrintWriter;false;print;;;Argument[0];write-file + pw.print(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + pw.close(); + } + + { + // java.io;PrintWriter;false;PrintWriter;(String);;Argument[0];create-file + PrintWriter pw = new PrintWriter("some_file.txt"); + // java.io;PrintWriter;false;printf;(String,Object[]);;Argument[0..1];write-file + pw.printf("%s:" + password, name); // $ hasCleartextStorageAndroidFilesystem + pw.printf("%s:%s", name, password); // $ hasCleartextStorageAndroidFilesystem + pw.close(); + } + { + // java.io;PrintWriter;false;PrintWriter;(String,String);;Argument[0];create-file + PrintWriter pw = new PrintWriter("some_file.txt", "utf-8"); + // java.io;PrintWriter;false;printf;(Locale,String,Object[]);;Argument[1..2];write-file + pw.printf(Locale.US, "%s:" + password, name); // $ hasCleartextStorageAndroidFilesystem + pw.printf(Locale.US, "%s:%s", name, password); // $ hasCleartextStorageAndroidFilesystem + pw.close(); + } + { + // java.io;PrintWriter;false;PrintWriter;(String,Charset);;Argument[0];create-file + PrintWriter pw = new PrintWriter("some_file.txt", Charset.defaultCharset()); + // java.io;PrintWriter;false;println;;;Argument[0];write-file + pw.println(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + pw.close(); + } + { + PrintWriter pw = new PrintWriter("some_file.txt"); + // java.io;Writer;true;write;;;Argument[0];write-file + pw.write(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + pw.close(); + } + + // java.nio.files.Files + { + // java.nio.file;Files;false;newBufferedWriter;;;Argument[0];create-file + BufferedWriter bw = Files.newBufferedWriter(Path.of("some_file.txt")); + bw.write(name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + bw.close(); + } + { + // java.nio.file;Files;false;newOutputStream;;;Argument[0];create-file + // try-with-resources + try (OutputStream os = Files.newOutputStream(Path.of("some_file.txt"))) { + String contents = name + ":" + password; + os.write(contents.getBytes()); + } + } + { + Path path = Path.of("some_file.txt"); + String contents = name + ":" + password; + // java.nio.file;Files;false;write;;;Argument[0];create-file + // java.nio.file;Files;false;write;;;Argument[1];write-file", + Files.write(path, contents.getBytes()); // $ hasCleartextStorageAndroidFilesystem + } + { + Path path = Path.of("some_file.txt"); + String contents = name + ":" + password; + Files.write(path, List.of(contents)); // $ hasCleartextStorageAndroidFilesystem + } + { + Path path = Path.of("some_file.txt"); + // java.nio.file;Files;false;writeString;;;Argument[0];create-file + // java.nio.file;Files;false;writeString;;;Argument[1];write-file" + Files.writeString(path, name + ":" + password); // $ hasCleartextStorageAndroidFilesystem + } + + // Safe writes + { + FileWriter fw = new FileWriter("some_file.txt"); + fw.write(name + ":" + hash(password)); // Safe - using a hash + fw.close(); + } + { + Writer writer = new OutputStreamWriter(new ByteArrayOutputStream(), "utf-8"); + writer.write(name + ":" + password); // Safe - not writing to a file + writer.close(); + } + { + File file = new File("some_file.txt"); + String contents = name + ":" + password; + EncryptedFile encryptedFile = new EncryptedFile.Builder(file, context, "some_key", + FileEncryptionScheme.AES256_GCM_HKDF_4KB).build(); + FileOutputStream encryptedOutputStream = encryptedFile.openFileOutput(); + encryptedOutputStream.write(contents.getBytes()); // Safe - using EncryptedFile + } + } + + private static String hash(String cleartext) throws Exception { + MessageDigest digest = MessageDigest.getInstance("SHA-256"); + byte[] hash = digest.digest(cleartext.getBytes(StandardCharsets.UTF_8)); + String encoded = Base64.getEncoder().encodeToString(hash); + return encoded; + } + +} diff --git a/java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.ql b/java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.ql new file mode 100644 index 000000000000..87a077f4ddba --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.ql @@ -0,0 +1,22 @@ +import java +import semmle.code.java.security.CleartextStorageAndroidFilesystemQuery +import TestUtilities.InlineExpectationsTest + +class CleartextStorageAndroidFilesystemTest extends InlineExpectationsTest { + CleartextStorageAndroidFilesystemTest() { this = "CleartextStorageAndroidFilesystemTest" } + + override string getARelevantTag() { result = "hasCleartextStorageAndroidFilesystem" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasCleartextStorageAndroidFilesystem" and + exists(SensitiveSource data, LocalFileOpenCall s, Expr input, Expr store | + input = s.getAnInput() and + store = s.getAStore() and + data.flowsToCached(input) + | + input.getLocation() = location and + element = input.toString() and + value = "" + ) + } +} diff --git a/java/ql/test/stubs/google-android-9.0.0/androidx/security/crypto/EncryptedFile.java b/java/ql/test/stubs/google-android-9.0.0/androidx/security/crypto/EncryptedFile.java new file mode 100644 index 000000000000..26f07872841b --- /dev/null +++ b/java/ql/test/stubs/google-android-9.0.0/androidx/security/crypto/EncryptedFile.java @@ -0,0 +1,60 @@ +/* + * Copyright 2018 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package androidx.security.crypto; + +import android.content.Context; +import androidx.annotation.NonNull; +import java.io.File; +import java.io.FileInputStream; +import java.io.FileNotFoundException; +import java.io.FileOutputStream; +import java.io.IOException; +import java.security.GeneralSecurityException; + +public final class EncryptedFile { + public enum FileEncryptionScheme { + AES256_GCM_HKDF_4KB; + } + public static final class Builder { + public Builder(@NonNull File file, @NonNull Context context, @NonNull String masterKeyAlias, + @NonNull FileEncryptionScheme fileEncryptionScheme) {} + + public Builder(@NonNull Context context, @NonNull File file, @NonNull MasterKey masterKey, + @NonNull FileEncryptionScheme fileEncryptionScheme) {} + + public Builder setKeysetPrefName(@NonNull String keysetPrefName) { + return null; + } + + public Builder setKeysetAlias(@NonNull String keysetAlias) { + return null; + } + + public EncryptedFile build() throws GeneralSecurityException, IOException { + return null; + } + + } + + public FileOutputStream openFileOutput() throws GeneralSecurityException, IOException { + return null; + } + + public FileInputStream openFileInput() + throws GeneralSecurityException, IOException, FileNotFoundException { + return null; + } + +} From 79ddbd6fe45bf8b3bc94d446d94a8c807a01ecb3 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 3 Sep 2021 14:12:50 +0200 Subject: [PATCH 2/8] Fix QLDoc and the qhelp example --- java/ql/lib/semmle/code/java/security/Files.qll | 2 ++ .../CWE/CWE-312/CleartextStorageAndroidFilesystem.qhelp | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/Files.qll b/java/ql/lib/semmle/code/java/security/Files.qll index 35764cc45511..c017f0cedc12 100644 --- a/java/ql/lib/semmle/code/java/security/Files.qll +++ b/java/ql/lib/semmle/code/java/security/Files.qll @@ -1,3 +1,5 @@ +/** Provides classes and predicates to work with File objects. */ + import java import semmle.code.java.dataflow.ExternalFlow diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.qhelp b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.qhelp index b14d34539cb7..e82f6a509dd2 100644 --- a/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.qhelp +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.qhelp @@ -19,7 +19,7 @@

    In the second and third examples, the code encrypts sensitive information before saving it to the filesystem.

    - + From 1e4840e0711a1e16610f8760ea7b9df3c38e559f Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 11 Nov 2021 10:55:35 +0100 Subject: [PATCH 3/8] Fix predicate name --- .../Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql | 2 +- .../security/CWE-312/CleartextStorageAndroidFilesystemTest.ql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql index 63efe303b090..dbb04bf497d3 100644 --- a/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql @@ -18,6 +18,6 @@ from SensitiveSource data, LocalFileOpenCall s, Expr input, Expr store where input = s.getAnInput() and store = s.getAStore() and - data.flowsToCached(input) + data.flowsTo(input) select store, "Local file $@ containing $@ is stored $@. Data was added $@.", s, s.toString(), data, "sensitive data", store, "here", input, "here" diff --git a/java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.ql b/java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.ql index 87a077f4ddba..ab221086f81b 100644 --- a/java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.ql +++ b/java/ql/test/query-tests/security/CWE-312/CleartextStorageAndroidFilesystemTest.ql @@ -12,7 +12,7 @@ class CleartextStorageAndroidFilesystemTest extends InlineExpectationsTest { exists(SensitiveSource data, LocalFileOpenCall s, Expr input, Expr store | input = s.getAnInput() and store = s.getAStore() and - data.flowsToCached(input) + data.flowsTo(input) | input.getLocation() = location and element = input.toString() and From 9bbba3c96f4571965d2e4f4a8155c90a750a0821 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 11 Nov 2021 11:45:55 +0100 Subject: [PATCH 4/8] Adjust UnsupportedExternalAPIs test --- .../UnsupportedExternalAPIs/UnsupportedExternalAPIs.expected | 1 - 1 file changed, 1 deletion(-) diff --git a/java/ql/test/query-tests/Telemetry/UnsupportedExternalAPIs/UnsupportedExternalAPIs.expected b/java/ql/test/query-tests/Telemetry/UnsupportedExternalAPIs/UnsupportedExternalAPIs.expected index f2eac05e6dec..d1cf0fe32542 100644 --- a/java/ql/test/query-tests/Telemetry/UnsupportedExternalAPIs/UnsupportedExternalAPIs.expected +++ b/java/ql/test/query-tests/Telemetry/UnsupportedExternalAPIs/UnsupportedExternalAPIs.expected @@ -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 | From 22aad17d0ed833b630fb589e79df12cbf4daa93f Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 12 Nov 2021 15:36:33 +0100 Subject: [PATCH 5/8] Apply review suggestions Co-authored-by: Ethan Palm <56270045+ethanpalm@users.noreply.github.com> --- .../Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.java b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.java index 5b4911333f66..2eb19c31183b 100644 --- a/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.java +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.java @@ -1,5 +1,5 @@ public void fileSystemStorageUnsafe(String name, String password) { - // BAD - sensitive data stored in plaintext + // BAD - sensitive data stored in cleartext FileWriter fw = new FileWriter("some_file.txt"); fw.write(name + ":" + password); fw.close(); From d9e6e5aa04e550e7226c80ae468a2f0e4bbd4181 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 17 Jan 2022 10:46:21 +0100 Subject: [PATCH 6/8] Apply suggestions from code review Co-authored-by: Chris Smowton --- .../security/CleartextStorageAndroidFilesystemQuery.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll index abdfd98e4838..7d03038fc29e 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll @@ -18,7 +18,7 @@ private class AndroidFilesystemCleartextStorageSink extends CleartextStorageSink } } -/** The creation of an object that can be used to write to files to the local filesystem. */ +/** 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() @@ -56,7 +56,7 @@ private predicate isVarargs(Argument arg, DataFlow::ImplicitVarargsArray varargs } /** Holds if `store` closes `file`. */ -private predicate filesystemStore(DataFlow::Node file, Call store) { +private predicate closesFile(DataFlow::Node file, Call closeCall) { store.getCallee() instanceof CloseFileMethod and if store.getCallee().isStatic() then file.asExpr() = store @@ -67,7 +67,7 @@ private predicate filesystemStore(DataFlow::Node file, Call store) { store = file.asExpr() } -/** A method that closes a file. */ +/** A method that closes a file, perhaps after writing some data. */ private class CloseFileMethod extends Method { CloseFileMethod() { this.hasQualifiedName("java.io", ["RandomAccessFile", "FileOutputStream", "PrintStream"], From 500deac12d1a345fedfe75f8232a936f19e5d3e3 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 17 Jan 2022 11:09:55 +0100 Subject: [PATCH 7/8] Change query description --- .../Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql index dbb04bf497d3..a8c32f9aeff5 100644 --- a/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageAndroidFilesystem.ql @@ -1,6 +1,6 @@ /** * @name Cleartext storage of sensitive information in the Android filesystem - * @description Cleartext Storage of Sensitive Information 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 From ba3a4fb717eedda20c3cf2a7bd07d97d29115eb5 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 17 Jan 2022 11:13:41 +0100 Subject: [PATCH 8/8] Rename filesystemStore predicate after d9e6e5aa04e550e7226c80ae468a2f0e4bbd4181 --- .../CleartextStorageAndroidFilesystemQuery.qll | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll index 7d03038fc29e..1b1a8d8ac8a2 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll @@ -33,7 +33,7 @@ class LocalFileOpenCall extends Storable { override Expr getAStore() { exists(FilesystemFlowConfig conf, DataFlow::Node n | - filesystemStore(n, result) and + closesFile(n, result) and conf.hasFlow(DataFlow::exprNode(this), n) ) } @@ -57,14 +57,14 @@ private predicate isVarargs(Argument arg, DataFlow::ImplicitVarargsArray varargs /** Holds if `store` closes `file`. */ private predicate closesFile(DataFlow::Node file, Call closeCall) { - store.getCallee() instanceof CloseFileMethod and - if store.getCallee().isStatic() - then file.asExpr() = store - else file.asExpr() = store.getQualifier() + 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() = store.(LocalFileOpenCall).getEnclosingStmt() and - store = file.asExpr() + any(TryStmt try).getAResource() = closeCall.(LocalFileOpenCall).getEnclosingStmt() and + closeCall = file.asExpr() } /** A method that closes a file, perhaps after writing some data. */ @@ -87,7 +87,7 @@ private class FilesystemFlowConfig extends DataFlow::Configuration { override predicate isSink(DataFlow::Node sink) { filesystemInput(sink, _) or - filesystemStore(sink, _) + closesFile(sink, _) } override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {