diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.ql b/java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.ql index 9e0835e2aac5..27736e4bf578 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.ql @@ -13,7 +13,7 @@ */ import java -import NonConstantTimeCheckOnSignatureQuery +import experimental.semmle.code.java.security.TimingAttack import DataFlow::PathGraph from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeCryptoComparisonConfig conf diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/SafeComparison.java b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/SafeComparison.java new file mode 100644 index 000000000000..cd2e580bd237 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/SafeComparison.java @@ -0,0 +1,5 @@ +private boolean safeComparison(String pwd, HttpServletRequest request) { + String password = request.getParameter("password"); + return MessageDigest.isEqual(password.getBytes(StandardCharsets.UTF_8), pwd.getBytes(StandardCharsets.UTF_8)); +} + diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.qhelp b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.qhelp new file mode 100644 index 000000000000..2c949602dc09 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.qhelp @@ -0,0 +1,54 @@ + + + + +

+ Timing Attack is based on the leakage of information of secret parameters by studying +how long it takes the system to respond to different inputs. + it can be circumvented by using a constant-time algorithm for checking the value of sensitive info, +more precisely, the comparison time should not depend on the content of the input. Otherwise the attacker gains +information that is indirectly leaked by the application. This information is then used for malicious purposes, +such as guessing the password of a user. +

+
+ + + +

+ Two types of countermeasures can be applied against timing attacks. The first one consists +in eliminating timing variations whereas the second renders these variations useless for an attacker. +The only absolute way to prevent timing attacks is to make the computation strictly constant time, +independent of the input. + + Use MessageDigest.isEqual() method to securely check the value of sensitive info. +If this method is used, then the calculation time depends only on the length of input byte arrays, +and does not depend on the contents of the arrays. + Unlike Arrays.equals() is a fail fast method, If the first byte is not equal, it will return immediately. +

+
+ +

+ The following example uses String.equals() which is a fail fast method for validating a password. +This method implements a non-constant-time algorithm: +

+ + +

+ The next example use a safe constant-time algorithm for validating a password: +

