From aa7934161aacf31869d6ecfa6e17ac6dc0b774e8 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Sun, 19 Mar 2023 23:24:35 -0400 Subject: [PATCH] Refactor CleartextStorage libraries --- .../CleartextStorageAndroidDatabaseQuery.qll | 20 +++++------ ...CleartextStorageAndroidFilesystemQuery.qll | 20 +++++------ .../security/CleartextStorageClassQuery.qll | 24 ++++++------- .../security/CleartextStorageCookieQuery.qll | 14 ++++---- .../CleartextStoragePropertiesQuery.qll | 18 +++++----- .../java/security/CleartextStorageQuery.qll | 34 ++++++++----------- .../CleartextStorageSharedPrefsQuery.qll | 18 +++++----- 7 files changed, 71 insertions(+), 77 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidDatabaseQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidDatabaseQuery.qll index 7edd9e74c50..d995d11ce5b 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidDatabaseQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidDatabaseQuery.qll @@ -29,16 +29,16 @@ class LocalDatabaseOpenMethodAccess extends Storable, Call { } override Expr getAnInput() { - exists(LocalDatabaseFlowConfig config, DataFlow::Node database | + exists(DataFlow::Node database | localDatabaseInput(database, result) and - config.hasFlow(DataFlow::exprNode(this), database) + LocalDatabaseFlow::flow(DataFlow::exprNode(this), database) ) } override Expr getAStore() { - exists(LocalDatabaseFlowConfig config, DataFlow::Node database | + exists(DataFlow::Node database | localDatabaseStore(database, result) and - config.hasFlow(DataFlow::exprNode(this), database) + LocalDatabaseFlow::flow(DataFlow::exprNode(this), database) ) } } @@ -93,19 +93,17 @@ private predicate localDatabaseStore(DataFlow::Node database, MethodAccess store ) } -private class LocalDatabaseFlowConfig extends DataFlow::Configuration { - LocalDatabaseFlowConfig() { this = "LocalDatabaseFlowConfig" } - - override predicate isSource(DataFlow::Node source) { +private module LocalDatabaseFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source.asExpr() instanceof LocalDatabaseOpenMethodAccess } - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { localDatabaseInput(sink, _) or localDatabaseStore(sink, _) } - override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { + predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { // Adds a step for tracking databases through field flow, that is, a database is opened and // assigned to a field, and then an input or store method is called on that field elsewhere. exists(Field f | @@ -115,3 +113,5 @@ private class LocalDatabaseFlowConfig extends DataFlow::Configuration { ) } } + +private module LocalDatabaseFlow = DataFlow::Global; diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll index 89cc7ac021b..2641a3ab0df 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll @@ -24,16 +24,16 @@ class LocalFileOpenCall extends Storable { } override Expr getAnInput() { - exists(FilesystemFlowConfig conf, DataFlow::Node n | + exists(DataFlow::Node n | filesystemInput(n, result) and - conf.hasFlow(DataFlow::exprNode(this), n) + FilesystemFlow::flow(DataFlow::exprNode(this), n) ) } override Expr getAStore() { - exists(FilesystemFlowConfig conf, DataFlow::Node n | + exists(DataFlow::Node n | closesFile(n, result) and - conf.hasFlow(DataFlow::exprNode(this), n) + FilesystemFlow::flow(DataFlow::exprNode(this), n) ) } } @@ -79,17 +79,15 @@ private class CloseFileMethod extends Method { } } -private class FilesystemFlowConfig extends DataFlow::Configuration { - FilesystemFlowConfig() { this = "FilesystemFlowConfig" } +private module FilesystemFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src.asExpr() instanceof LocalFileOpenCall } - override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof LocalFileOpenCall } - - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { filesystemInput(sink, _) or closesFile(sink, _) } - override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { // Add nested Writer constructors as extra data flow steps exists(ClassInstanceExpr cie | cie.getConstructedType().getAnAncestor().hasQualifiedName("java.io", "Writer") and @@ -98,3 +96,5 @@ private class FilesystemFlowConfig extends DataFlow::Configuration { ) } } + +private module FilesystemFlow = DataFlow::Global; diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageClassQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageClassQuery.qll index 3fe06a2de08..41e5c060816 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageClassQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageClassQuery.qll @@ -14,8 +14,8 @@ private class ClassCleartextStorageSink extends CleartextStorageSink { abstract class ClassStore extends Storable, ClassInstanceExpr { /** Gets an input, for example `input` in `instance.password = input`. */ override Expr getAnInput() { - exists(ClassStoreFlowConfig conf, DataFlow::Node instance | - conf.hasFlow(DataFlow::exprNode(this), instance) and + exists(DataFlow::Node instance | + ClassStoreFlow::flow(DataFlow::exprNode(this), instance) and result = getInstanceInput(instance, this.getConstructor().getDeclaringType()) ) } @@ -40,9 +40,9 @@ private class Serializable extends ClassStore { /** Gets a store, for example `outputStream.writeObject(instance)`. */ override Expr getAStore() { - exists(ClassStoreFlowConfig conf, DataFlow::Node n | + exists(DataFlow::Node n | serializableStore(n, result) and - conf.hasFlow(DataFlow::exprNode(this), n) + ClassStoreFlow::flow(DataFlow::exprNode(this), n) ) } } @@ -53,9 +53,9 @@ private class Marshallable extends ClassStore { /** Gets a store, for example `marshaller.marshal(instance)`. */ override Expr getAStore() { - exists(ClassStoreFlowConfig conf, DataFlow::Node n | + exists(DataFlow::Node n | marshallableStore(n, result) and - conf.hasFlow(DataFlow::exprNode(this), n) + ClassStoreFlow::flow(DataFlow::exprNode(this), n) ) } } @@ -73,20 +73,20 @@ private Expr getInstanceInput(DataFlow::Node instance, RefType t) { ) } -private class ClassStoreFlowConfig extends DataFlow::Configuration { - ClassStoreFlowConfig() { this = "ClassStoreFlowConfig" } +private module ClassStoreFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src.asExpr() instanceof ClassStore } - override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof ClassStore } - - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { exists(getInstanceInput(sink, _)) or serializableStore(sink, _) or marshallableStore(sink, _) } - override int fieldFlowBranchLimit() { result = 1 } + int fieldFlowBranchLimit() { result = 1 } } +private module ClassStoreFlow = DataFlow::Global; + private predicate serializableStore(DataFlow::Node instance, Expr store) { exists(MethodAccess m | store = m and diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll index bd071e7fef6..59c098281f1 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll @@ -20,9 +20,9 @@ class Cookie extends Storable, ClassInstanceExpr { /** Gets a store, for example `response.addCookie(cookie);`. */ override Expr getAStore() { - exists(CookieToStoreFlowConfig conf, DataFlow::Node n | + exists(DataFlow::Node n | cookieStore(n, result) and - conf.hasFlow(DataFlow::exprNode(this), n) + CookieToStoreFlow::flow(DataFlow::exprNode(this), n) ) } } @@ -37,12 +37,12 @@ private predicate cookieStore(DataFlow::Node cookie, Expr store) { ) } -private class CookieToStoreFlowConfig extends DataFlow3::Configuration { - CookieToStoreFlowConfig() { this = "CookieToStoreFlowConfig" } +private module CookieToStoreFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src.asExpr() instanceof Cookie } - override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof Cookie } - - override predicate isSink(DataFlow::Node sink) { cookieStore(sink, _) } + predicate isSink(DataFlow::Node sink) { cookieStore(sink, _) } } +private module CookieToStoreFlow = DataFlow::Global; + private Expr cookieInput(Cookie c) { result = c.getArgument(1) } diff --git a/java/ql/lib/semmle/code/java/security/CleartextStoragePropertiesQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStoragePropertiesQuery.qll index c75689c1fd2..f262104078e 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStoragePropertiesQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStoragePropertiesQuery.qll @@ -19,17 +19,17 @@ class Properties extends Storable, ClassInstanceExpr { /** Gets an input, for example `input` in `props.setProperty("password", input);`. */ override Expr getAnInput() { - exists(PropertiesFlowConfig conf, DataFlow::Node n | + exists(DataFlow::Node n | propertiesInput(n, result) and - conf.hasFlow(DataFlow::exprNode(this), n) + PropertiesFlow::flow(DataFlow::exprNode(this), n) ) } /** Gets a store, for example `props.store(outputStream, "...")`. */ override Expr getAStore() { - exists(PropertiesFlowConfig conf, DataFlow::Node n | + exists(DataFlow::Node n | propertiesStore(n, result) and - conf.hasFlow(DataFlow::exprNode(this), n) + PropertiesFlow::flow(DataFlow::exprNode(this), n) ) } } @@ -50,13 +50,13 @@ private predicate propertiesStore(DataFlow::Node prop, Expr store) { ) } -private class PropertiesFlowConfig extends DataFlow::Configuration { - PropertiesFlowConfig() { this = "PropertiesFlowConfig" } +private module PropertiesFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src.asExpr() instanceof Properties } - override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof Properties } - - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { propertiesInput(sink, _) or propertiesStore(sink, _) } } + +private module PropertiesFlow = DataFlow::Global; diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll index 8bd14b63ea6..fa19a25f110 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll @@ -21,9 +21,7 @@ class CleartextStorageAdditionalTaintStep extends Unit { class SensitiveSource extends Expr instanceof SensitiveExpr { /** Holds if this source flows to the `sink`. */ predicate flowsTo(Expr sink) { - exists(SensitiveSourceFlowConfig conf | - conf.hasFlow(DataFlow::exprNode(this), DataFlow::exprNode(sink)) - ) + SensitiveSourceFlow::flow(DataFlow::exprNode(this), DataFlow::exprNode(sink)) } } @@ -40,27 +38,25 @@ abstract class Storable extends Call { abstract Expr getAStore(); } -private class SensitiveSourceFlowConfig extends TaintTracking2::Configuration { - SensitiveSourceFlowConfig() { this = "SensitiveSourceFlowConfig" } +private module SensitiveSourceFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SensitiveExpr } - override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SensitiveExpr } + predicate isSink(DataFlow::Node sink) { sink instanceof CleartextStorageSink } - override predicate isSink(DataFlow::Node sink) { sink instanceof CleartextStorageSink } + predicate isBarrier(DataFlow::Node sanitizer) { sanitizer instanceof CleartextStorageSanitizer } - override predicate isSanitizer(DataFlow::Node sanitizer) { - sanitizer instanceof CleartextStorageSanitizer - } - - override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) { + predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { any(CleartextStorageAdditionalTaintStep c).step(n1, n2) } } +private module SensitiveSourceFlow = TaintTracking::Global; + private class DefaultCleartextStorageSanitizer extends CleartextStorageSanitizer { DefaultCleartextStorageSanitizer() { this.getType() instanceof NumericType or this.getType() instanceof BooleanType or - exists(EncryptedValueFlowConfig conf | conf.hasFlow(_, this)) + EncryptedValueFlow::flow(_, this) } } @@ -76,12 +72,10 @@ private class EncryptedSensitiveMethodAccess extends MethodAccess { } /** Flow configuration for encryption methods flowing to inputs of persistent storage. */ -private class EncryptedValueFlowConfig extends DataFlow4::Configuration { - EncryptedValueFlowConfig() { this = "EncryptedValueFlowConfig" } +private module EncryptedValueFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src.asExpr() instanceof EncryptedSensitiveMethodAccess } - override predicate isSource(DataFlow::Node src) { - src.asExpr() instanceof EncryptedSensitiveMethodAccess - } - - override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof SensitiveExpr } + predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof SensitiveExpr } } + +private module EncryptedValueFlow = DataFlow::Global; diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageSharedPrefsQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageSharedPrefsQuery.qll index e67a2d1aa21..07764a251f7 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageSharedPrefsQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageSharedPrefsQuery.qll @@ -28,17 +28,17 @@ class SharedPreferencesEditorMethodAccess extends Storable, MethodAccess { /** Gets an input, for example `password` in `editor.putString("password", password);`. */ override Expr getAnInput() { - exists(SharedPreferencesFlowConfig conf, DataFlow::Node editor | + exists(DataFlow::Node editor | sharedPreferencesInput(editor, result) and - conf.hasFlow(DataFlow::exprNode(this), editor) + SharedPreferencesFlow::flow(DataFlow::exprNode(this), editor) ) } /** Gets a store, for example `editor.commit();`. */ override Expr getAStore() { - exists(SharedPreferencesFlowConfig conf, DataFlow::Node editor | + exists(DataFlow::Node editor | sharedPreferencesStore(editor, result) and - conf.hasFlow(DataFlow::exprNode(this), editor) + SharedPreferencesFlow::flow(DataFlow::exprNode(this), editor) ) } } @@ -65,15 +65,15 @@ private predicate sharedPreferencesStore(DataFlow::Node editor, MethodAccess m) } /** Flow from `SharedPreferences.Editor` to either a setter or a store method. */ -private class SharedPreferencesFlowConfig extends DataFlow::Configuration { - SharedPreferencesFlowConfig() { this = "SharedPreferencesFlowConfig" } - - override predicate isSource(DataFlow::Node src) { +private module SharedPreferencesFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SharedPreferencesEditorMethodAccess } - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { sharedPreferencesInput(sink, _) or sharedPreferencesStore(sink, _) } } + +private module SharedPreferencesFlow = DataFlow::Global;