diff --git a/java/ql/src/Security/CWE/CWE-295/InsecureTrustManager.ql b/java/ql/src/Security/CWE/CWE-295/InsecureTrustManager.ql index afe5a2f1d54..866641d3cf1 100644 --- a/java/ql/src/Security/CWE/CWE-295/InsecureTrustManager.ql +++ b/java/ql/src/Security/CWE/CWE-295/InsecureTrustManager.ql @@ -13,8 +13,8 @@ import java import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.FlowSources -import semmle.code.java.dataflow.TaintTracking2 import semmle.code.java.security.Encryption +import semmle.code.java.security.SecurityFlag import DataFlow::PathGraph /** @@ -80,72 +80,30 @@ class InsecureTrustManagerConfiguration extends TaintTracking::Configuration { } } -bindingset[result] -private string getAFlagName() { - result - .regexpMatch("(?i).*(secure|disable|selfCert|selfSign|validat|verif|trust|ignore|nocertificatecheck).*") and - result != "equalsIgnoreCase" -} - /** - * A flag has to either be of type `String`, `boolean` or `Boolean`. + * Flags suggesting a deliberately insecure `TrustManager` usage. */ -private class FlagType extends Type { - FlagType() { - this instanceof TypeString - or - this instanceof BooleanType +private class InsecureTrustManagerFlag extends FlagKind { + InsecureTrustManagerFlag() { this = "InsecureTrustManagerFlag" } + + bindingset[result] + override string getAFlagName() { + result + .regexpMatch("(?i).*(secure|disable|selfCert|selfSign|validat|verif|trust|ignore|nocertificatecheck).*") and + result != "equalsIgnoreCase" } } -/** Holds if `source` should is considered a flag. */ -private predicate isFlag(DataFlow::Node source) { - exists(VarAccess v | v.getVariable().getName() = getAFlagName() | - source.asExpr() = v and v.getType() instanceof FlagType - ) - or - exists(StringLiteral s | s.getRepresentedString() = getAFlagName() | source.asExpr() = s) - or - exists(MethodAccess ma | ma.getMethod().getName() = getAFlagName() | - source.asExpr() = ma and - ma.getType() instanceof FlagType - ) +/** Gets a guard that represents a (likely) flag controlling an insecure `TrustManager` use. */ +private Guard getAnInsecureTrustManagerFlagGuard() { + result = any(InsecureTrustManagerFlag flag).getAFlag().asExpr() } -/** - * Holds if there is local flow from `node1` to `node2` either due to standard data-flow steps or the - * following custom flow steps: - * 1. `Boolean.parseBoolean(taintedValue)` taints the return value of `parseBoolean`. - * 2. A call to an `EnvReadMethod` such as `System.getProperty` where a tainted value is used as an argument. - * The return value of such a method is then tainted. - */ -private predicate flagFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - DataFlow::localFlowStep(node1, node2) - or - exists(MethodAccess ma | ma.getMethod() = any(EnvReadMethod m) | - ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr() - ) - or - exists(MethodAccess ma | - ma.getMethod().hasName("parseBoolean") and - ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Boolean") - | - ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr() - ) -} - -/** Gets a guard that depends on a flag. */ -private Guard getAGuard() { - exists(DataFlow::Node source, DataFlow::Node sink | - isFlag(source) and - flagFlowStep*(source, sink) and - sink.asExpr() = result - ) -} - -/** Holds if `node` is guarded by a flag that suggests an intentionally insecure feature. */ +/** Holds if `node` is guarded by a flag that suggests an intentionally insecure use. */ private predicate isNodeGuardedByFlag(DataFlow::Node node) { - exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) | g = getAGuard()) + exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) | + g = getASecurityFeatureFlagGuard() or g = getAnInsecureTrustManagerFlagGuard() + ) } from diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index 5fce4a588ea..6d68c6642d2 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -15,6 +15,7 @@ import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.Encryption +import semmle.code.java.security.SecurityFlag import DataFlow::PathGraph private import semmle.code.java.dataflow.ExternalFlow @@ -86,76 +87,30 @@ private class HostnameVerifierSink extends DataFlow::Node { HostnameVerifierSink() { sinkNode(this, "set-hostname-verifier") } } -bindingset[result] -private string getAFlagName() { - result - .regexpMatch("(?i).*(secure|disable|selfCert|selfSign|validat|verif|trust|ignore|nocertificatecheck).*") -} - /** - * A flag has to either be of type `String`, `boolean` or `Boolean`. + * Flags suggesting a deliberately unsafe `HostnameVerifier` usage. */ -private class FlagType extends Type { - FlagType() { - this instanceof TypeString - or - this instanceof BooleanType +private class UnsafeHostnameVerificationFlag extends FlagKind { + UnsafeHostnameVerificationFlag() { this = "UnsafeHostnameVerificationFlag" } + + bindingset[result] + override string getAFlagName() { + result + .regexpMatch("(?i).*(secure|disable|selfCert|selfSign|validat|verif|trust|ignore|nocertificatecheck).*") and + result != "equalsIgnoreCase" } } -private predicate isEqualsIgnoreCaseMethodAccess(MethodAccess ma) { - ma.getMethod().hasName("equalsIgnoreCase") and - ma.getMethod().getDeclaringType() instanceof TypeString +/** Gets a guard that represents a (likely) flag controlling an unsafe `HostnameVerifier` use. */ +private Guard getAnUnsafeHostnameVerifierFlagGuard() { + result = any(UnsafeHostnameVerificationFlag flag).getAFlag().asExpr() } -/** Holds if `source` should is considered a flag. */ -private predicate isFlag(DataFlow::Node source) { - exists(VarAccess v | v.getVariable().getName() = getAFlagName() | - source.asExpr() = v and v.getType() instanceof FlagType - ) - or - exists(StringLiteral s | s.getRepresentedString() = getAFlagName() | source.asExpr() = s) - or - exists(MethodAccess ma | ma.getMethod().getName() = getAFlagName() | - source.asExpr() = ma and - ma.getType() instanceof FlagType and - not isEqualsIgnoreCaseMethodAccess(ma) - ) -} - -/** - * Holds if there is flow from `node1` to `node2` either due to local flow or due to custom flow steps: - * 1. `Boolean.parseBoolean(taintedValue)` taints the return value of `parseBoolean`. - * 2. A call to an `EnvReadMethod` such as `System.getProperty` where a tainted value is used as an argument. - * The return value of such a method is then tainted. - */ -private predicate flagFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - DataFlow::localFlowStep(node1, node2) - or - exists(MethodAccess ma | ma.getMethod() = any(EnvReadMethod m) | - ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr() - ) - or - exists(MethodAccess ma | - ma.getMethod().hasName("parseBoolean") and - ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Boolean") - | - ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr() - ) -} - -/** Gets a guard that depends on a flag. */ -private Guard getAGuard() { - exists(DataFlow::Node source, DataFlow::Node sink | - isFlag(source) and - flagFlowStep*(source, sink) and - sink.asExpr() = result - ) -} - -/** Holds if `node` is guarded by a flag that suggests an intentionally insecure feature. */ +/** Holds if `node` is guarded by a flag that suggests an intentionally insecure use. */ private predicate isNodeGuardedByFlag(DataFlow::Node node) { - exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) | g = getAGuard()) + exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) | + g = getASecurityFeatureFlagGuard() or g = getAnUnsafeHostnameVerifierFlagGuard() + ) } from diff --git a/java/ql/src/semmle/code/java/security/SecurityFlag.qll b/java/ql/src/semmle/code/java/security/SecurityFlag.qll new file mode 100644 index 00000000000..cd6fe6ddd17 --- /dev/null +++ b/java/ql/src/semmle/code/java/security/SecurityFlag.qll @@ -0,0 +1,84 @@ +/** + * Provides utility predicates to spot variable names, parameter names, and string literals that suggest deliberately insecure settings. + */ + +import java +import semmle.code.java.controlflow.Guards +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.FlowSources + +/** + * A kind of flag that may indicate security expectations regarding the code it guards. + */ +abstract class FlagKind extends string { + bindingset[this] + FlagKind() { any() } + + /** + * Returns a flag name of this type. + */ + bindingset[result] + abstract string getAFlagName(); + + /** Gets a node representing a (likely) security flag. */ + DataFlow::Node getAFlag() { + exists(VarAccess v | v.getVariable().getName() = getAFlagName() | + result.asExpr() = v and v.getType() instanceof FlagType + ) + or + exists(StringLiteral s | s.getRepresentedString() = getAFlagName() | result.asExpr() = s) + or + exists(MethodAccess ma | ma.getMethod().getName() = getAFlagName() | + result.asExpr() = ma and + ma.getType() instanceof FlagType + ) + or + flagFlowStep*(getAFlag(), result) + } +} + +/** + * Flags suggesting an optional feature, perhaps deliberately insecure. + */ +class SecurityFeatureFlag extends FlagKind { + SecurityFeatureFlag() { this = "SecurityFeatureFlag" } + + bindingset[result] + override string getAFlagName() { result.regexpMatch("(?i).*(secure|(en|dis)able).*") } +} + +/** + * A flag has to either be of type `String`, `boolean` or `Boolean`. + */ +private class FlagType extends Type { + FlagType() { + this instanceof TypeString + or + this instanceof BooleanType + } +} + +/** + * Holds if there is local flow from `node1` to `node2` either due to standard data-flow steps or the + * following custom flow steps: + * 1. `Boolean.parseBoolean(taintedValue)` taints the return value of `parseBoolean`. + * 2. A call to an `EnvReadMethod` such as `System.getProperty` where a tainted value is used as an argument. + * The return value of such a method is then tainted. + */ +private predicate flagFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + DataFlow::localFlowStep(node1, node2) + or + exists(MethodAccess ma | ma.getMethod() = any(EnvReadMethod m) | + ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr() + ) + or + exists(MethodAccess ma | + ma.getMethod().hasName("parseBoolean") and + ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Boolean") + | + ma = node2.asExpr() and ma.getAnArgument() = node1.asExpr() + ) +} + +/** Gets a guard that represents a (likely) security feature-flag check. */ +Guard getASecurityFeatureFlagGuard() { result = any(SecurityFeatureFlag flag).getAFlag().asExpr() }