+ +
+ + +
  • + Wikipedia: + Timing attack. +
  • + +
  • + Java API Specification: + MessageDigest.isEqual() method +
  • +
    + +
    diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.ql b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.ql new file mode 100644 index 000000000000..8b764e965b3a --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.ql @@ -0,0 +1,40 @@ +/** + * @name Timing attack against sensitive info + * @description Use of a non-constant-time verification routine to check the value of an sensitive info, + * possibly allowing a timing attack to infer the info's expected value. + * @kind path-problem + * @problem.severity error + * @precision high + * @id java/timing-attack-against-sensitive-info + * @tags security + * external/cwe/cwe-208 + */ + +import java +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.TaintTracking +import DataFlow::PathGraph +import experimental.semmle.code.java.security.TimingAttack + +/** + * A configuration that tracks data flow from variable that may hold sensitive data + * to methods that compare data using a non-constant-time algorithm. + */ +class NonConstantTimeComparisonConfig extends TaintTracking::Configuration { + NonConstantTimeComparisonConfig() { this = "NonConstantTimeComparisonConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof SecretSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof NonConstantTimeComparisonSink } +} + +from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeComparisonConfig conf +where + conf.hasFlowPath(source, sink) and + ( + source.getNode().(SecretSource).includesUserInput() or + sink.getNode().(NonConstantTimeComparisonSink).includesUserInput() + ) and + not sink.getNode().(NonConstantTimeComparisonSink).includesIs() +select sink.getNode(), source, sink, "timing attack against $@ validation.", + source.getNode(), "time constant" diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/UnsafeComparison.java b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/UnsafeComparison.java new file mode 100644 index 000000000000..d4e2442f4e21 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/UnsafeComparison.java @@ -0,0 +1,5 @@ +private boolean UnsafeComparison(String pwd, HttpServletRequest request) { + String password = request.getParameter("password"); + return password.equals(pwd); +} + diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.ql b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.ql index 488b49684b2c..22a3eeeb6973 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.ql @@ -14,7 +14,7 @@ */ import java -import NonConstantTimeCheckOnSignatureQuery +import experimental.semmle.code.java.security.TimingAttack import DataFlow::PathGraph from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeCryptoComparisonConfig conf diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCheckOnSignatureQuery.qll b/java/ql/src/experimental/semmle/code/java/security/TimingAttack.qll similarity index 80% rename from java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCheckOnSignatureQuery.qll rename to java/ql/src/experimental/semmle/code/java/security/TimingAttack.qll index c90d16a6681f..f977192b25d5 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCheckOnSignatureQuery.qll +++ b/java/ql/src/experimental/semmle/code/java/security/TimingAttack.qll @@ -2,12 +2,55 @@ * Provides classes and predicates for queries that detect timing attacks. */ +import java import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.TaintTracking2 +import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.DataFlow3 +import semmle.code.java.dataflow.DataFlow2 import semmle.code.java.dataflow.FlowSources +/** A string for `match` that identifies strings that look like they represent secret data. */ +private string suspicious() { + result = + [ + "%password", "%passwd", "%pwd%", "%refresh%token%", "%secret%token", "%secret%key", + "%passcode", "%passphrase", "%secret%", "%userpass%", "%digest%", "%signature%" + ] +} + +/** + * A string for `match` that identifies strings that look like they represent secret data that is + * hashed or encrypted. + */ +private string nonSuspicious() { + result = "%hashed%" or + result = "%encrypted%" or + result = "%crypt%" or + result in ["%md5%", "%md2%", "%sha%"] +} + +/** A variable that may hold nonsensitive information, judging by its name. * */ +class NonSensitiveExpr extends Expr { + NonSensitiveExpr() { + exists(Variable v | this = v.getAnAccess() | + v.getName().toLowerCase().matches(nonSuspicious()) + ) + } +} + +/** A variable that may hold sensitive information, judging by its name. * */ +class CredentialExpr extends Expr { + CredentialExpr() { + exists(Variable v | this = v.getAnAccess() | + v.getName().toLowerCase().matches(suspicious()) and + not v.getName().toLowerCase().matches(nonSuspicious()) + not v.isFinal() + ) + } +} + /** A method call that produces cryptographic result. */ abstract private class ProduceCryptoCall extends MethodAccess { Expr output; @@ -228,6 +271,20 @@ private class UserInputInComparisonConfig extends TaintTracking2::Configuration } } +private class UserInputIs extends TaintTracking2::Configuration { + UserInputIs() { this = "UserInputIs" } + + override predicate isSource(DataFlow::Node source) { source instanceof InSecretSource } + + override predicate isSink(DataFlow::Node sink) { + exists(NonConstantTimeEqualsCall call | + sink.asExpr() = [call.getAnArgument(), call.getQualifier()] + ) + or + exists(NonConstantTimeComparisonCall call | sink.asExpr() = call.getAnArgument()) + } +} + /** Holds if `expr` looks like a constant. */ private predicate looksLikeConstant(Expr expr) { expr.isCompileTimeConstant() @@ -308,6 +365,30 @@ class NonConstantTimeComparisonSink extends DataFlow::Node { config.hasFlowTo(DataFlow2::exprNode(anotherParameter)) ) } + + predicate includesIs() { + exists(UserInputIs config | + config.hasFlowTo(DataFlow2::exprNode(anotherParameter)) + ) + } +} + +/** A data flow source of the secret obtained. */ +class SecretSource extends DataFlow::Node { + CredentialExpr secret; + + SecretSource() { secret = this.asExpr() } + + /** Holds if the secret was deliverd by remote user. */ + predicate includesUserInput() { + exists(UserInputSecretConfig config | config.hasFlowTo(DataFlow2::exprNode(secret))) + } +} + +class InSecretSource extends DataFlow::Node { + NonSensitiveExpr insecret; + + InSecretSource() { insecret = this.asExpr() } } /** @@ -321,3 +402,14 @@ class NonConstantTimeCryptoComparisonConfig extends TaintTracking::Configuration override predicate isSink(DataFlow::Node sink) { sink instanceof NonConstantTimeComparisonSink } } + +/** + * A config that tracks data flow from remote user input to Variable that hold sensitive info + */ +class UserInputSecretConfig extends TaintTracking2::Configuration { + UserInputSecretConfig() { this = "UserInputSecretConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof CredentialExpr } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSensitiveInfo/Test.java b/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSensitiveInfo/Test.java new file mode 100644 index 000000000000..d7a0346f38e2 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSensitiveInfo/Test.java @@ -0,0 +1,17 @@ +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.lang.String; +import javax.servlet.http.HttpServletRequest; + +public class Test { + private boolean UnsafeComparison(String pwd, HttpServletRequest request) { + String password = request.getParameter("password"); + return password.equals(pwd); + } + + private boolean safeComparison(String pwd, HttpServletRequest request) { + String password = request.getParameter("password"); + return MessageDigest.isEqual(password.getBytes(StandardCharsets.UTF_8), pwd.getBytes(StandardCharsets.UTF_8)); + } + +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.expected b/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.expected new file mode 100644 index 000000000000..f0f65cdffc4c --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.expected @@ -0,0 +1,8 @@ +edges +nodes +| Test.java:10:16:10:23 | password | semmle.label | password | +| Test.java:10:32:10:34 | pwd | semmle.label | pwd | +subpaths +#select +| Test.java:10:16:10:23 | password | Test.java:10:16:10:23 | password | Test.java:10:16:10:23 | password | Possible timing attack against $@ validation. | Test.java:10:16:10:23 | password | +| Test.java:10:32:10:34 | pwd | Test.java:10:32:10:34 | pwd | Test.java:10:32:10:34 | pwd | Possible timing attack against $@ validation. | Test.java:10:32:10:34 | pwd | diff --git a/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.qlref b/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.qlref new file mode 100644 index 000000000000..7c3931e93dde --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSensitiveInfo/TimingAttackAgainstSensitiveInfo.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-208/TimingAttackAgainstSensitiveInfo/PossibleTimingAttackAgainstSensitiveInfo.ql