From a30554e97c49aa14c81d8cc507e2e88fa998adac Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 17 Aug 2021 10:35:57 +0200 Subject: [PATCH 1/5] Refactored cleartext storage libraries --- .../security/CleartextStorageClassQuery.qll | 105 +++++++++++++++++ .../security/CleartextStorageCookieQuery.qll | 48 ++++++++ .../CleartextStoragePropertiesQuery.qll | 62 ++++++++++ .../java/security/CleartextStorageQuery.qll | 108 ++++++++++++++++++ .../CWE/CWE-312/CleartextStorageClass.ql | 7 +- .../CWE/CWE-312/CleartextStorageCookie.ql | 7 +- .../CWE/CWE-312/CleartextStorageProperties.ql | 7 +- 7 files changed, 329 insertions(+), 15 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/security/CleartextStorageClassQuery.qll create mode 100644 java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll create mode 100644 java/ql/lib/semmle/code/java/security/CleartextStoragePropertiesQuery.qll create mode 100644 java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageClassQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageClassQuery.qll new file mode 100644 index 000000000000..73c1db36908a --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageClassQuery.qll @@ -0,0 +1,105 @@ +/** Provides classes and predicates to reason about cleartext storage in serializable classes. */ + +import java +import semmle.code.java.frameworks.JAXB +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.DataFlow2 +import semmle.code.java.security.CleartextStorageQuery +import semmle.code.java.security.CleartextStoragePropertiesQuery + +private class ClassCleartextStorageSink extends CleartextStorageSink { + ClassCleartextStorageSink() { this.asExpr() = getInstanceInput(_, _) } +} + +/** The instantiation of a storable class, which can be stored to disk. */ +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 + result = getInstanceInput(instance, this.getConstructor().getDeclaringType()) + ) + } +} + +/** + * The instantiation of a serializable class, which can be stored to disk. + * + * Only includes tainted instances where data from a `SensitiveSource` may flow + * to an input of the `Serializable`. + */ +private class Serializable extends ClassStore { + Serializable() { + this.getConstructor().getDeclaringType().getASupertype*() instanceof TypeSerializable and + // `Properties` are `Serializable`, but handled elsewhere. + not this instanceof Properties and + // restrict attention to tainted instances + exists(SensitiveSource data | + data.flowsToCached(getInstanceInput(_, this.getConstructor().getDeclaringType())) + ) + } + + /** Gets a store, for example `outputStream.writeObject(instance)`. */ + override Expr getAStore() { + exists(ClassStoreFlowConfig conf, DataFlow::Node n | + serializableStore(n, result) and + conf.hasFlow(DataFlow::exprNode(this), n) + ) + } +} + +/** The instantiation of a marshallable class, which can be stored to disk as XML. */ +private class Marshallable extends ClassStore { + Marshallable() { this.getConstructor().getDeclaringType() instanceof JAXBElement } + + /** Gets a store, for example `marshaller.marshal(instance)`. */ + override Expr getAStore() { + exists(ClassStoreFlowConfig conf, DataFlow::Node n | + marshallableStore(n, result) and + conf.hasFlow(DataFlow::exprNode(this), n) + ) + } +} + +/** Gets an input, for example `input` in `instance.password = input`. */ +private Expr getInstanceInput(DataFlow::Node instance, RefType t) { + exists(AssignExpr a, FieldAccess fa | + instance = DataFlow::getFieldQualifier(fa) and + a.getDest() = fa and + a.getSource() = result and + fa.getField().getDeclaringType() = t + | + t.getASourceSupertype*() instanceof TypeSerializable or + t instanceof JAXBElement + ) +} + +private class ClassStoreFlowConfig extends DataFlow2::Configuration { + ClassStoreFlowConfig() { this = "ClassStoreFlowConfig" } + + override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof ClassStore } + + override predicate isSink(DataFlow::Node sink) { + exists(getInstanceInput(sink, _)) or + serializableStore(sink, _) or + marshallableStore(sink, _) + } + + override int fieldFlowBranchLimit() { result = 1 } +} + +private predicate serializableStore(DataFlow::Node instance, Expr store) { + exists(MethodAccess m | + store = m and + m.getMethod() instanceof WriteObjectMethod and + instance.asExpr() = m.getArgument(0) + ) +} + +private predicate marshallableStore(DataFlow::Node instance, Expr store) { + exists(MethodAccess m | + store = m and + m.getMethod() instanceof JAXBMarshalMethod and + instance.asExpr() = m.getArgument(0) + ) +} diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll new file mode 100644 index 000000000000..9b1ffd80986a --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll @@ -0,0 +1,48 @@ +/** Provides classes and predicates to reason about cleartext storage in cookies. */ + +import java +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.DataFlow3 +import semmle.code.java.security.CleartextStorageQuery + +private class CookieCleartextStorageSink extends CleartextStorageSink { + CookieCleartextStorageSink() { this.asExpr() = cookieInput(_) } +} + +/** The instantiation of a cookie, which can act as storage. */ +class Cookie extends Storable, ClassInstanceExpr { + Cookie() { + this.getConstructor().getDeclaringType().getQualifiedName() = "javax.servlet.http.Cookie" + } + + /** Gets an input, for example `input` in `new Cookie("...", input);`. */ + override Expr getAnInput() { result = cookieInput(this) } + + /** Gets a store, for example `response.addCookie(cookie);`. */ + override Expr getAStore() { + exists(CookieToStoreFlowConfig conf, DataFlow::Node n | + cookieStore(n, result) and + conf.hasFlow(DataFlow::exprNode(this), n) + ) + } +} + +private predicate cookieStore(DataFlow::Node cookie, Expr store) { + exists(MethodAccess m, Method def | + m.getMethod() = def and + def.getName() = "addCookie" and + def.getDeclaringType().getQualifiedName() = "javax.servlet.http.HttpServletResponse" and + store = m and + cookie.asExpr() = m.getAnArgument() + ) +} + +private class CookieToStoreFlowConfig extends DataFlow3::Configuration { + CookieToStoreFlowConfig() { this = "CookieToStoreFlowConfig" } + + override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof Cookie } + + override predicate isSink(DataFlow::Node sink) { cookieStore(sink, _) } +} + +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 new file mode 100644 index 000000000000..c75689c1fd2c --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/CleartextStoragePropertiesQuery.qll @@ -0,0 +1,62 @@ +/** Provides classes and predicates to reason about cleartext storage in Properties files. */ + +import java +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.frameworks.Properties +import semmle.code.java.security.CleartextStorageQuery + +private class PropertiesCleartextStorageSink extends CleartextStorageSink { + PropertiesCleartextStorageSink() { + exists(MethodAccess m | + m.getMethod() instanceof PropertiesSetPropertyMethod and this.asExpr() = m.getArgument(1) + ) + } +} + +/** The instantiation of a `Properties` object, which can be stored to disk. */ +class Properties extends Storable, ClassInstanceExpr { + Properties() { this.getConstructor().getDeclaringType() instanceof TypeProperty } + + /** Gets an input, for example `input` in `props.setProperty("password", input);`. */ + override Expr getAnInput() { + exists(PropertiesFlowConfig conf, DataFlow::Node n | + propertiesInput(n, result) and + conf.hasFlow(DataFlow::exprNode(this), n) + ) + } + + /** Gets a store, for example `props.store(outputStream, "...")`. */ + override Expr getAStore() { + exists(PropertiesFlowConfig conf, DataFlow::Node n | + propertiesStore(n, result) and + conf.hasFlow(DataFlow::exprNode(this), n) + ) + } +} + +private predicate propertiesInput(DataFlow::Node prop, Expr input) { + exists(MethodAccess m | + m.getMethod() instanceof PropertiesSetPropertyMethod and + input = m.getArgument(1) and + prop.asExpr() = m.getQualifier() + ) +} + +private predicate propertiesStore(DataFlow::Node prop, Expr store) { + exists(MethodAccess m | + m.getMethod() instanceof PropertiesStoreMethod and + store = m and + prop.asExpr() = m.getQualifier() + ) +} + +private class PropertiesFlowConfig extends DataFlow::Configuration { + PropertiesFlowConfig() { this = "PropertiesFlowConfig" } + + override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof Properties } + + override predicate isSink(DataFlow::Node sink) { + propertiesInput(sink, _) or + propertiesStore(sink, _) + } +} diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll new file mode 100644 index 000000000000..8d494785be2c --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll @@ -0,0 +1,108 @@ +/** Provides classes and predicates to reason about cleartext storage vulnerabilities. */ + +import java +private import semmle.code.java.dataflow.DataFlow4 +private import semmle.code.java.dataflow.TaintTracking +private import semmle.code.java.security.SensitiveActions + +/** A sink representing persistent storage that saves data in clear text. */ +abstract class CleartextStorageSink extends DataFlow::Node { } + +/** A sanitizer for flows tracking sensitive data being stored in persistent storage. */ +abstract class CleartextStorageSanitizer extends DataFlow::Node { } + +/** An additional taint step for sensitive data flowing into cleartext storage. */ +class CleartextStorageAdditionalTaintStep extends Unit { + abstract predicate step(DataFlow::Node n1, DataFlow::Node n2); +} + +/** Class for expressions that may represent 'sensitive' information */ +class SensitiveSource extends Expr { + SensitiveSource() { + // SensitiveExpr is abstract, this lets us inherit from it without + // being a technical subclass + this instanceof SensitiveExpr + } + + /** Holds if this source flows to the `sink`. */ + cached + predicate flowsToCached(Expr sink) { + exists(SensitiveSourceFlowConfig conf | + conf.hasFlow(DataFlow::exprNode(this), DataFlow::exprNode(sink)) + ) + } +} + +/** + * Class representing entities that may be stored/written, with methods + * for finding values that are stored within them, and cases + * of the entity being stored. + */ +abstract class Storable extends Call { + /** Gets an "input" that is stored in an instance of this class. */ + abstract Expr getAnInput(); + + /** Gets an expression where an instance of this class is stored (e.g. to disk). */ + abstract Expr getAStore(); +} + +private class SensitiveSourceFlowConfig extends TaintTracking::Configuration { + SensitiveSourceFlowConfig() { this = "SensitiveSourceFlowConfig" } + + override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SensitiveExpr } + + override predicate isSink(DataFlow::Node sink) { sink instanceof CleartextStorageSink } + + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer instanceof CleartextStorageSanitizer + } + + override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) { + any(CleartextStorageAdditionalTaintStep c).step(n1, n2) + } +} + +private class DefaultCleartextStorageSanitizer extends CleartextStorageSanitizer { + DefaultCleartextStorageSanitizer() { + this.getType() instanceof NumericType or + this.getType() instanceof BooleanType or + exists(EncryptedValueFlowConfig conf | conf.hasFlow(_, this)) + } +} + +/** + * Method call for encrypting sensitive information. As there are various implementations of + * encryption (reversible and non-reversible) from both JDK and third parties, this class simply + * checks method name to take a best guess to reduce false positives. + */ +private class EncryptedSensitiveMethodAccess extends MethodAccess { + EncryptedSensitiveMethodAccess() { + this.getMethod().getName().toLowerCase().matches(["%encrypt%", "%hash%", "%digest%"]) + } +} + +/** Flow configuration for encryption methods flowing to inputs of persistent storage. */ +private class EncryptedValueFlowConfig extends DataFlow4::Configuration { + EncryptedValueFlowConfig() { this = "EncryptedValueFlowConfig" } + + override predicate isSource(DataFlow::Node src) { + src.asExpr() instanceof EncryptedSensitiveMethodAccess + } + + override predicate isSink(DataFlow::Node sink) { sink instanceof CleartextStorageSink } +} + +/** A taint step for `EditText.toString` in Android. */ +private class AndroidEditTextCleartextStorageStep extends CleartextStorageAdditionalTaintStep { + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + // EditText.getText() return type is parsed as `Object`, so we need to + // add a taint step for `Object.toString` to model `editText.getText().toString()` + exists(MethodAccess ma, Method m | + ma.getMethod() = m and + m.getDeclaringType() instanceof TypeObject and + m.hasName("toString") + | + n1.asExpr() = ma.getQualifier() and n2.asExpr() = ma + ) + } +} diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageClass.ql b/java/ql/src/Security/CWE/CWE-312/CleartextStorageClass.ql index e14b9bfe552d..7b91df39bfdd 100644 --- a/java/ql/src/Security/CWE/CWE-312/CleartextStorageClass.ql +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageClass.ql @@ -12,15 +12,12 @@ */ import java -import SensitiveStorage +import semmle.code.java.security.CleartextStorageClassQuery from SensitiveSource data, ClassStore s, Expr input, Expr store where input = s.getAnInput() and store = s.getAStore() and - data.flowsToCached(input) and - // Exclude results in test code. - not testMethod(store.getEnclosingCallable()) and - not testMethod(data.getEnclosingCallable()) + data.flowsToCached(input) select store, "Storable class $@ containing $@ is stored here. Data was added $@.", s, s.toString(), data, "sensitive data", input, "here" diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageCookie.ql b/java/ql/src/Security/CWE/CWE-312/CleartextStorageCookie.ql index c5a76434dcd6..2641a5074735 100644 --- a/java/ql/src/Security/CWE/CWE-312/CleartextStorageCookie.ql +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageCookie.ql @@ -11,15 +11,12 @@ */ import java -import SensitiveStorage +import semmle.code.java.security.CleartextStorageCookieQuery from SensitiveSource data, Cookie s, Expr input, Expr store where input = s.getAnInput() and store = s.getAStore() and - data.flowsToCached(input) and - // Exclude results in test code. - not testMethod(store.getEnclosingCallable()) and - not testMethod(data.getEnclosingCallable()) + data.flowsToCached(input) select store, "Cookie $@ containing $@ is stored here. Data was added $@.", s, s.toString(), data, "sensitive data", input, "here" diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageProperties.ql b/java/ql/src/Security/CWE/CWE-312/CleartextStorageProperties.ql index 495fd3f6f208..5be703dba0da 100644 --- a/java/ql/src/Security/CWE/CWE-312/CleartextStorageProperties.ql +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageProperties.ql @@ -11,15 +11,12 @@ */ import java -import SensitiveStorage +import semmle.code.java.security.CleartextStoragePropertiesQuery from SensitiveSource data, Properties s, Expr input, Expr store where input = s.getAnInput() and store = s.getAStore() and - data.flowsToCached(input) and - // Exclude results in test code. - not testMethod(store.getEnclosingCallable()) and - not testMethod(data.getEnclosingCallable()) + data.flowsToCached(input) select store, "'Properties' class $@ containing $@ is stored here. Data was added $@.", s, s.toString(), data, "sensitive data", input, "here" From 563e8a2bd651a41202c75be8342234239df6e005 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 3 Sep 2021 13:50:50 +0200 Subject: [PATCH 2/5] Remove unused library --- .../Security/CWE/CWE-312/SensitiveStorage.qll | 245 ------------------ 1 file changed, 245 deletions(-) delete mode 100644 java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll diff --git a/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll b/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll deleted file mode 100644 index b07105e3bf5a..000000000000 --- a/java/ql/src/Security/CWE/CWE-312/SensitiveStorage.qll +++ /dev/null @@ -1,245 +0,0 @@ -import java -import semmle.code.java.frameworks.Properties -import semmle.code.java.frameworks.JAXB -import semmle.code.java.dataflow.TaintTracking -import semmle.code.java.dataflow.DataFlow3 -import semmle.code.java.dataflow.DataFlow4 -import semmle.code.java.security.SensitiveActions - -/** Test code filter. */ -predicate testMethod(Method m) { - ( - m instanceof TestMethod or - m.getDeclaringType() instanceof TestClass - ) and - // Do report results in the Juliet tests. - not m.getLocation().getFile().getAbsolutePath().matches("%CWE%") -} - -private class SensitiveSourceFlowConfig extends TaintTracking::Configuration { - SensitiveSourceFlowConfig() { this = "SensitiveStorage::SensitiveSourceFlowConfig" } - - override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SensitiveExpr } - - override predicate isSink(DataFlow::Node sink) { - sink.asExpr() = cookieInput(_) - or - exists(MethodAccess m | - m.getMethod() instanceof PropertiesSetPropertyMethod and sink.asExpr() = m.getArgument(1) - ) - or - sink.asExpr() = getInstanceInput(_, _) - } - - override predicate isSanitizer(DataFlow::Node n) { - n.getType() instanceof NumericType or n.getType() instanceof BooleanType - } -} - -/** Class for expressions that may represent 'sensitive' information */ -class SensitiveSource extends Expr { - SensitiveSource() { - // SensitiveExpr is abstract, this lets us inherit from it without - // being a technical subclass - this instanceof SensitiveExpr - } - - /** Holds if this source flows to the `sink`. */ - cached - predicate flowsToCached(Expr sink) { - exists(SensitiveSourceFlowConfig conf | - conf.hasFlow(DataFlow::exprNode(this), DataFlow::exprNode(sink)) - ) - } -} - -/** - * Class representing entities that may be stored/written, with methods - * for finding values that are stored within them, and cases - * of the entity being stored. - */ -abstract class Storable extends ClassInstanceExpr { - /** Gets an "input" that is stored in an instance of this class. */ - abstract Expr getAnInput(); - - /** Gets an expression where an instance of this class is stored (e.g. to disk). */ - abstract Expr getAStore(); -} - -private predicate cookieStore(DataFlow::Node cookie, Expr store) { - exists(MethodAccess m, Method def | - m.getMethod() = def and - def.getName() = "addCookie" and - def.getDeclaringType().getQualifiedName() = "javax.servlet.http.HttpServletResponse" and - store = m and - cookie.asExpr() = m.getAnArgument() - ) -} - -private class CookieToStoreFlowConfig extends DataFlow2::Configuration { - CookieToStoreFlowConfig() { this = "SensitiveStorage::CookieToStoreFlowConfig" } - - override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof Cookie } - - override predicate isSink(DataFlow::Node sink) { cookieStore(sink, _) } -} - -private Expr cookieInput(Cookie c) { result = c.getArgument(1) } - -/** The instantiation of a cookie, which can act as storage. */ -class Cookie extends Storable { - Cookie() { - this.getConstructor().getDeclaringType().getQualifiedName() = "javax.servlet.http.Cookie" - } - - /** Gets an input, for example `input` in `new Cookie("...", input);`. */ - override Expr getAnInput() { result = cookieInput(this) } - - /** Gets a store, for example `response.addCookie(cookie);`. */ - override Expr getAStore() { - exists(CookieToStoreFlowConfig conf, DataFlow::Node n | - cookieStore(n, result) and - conf.hasFlow(DataFlow::exprNode(this), n) - ) - } -} - -private predicate propertiesInput(DataFlow::Node prop, Expr input) { - exists(MethodAccess m | - m.getMethod() instanceof PropertiesSetPropertyMethod and - input = m.getArgument(1) and - prop.asExpr() = m.getQualifier() - ) -} - -private predicate propertiesStore(DataFlow::Node prop, Expr store) { - exists(MethodAccess m | - m.getMethod() instanceof PropertiesStoreMethod and - store = m and - prop.asExpr() = m.getQualifier() - ) -} - -private class PropertiesFlowConfig extends DataFlow3::Configuration { - PropertiesFlowConfig() { this = "SensitiveStorage::PropertiesFlowConfig" } - - override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof Properties } - - override predicate isSink(DataFlow::Node sink) { - propertiesInput(sink, _) or - propertiesStore(sink, _) - } -} - -/** The instantiation of a `Properties` object, which can be stored to disk. */ -class Properties extends Storable { - Properties() { this.getConstructor().getDeclaringType() instanceof TypeProperty } - - /** Gets an input, for example `input` in `props.setProperty("password", input);`. */ - override Expr getAnInput() { - exists(PropertiesFlowConfig conf, DataFlow::Node n | - propertiesInput(n, result) and - conf.hasFlow(DataFlow::exprNode(this), n) - ) - } - - /** Gets a store, for example `props.store(outputStream, "...")`. */ - override Expr getAStore() { - exists(PropertiesFlowConfig conf, DataFlow::Node n | - propertiesStore(n, result) and - conf.hasFlow(DataFlow::exprNode(this), n) - ) - } -} - -abstract class ClassStore extends Storable { - /** 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 - result = getInstanceInput(instance, this.getConstructor().getDeclaringType()) - ) - } -} - -/** Gets an input, for example `input` in `instance.password = input`. */ -private Expr getInstanceInput(DataFlow::Node instance, RefType t) { - exists(AssignExpr a, FieldAccess fa | - instance = DataFlow::getFieldQualifier(fa) and - a.getDest() = fa and - a.getSource() = result and - fa.getField().getDeclaringType() = t - | - t.getASourceSupertype*() instanceof TypeSerializable or - t instanceof JAXBElement - ) -} - -private class ClassStoreFlowConfig extends DataFlow4::Configuration { - ClassStoreFlowConfig() { this = "SensitiveStorage::ClassStoreFlowConfig" } - - override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof ClassStore } - - override predicate isSink(DataFlow::Node sink) { - exists(getInstanceInput(sink, _)) or - serializableStore(sink, _) or - marshallableStore(sink, _) - } - - override int fieldFlowBranchLimit() { result = 1 } -} - -private predicate serializableStore(DataFlow::Node instance, Expr store) { - exists(MethodAccess m | - store = m and - m.getMethod() instanceof WriteObjectMethod and - instance.asExpr() = m.getArgument(0) - ) -} - -private predicate marshallableStore(DataFlow::Node instance, Expr store) { - exists(MethodAccess m | - store = m and - m.getMethod() instanceof JAXBMarshalMethod and - instance.asExpr() = m.getArgument(0) - ) -} - -/** - * The instantiation of a serializable class, which can be stored to disk. - * - * Only includes tainted instances where data from a `SensitiveSource` may flow - * to an input of the `Serializable`. - */ -class Serializable extends ClassStore { - Serializable() { - this.getConstructor().getDeclaringType().getASupertype*() instanceof TypeSerializable and - // `Properties` are `Serializable`, but handled elsewhere. - not this instanceof Properties and - // restrict attention to tainted instances - exists(SensitiveSource data | - data.flowsToCached(getInstanceInput(_, this.getConstructor().getDeclaringType())) - ) - } - - /** Gets a store, for example `outputStream.writeObject(instance)`. */ - override Expr getAStore() { - exists(ClassStoreFlowConfig conf, DataFlow::Node n | - serializableStore(n, result) and - conf.hasFlow(DataFlow::exprNode(this), n) - ) - } -} - -/** The instantiation of a marshallable class, which can be stored to disk as XML. */ -class Marshallable extends ClassStore { - Marshallable() { this.getConstructor().getDeclaringType() instanceof JAXBElement } - - /** Gets a store, for example `marshaller.marshal(instance)`. */ - override Expr getAStore() { - exists(ClassStoreFlowConfig conf, DataFlow::Node n | - marshallableStore(n, result) and - conf.hasFlow(DataFlow::exprNode(this), n) - ) - } -} From 51d2b5225e7ab357714caab263937c1622178ac1 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 14 Sep 2021 12:38:41 +0200 Subject: [PATCH 3/5] Remove cached property from SensitiveSource::flowsTo --- .../semmle/code/java/security/CleartextStorageClassQuery.qll | 2 +- .../ql/lib/semmle/code/java/security/CleartextStorageQuery.qll | 3 +-- java/ql/src/Security/CWE/CWE-312/CleartextStorageClass.ql | 2 +- java/ql/src/Security/CWE/CWE-312/CleartextStorageCookie.ql | 2 +- java/ql/src/Security/CWE/CWE-312/CleartextStorageProperties.ql | 2 +- 5 files changed, 5 insertions(+), 6 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageClassQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageClassQuery.qll index 73c1db36908a..0f0528fde5ff 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageClassQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageClassQuery.qll @@ -35,7 +35,7 @@ private class Serializable extends ClassStore { not this instanceof Properties and // restrict attention to tainted instances exists(SensitiveSource data | - data.flowsToCached(getInstanceInput(_, this.getConstructor().getDeclaringType())) + data.flowsTo(getInstanceInput(_, this.getConstructor().getDeclaringType())) ) } diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll index 8d494785be2c..352893c64e96 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll @@ -25,8 +25,7 @@ class SensitiveSource extends Expr { } /** Holds if this source flows to the `sink`. */ - cached - predicate flowsToCached(Expr sink) { + predicate flowsTo(Expr sink) { exists(SensitiveSourceFlowConfig conf | conf.hasFlow(DataFlow::exprNode(this), DataFlow::exprNode(sink)) ) diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageClass.ql b/java/ql/src/Security/CWE/CWE-312/CleartextStorageClass.ql index 7b91df39bfdd..525e8148a8fe 100644 --- a/java/ql/src/Security/CWE/CWE-312/CleartextStorageClass.ql +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageClass.ql @@ -18,6 +18,6 @@ from SensitiveSource data, ClassStore s, Expr input, Expr store where input = s.getAnInput() and store = s.getAStore() and - data.flowsToCached(input) + data.flowsTo(input) select store, "Storable class $@ containing $@ is stored here. Data was added $@.", s, s.toString(), data, "sensitive data", input, "here" diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageCookie.ql b/java/ql/src/Security/CWE/CWE-312/CleartextStorageCookie.ql index 2641a5074735..4e370e29d295 100644 --- a/java/ql/src/Security/CWE/CWE-312/CleartextStorageCookie.ql +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageCookie.ql @@ -17,6 +17,6 @@ from SensitiveSource data, Cookie s, Expr input, Expr store where input = s.getAnInput() and store = s.getAStore() and - data.flowsToCached(input) + data.flowsTo(input) select store, "Cookie $@ containing $@ is stored here. Data was added $@.", s, s.toString(), data, "sensitive data", input, "here" diff --git a/java/ql/src/Security/CWE/CWE-312/CleartextStorageProperties.ql b/java/ql/src/Security/CWE/CWE-312/CleartextStorageProperties.ql index 5be703dba0da..9a8ce42130ea 100644 --- a/java/ql/src/Security/CWE/CWE-312/CleartextStorageProperties.ql +++ b/java/ql/src/Security/CWE/CWE-312/CleartextStorageProperties.ql @@ -17,6 +17,6 @@ from SensitiveSource data, Properties s, Expr input, Expr store where input = s.getAnInput() and store = s.getAStore() and - data.flowsToCached(input) + data.flowsTo(input) select store, "'Properties' class $@ containing $@ is stored here. Data was added $@.", s, s.toString(), data, "sensitive data", input, "here" From d0b9920cac5c762358cda00ba57c24e6c9cf2eb4 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 14 Sep 2021 12:49:38 +0200 Subject: [PATCH 4/5] Fix encryption sanitizer It now discards sensitive exprs (sources) instead of sinks for better precision --- java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll index 352893c64e96..f5c5519f48a9 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll @@ -88,7 +88,7 @@ private class EncryptedValueFlowConfig extends DataFlow4::Configuration { src.asExpr() instanceof EncryptedSensitiveMethodAccess } - override predicate isSink(DataFlow::Node sink) { sink instanceof CleartextStorageSink } + override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof SensitiveExpr } } /** A taint step for `EditText.toString` in Android. */ From b52a2cd292df3ee79fc812e688a6c1000e439e24 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 23 Sep 2021 15:48:15 +0200 Subject: [PATCH 5/5] Apply code review comments Co-authored-by: Anders Schack-Mulligen --- .../code/java/security/CleartextStorageCookieQuery.qll | 4 ++-- .../semmle/code/java/security/CleartextStorageQuery.qll | 8 +------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll index 9b1ffd80986a..bd071e7fef66 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll @@ -12,7 +12,7 @@ private class CookieCleartextStorageSink extends CleartextStorageSink { /** The instantiation of a cookie, which can act as storage. */ class Cookie extends Storable, ClassInstanceExpr { Cookie() { - this.getConstructor().getDeclaringType().getQualifiedName() = "javax.servlet.http.Cookie" + this.getConstructor().getDeclaringType().hasQualifiedName("javax.servlet.http", "Cookie") } /** Gets an input, for example `input` in `new Cookie("...", input);`. */ @@ -31,7 +31,7 @@ private predicate cookieStore(DataFlow::Node cookie, Expr store) { exists(MethodAccess m, Method def | m.getMethod() = def and def.getName() = "addCookie" and - def.getDeclaringType().getQualifiedName() = "javax.servlet.http.HttpServletResponse" and + def.getDeclaringType().hasQualifiedName("javax.servlet.http", "HttpServletResponse") and store = m and cookie.asExpr() = m.getAnArgument() ) diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll index f5c5519f48a9..96197b8cab17 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll @@ -17,13 +17,7 @@ class CleartextStorageAdditionalTaintStep extends Unit { } /** Class for expressions that may represent 'sensitive' information */ -class SensitiveSource extends Expr { - SensitiveSource() { - // SensitiveExpr is abstract, this lets us inherit from it without - // being a technical subclass - this instanceof SensitiveExpr - } - +class SensitiveSource extends Expr instanceof SensitiveExpr { /** Holds if this source flows to the `sink`. */ predicate flowsTo(Expr sink) { exists(SensitiveSourceFlowConfig conf |