diff --git a/java/change-notes/2021-06-22-more-steps-for-bytebuffer-inputstream.md b/java/change-notes/2021-06-22-more-steps-for-bytebuffer-inputstream.md new file mode 100644 index 00000000000..8ffaf6926ea --- /dev/null +++ b/java/change-notes/2021-06-22-more-steps-for-bytebuffer-inputstream.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* Added more taint propagation steps for `InputStream` and `ByteBuffer`. diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCheckOnSignatureQuery.qll b/java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCheckOnSignatureQuery.qll new file mode 100644 index 00000000000..4c35b8e2940 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCheckOnSignatureQuery.qll @@ -0,0 +1,323 @@ +/** + * Provides classes and predicates for queries that detect timing attacks. + */ + +import semmle.code.java.controlflow.Guards +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.TaintTracking2 +import semmle.code.java.dataflow.DataFlow3 +import semmle.code.java.dataflow.FlowSources + +/** A method call that produces cryptographic result. */ +abstract private class ProduceCryptoCall extends MethodAccess { + Expr output; + + /** Gets the result of cryptographic operation. */ + Expr output() { result = output } + + /** Gets a type of cryptographic operation such as MAC, signature or ciphertext. */ + abstract string getResultType(); +} + +/** A method call that produces a MAC. */ +private class ProduceMacCall extends ProduceCryptoCall { + ProduceMacCall() { + getMethod().getDeclaringType().hasQualifiedName("javax.crypto", "Mac") and + ( + getMethod().hasStringSignature(["doFinal()", "doFinal(byte[])"]) and this = output + or + getMethod().hasStringSignature("doFinal(byte[], int)") and getArgument(0) = output + ) + } + + override string getResultType() { result = "MAC" } +} + +/** A method call that produces a signature. */ +private class ProduceSignatureCall extends ProduceCryptoCall { + ProduceSignatureCall() { + getMethod().getDeclaringType().hasQualifiedName("java.security", "Signature") and + ( + getMethod().hasStringSignature("sign()") and this = output + or + getMethod().hasStringSignature("sign(byte[], int, int)") and getArgument(0) = output + ) + } + + override string getResultType() { result = "signature" } +} + +/** + * A config that tracks data flow from initializing a cipher for encryption + * to producing a ciphertext using this cipher. + */ +private class InitializeEncryptorConfig extends DataFlow3::Configuration { + InitializeEncryptorConfig() { this = "InitializeEncryptorConfig" } + + override predicate isSource(DataFlow::Node source) { + exists(MethodAccess ma | + ma.getMethod().hasQualifiedName("javax.crypto", "Cipher", "init") and + ma.getArgument(0).(VarAccess).getVariable().hasName("ENCRYPT_MODE") and + ma.getQualifier() = source.asExpr() + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + ma.getMethod().hasQualifiedName("javax.crypto", "Cipher", "doFinal") and + ma.getQualifier() = sink.asExpr() + ) + } +} + +/** A method call that produces a ciphertext. */ +private class ProduceCiphertextCall extends ProduceCryptoCall { + ProduceCiphertextCall() { + exists(Method m | m = this.getMethod() | + m.getDeclaringType().hasQualifiedName("javax.crypto", "Cipher") and + ( + m.hasStringSignature(["doFinal()", "doFinal(byte[])", "doFinal(byte[], int, int)"]) and + this = output + or + m.hasStringSignature("doFinal(byte[], int)") and getArgument(0) = output + or + m.hasStringSignature([ + "doFinal(byte[], int, int, byte[])", "doFinal(byte[], int, int, byte[], int)" + ]) and + getArgument(3) = output + or + m.hasStringSignature("doFinal(ByteBuffer, ByteBuffer)") and + getArgument(1) = output + ) + ) and + exists(InitializeEncryptorConfig config | + config.hasFlowTo(DataFlow3::exprNode(this.getQualifier())) + ) + } + + override string getResultType() { result = "ciphertext" } +} + +/** Holds if `fromNode` to `toNode` is a dataflow step that updates a cryptographic operation. */ +private predicate updateCryptoOperationStep(DataFlow2::Node fromNode, DataFlow2::Node toNode) { + exists(MethodAccess call, Method m | + m = call.getMethod() and + call.getQualifier() = toNode.asExpr() and + call.getArgument(0) = fromNode.asExpr() + | + m.hasQualifiedName("java.security", "Signature", "update") + or + m.hasQualifiedName("javax.crypto", ["Mac", "Cipher"], "update") + or + m.hasQualifiedName("javax.crypto", ["Mac", "Cipher"], "doFinal") and + not m.hasStringSignature("doFinal(byte[], int)") + ) +} + +/** Holds if `fromNode` to `toNode` is a dataflow step that creates a hash. */ +private predicate createMessageDigestStep(DataFlow2::Node fromNode, DataFlow2::Node toNode) { + exists(MethodAccess ma, Method m | m = ma.getMethod() | + m.getDeclaringType().hasQualifiedName("java.security", "MessageDigest") and + m.hasStringSignature("digest()") and + ma.getQualifier() = fromNode.asExpr() and + ma = toNode.asExpr() + ) + or + exists(MethodAccess ma, Method m | m = ma.getMethod() | + m.getDeclaringType().hasQualifiedName("java.security", "MessageDigest") and + m.hasStringSignature("digest(byte[], int, int)") and + ma.getQualifier() = fromNode.asExpr() and + ma.getArgument(0) = toNode.asExpr() + ) + or + exists(MethodAccess ma, Method m | m = ma.getMethod() | + m.getDeclaringType().hasQualifiedName("java.security", "MessageDigest") and + m.hasStringSignature("digest(byte[])") and + ma.getArgument(0) = fromNode.asExpr() and + ma = toNode.asExpr() + ) +} + +/** Holds if `fromNode` to `toNode` is a dataflow step that updates a hash. */ +private predicate updateMessageDigestStep(DataFlow2::Node fromNode, DataFlow2::Node toNode) { + exists(MethodAccess ma, Method m | m = ma.getMethod() | + m.hasQualifiedName("java.security", "MessageDigest", "update") and + ma.getArgument(0) = fromNode.asExpr() and + ma.getQualifier() = toNode.asExpr() + ) +} + +/** + * A config that tracks data flow from remote user input to a cryptographic operation + * such as cipher, MAC or signature. + */ +private class UserInputInCryptoOperationConfig extends TaintTracking2::Configuration { + UserInputInCryptoOperationConfig() { this = "UserInputInCryptoOperationConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { + exists(ProduceCryptoCall call | call.getQualifier() = sink.asExpr()) + } + + override predicate isAdditionalTaintStep(DataFlow2::Node fromNode, DataFlow2::Node toNode) { + updateCryptoOperationStep(fromNode, toNode) + or + createMessageDigestStep(fromNode, toNode) + or + updateMessageDigestStep(fromNode, toNode) + } +} + +/** A source that produces result of cryptographic operation. */ +class CryptoOperationSource extends DataFlow::Node { + ProduceCryptoCall call; + + CryptoOperationSource() { call.output() = this.asExpr() } + + /** Holds if remote user input was used in the cryptographic operation. */ + predicate includesUserInput() { + exists( + DataFlow2::PathNode source, DataFlow2::PathNode sink, UserInputInCryptoOperationConfig config + | + config.hasFlowPath(source, sink) + | + sink.getNode().asExpr() = call.getQualifier() + ) + } + + /** Gets a method call that produces cryptographic result. */ + ProduceCryptoCall getCall() { result = call } +} + +/** Methods that use a non-constant-time algorithm for comparing inputs. */ +private class NonConstantTimeEqualsCall extends MethodAccess { + NonConstantTimeEqualsCall() { + getMethod() + .hasQualifiedName("java.lang", "String", ["equals", "contentEquals", "equalsIgnoreCase"]) or + getMethod().hasQualifiedName("java.nio", "ByteBuffer", ["equals", "compareTo"]) + } +} + +/** A static method that uses a non-constant-time algorithm for comparing inputs. */ +private class NonConstantTimeComparisonCall extends StaticMethodAccess { + NonConstantTimeComparisonCall() { + getMethod().hasQualifiedName("java.util", "Arrays", ["equals", "deepEquals"]) or + getMethod().hasQualifiedName("java.util", "Objects", "deepEquals") or + getMethod() + .hasQualifiedName("org.apache.commons.lang3", "StringUtils", + ["equals", "equalsAny", "equalsAnyIgnoreCase", "equalsIgnoreCase"]) + } +} + +/** + * A config that tracks data flow from remote user input to methods + * that compare inputs using a non-constant-time algorithm. + */ +private class UserInputInComparisonConfig extends TaintTracking2::Configuration { + UserInputInComparisonConfig() { this = "UserInputInComparisonConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + 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() + or + expr.(VarAccess).getVariable().isFinal() and expr.getType() instanceof TypeString +} + +/** + * Holds if `firstObject` and `secondObject` are compared using a method + * that does not use a constant-time algorithm, for example, `String.equals()`. + */ +private predicate isNonConstantTimeEqualsCall(Expr firstObject, Expr secondObject) { + exists(NonConstantTimeEqualsCall call | + firstObject = call.getQualifier() and + secondObject = call.getAnArgument() + or + firstObject = call.getAnArgument() and + secondObject = call.getQualifier() + ) +} + +/** + * Holds if `firstInput` and `secondInput` are compared using a static method + * that does not use a constant-time algorithm, for example, `Arrays.equals()`. + */ +private predicate isNonConstantTimeComparisonCall(Expr firstInput, Expr secondInput) { + exists(NonConstantTimeComparisonCall call | + firstInput = call.getArgument(0) and secondInput = call.getArgument(1) + or + firstInput = call.getArgument(1) and secondInput = call.getArgument(0) + ) +} + +/** + * Holds if there is a fast-fail check while comparing `firstArray` and `secondArray`. + */ +private predicate existsFailFastCheck(Expr firstArray, Expr secondArray) { + exists( + Guard guard, EqualityTest eqTest, boolean branch, Stmt fastFailingStmt, + ArrayAccess firstArrayAccess, ArrayAccess secondArrayAccess + | + guard = eqTest and + // For `==` false branch is fail fast; for `!=` true branch is fail fast + branch = eqTest.polarity().booleanNot() and + ( + fastFailingStmt instanceof ReturnStmt or + fastFailingStmt instanceof BreakStmt or + fastFailingStmt instanceof ThrowStmt + ) and + guard.controls(fastFailingStmt.getBasicBlock(), branch) and + DataFlow::localExprFlow(firstArrayAccess, eqTest.getLeftOperand()) and + DataFlow::localExprFlow(secondArrayAccess, eqTest.getRightOperand()) + | + firstArrayAccess.getArray() = firstArray and secondArray = secondArrayAccess + or + secondArrayAccess.getArray() = firstArray and secondArray = firstArrayAccess + ) +} + +/** A sink that compares input using a non-constant-time algorithm. */ +class NonConstantTimeComparisonSink extends DataFlow::Node { + Expr anotherParameter; + + NonConstantTimeComparisonSink() { + ( + isNonConstantTimeEqualsCall(this.asExpr(), anotherParameter) + or + isNonConstantTimeComparisonCall(this.asExpr(), anotherParameter) + or + existsFailFastCheck(this.asExpr(), anotherParameter) + ) and + not looksLikeConstant(anotherParameter) + } + + /** Holds if remote user input was used in the comparison. */ + predicate includesUserInput() { + exists(UserInputInComparisonConfig config | + config.hasFlowTo(DataFlow2::exprNode(anotherParameter)) + ) + } +} + +/** + * A configuration that tracks data flow from cryptographic operations + * to methods that compare data using a non-constant-time algorithm. + */ +class NonConstantTimeCryptoComparisonConfig extends TaintTracking::Configuration { + NonConstantTimeCryptoComparisonConfig() { this = "NonConstantTimeCryptoComparisonConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof CryptoOperationSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof NonConstantTimeComparisonSink } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.qhelp b/java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.qhelp new file mode 100644 index 00000000000..aee0196686f --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.qhelp @@ -0,0 +1,4 @@ + + + + diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.ql b/java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.ql new file mode 100644 index 00000000000..9e0835e2aac --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.ql @@ -0,0 +1,22 @@ +/** + * @name Possible timing attack against signature validation + * @description When checking a signature over a message, a constant-time algorithm should be used. + * Otherwise, there is a risk of a timing attack that allows an attacker + * to forge a valid signature for an arbitrary message. For a successful attack, + * the attacker has to be able to send to the validation procedure both the message and the signature. + * @kind path-problem + * @problem.severity warning + * @precision medium + * @id java/possible-timing-attack-against-signature + * @tags security + * external/cwe/cwe-208 + */ + +import java +import NonConstantTimeCheckOnSignatureQuery +import DataFlow::PathGraph + +from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeCryptoComparisonConfig conf +where conf.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "Possible timing attack against $@ validation.", source, + source.getNode().(CryptoOperationSource).getCall().getResultType() diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/SafeMacComparison.java b/java/ql/src/experimental/Security/CWE/CWE-208/SafeMacComparison.java new file mode 100644 index 00000000000..fbdb6131a5c --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/SafeMacComparison.java @@ -0,0 +1,9 @@ +public boolean validate(HttpRequest request, SecretKey key) throws Exception { + byte[] message = getMessageFrom(request); + byte[] signature = getSignatureFrom(request); + + Mac mac = Mac.getInstance("HmacSHA256"); + mac.init(new SecretKeySpec(key.getEncoded(), "HmacSHA256")); + byte[] actual = mac.doFinal(message); + return MessageDigest.isEqual(signature, actual); +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.qhelp b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.qhelp new file mode 100644 index 00000000000..7815312414a --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.qhelp @@ -0,0 +1,55 @@ + + + + +

+A constant-time algorithm should be used for checking a MAC or a digital signature. +In other words, the comparison time should not depend on the content of the input. +Otherwise, an attacker may be able to forge a valid signature for an arbitrary message +by running a timing attack if they can send to the validation procedure +both the message and the signature. A successful attack can result in authentication bypass. +

+ + + +

+Use MessageDigest.isEqual() method to check MACs and signatures. +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. +

+ + + +

+The following example uses Arrays.equals() method for validating a MAC over a message. +This method implements a non-constant-time algorithm. +Both the message and the signature come from an untrusted HTTP request: +

+ + +

+The next example uses a safe constant-time algorithm for validating a MAC: +

+ + + + +
  • + Wikipedia: + Timing attack. +
    • +
  • + Coursera: + Timing attacks on MAC verification +
    • +
  • + NCC Group: + Time Trial: Racing Towards Practical Remote Timing Attacks +
    • +
  • + Java API Specification: + MessageDigest.isEqual() method +
    • +     + +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.ql b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.ql new file mode 100644 index 00000000000..488b49684b2 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.ql @@ -0,0 +1,28 @@ +/** + * @name Timing attack against signature validation + * @description When checking a signature over a message, a constant-time algorithm should be used. + * Otherwise, an attacker may be able to forge a valid signature for an arbitrary message + * by running a timing attack if they can send to the validation procedure + * both the message and the signature. + * A successful attack can result in authentication bypass. + * @kind path-problem + * @problem.severity error + * @precision high + * @id java/timing-attack-against-signature + * @tags security + * external/cwe/cwe-208 + */ + +import java +import NonConstantTimeCheckOnSignatureQuery +import DataFlow::PathGraph + +from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeCryptoComparisonConfig conf +where + conf.hasFlowPath(source, sink) and + ( + source.getNode().(CryptoOperationSource).includesUserInput() and + sink.getNode().(NonConstantTimeComparisonSink).includesUserInput() + ) +select sink.getNode(), source, sink, "Timing attack against $@ validation.", source, + source.getNode().(CryptoOperationSource).getCall().getResultType() diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/UnsafeMacComparison.java b/java/ql/src/experimental/Security/CWE/CWE-208/UnsafeMacComparison.java new file mode 100644 index 00000000000..1785ff2e7c6 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/UnsafeMacComparison.java @@ -0,0 +1,9 @@ +public boolean validate(HttpRequest request, SecretKey key) throws Exception { + byte[] message = getMessageFrom(request); + byte[] signature = getSignatureFrom(request); + + Mac mac = Mac.getInstance("HmacSHA256"); + mac.init(new SecretKeySpec(key.getEncoded(), "HmacSHA256")); + byte[] actual = mac.doFinal(message); + return Arrays.equals(signature, actual); +} \ No newline at end of file diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 3b49289d885..aae71ce6d9a 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -268,11 +268,15 @@ private predicate summaryModelCsv(string row) { // qualifier to arg "java.io;InputStream;true;read;(byte[]);;Argument[-1];Argument[0];taint", "java.io;InputStream;true;read;(byte[],int,int);;Argument[-1];Argument[0];taint", + "java.io;InputStream;true;readNBytes;(byte[],int,int);;Argument[-1];Argument[0];taint", + "java.io;InputStream;true;transferTo;(OutputStream);;Argument[-1];Argument[0];taint", "java.io;ByteArrayOutputStream;false;writeTo;;;Argument[-1];Argument[0];taint", "java.io;Reader;true;read;;;Argument[-1];Argument[0];taint", // qualifier to return "java.io;ByteArrayOutputStream;false;toByteArray;;;Argument[-1];ReturnValue;taint", "java.io;ByteArrayOutputStream;false;toString;;;Argument[-1];ReturnValue;taint", + "java.io;InputStream;true;readAllBytes;;;Argument[-1];ReturnValue;taint", + "java.io;InputStream;true;readNBytes;(int);;Argument[-1];ReturnValue;taint", "java.util;StringTokenizer;false;nextElement;();;Argument[-1];ReturnValue;taint", "java.util;StringTokenizer;false;nextToken;;;Argument[-1];ReturnValue;taint", "javax.xml.transform.sax;SAXSource;false;getInputSource;;;Argument[-1];ReturnValue;taint", @@ -283,10 +287,12 @@ private predicate summaryModelCsv(string row) { "java.net;URI;false;toAsciiString;;;Argument[-1];ReturnValue;taint", "java.io;File;false;toURI;;;Argument[-1];ReturnValue;taint", "java.io;File;false;toPath;;;Argument[-1];ReturnValue;taint", + "java.nio;ByteBuffer;false;array;();;Argument[-1];ReturnValue;taint", "java.nio.file;Path;false;toFile;;;Argument[-1];ReturnValue;taint", "java.io;BufferedReader;true;readLine;;;Argument[-1];ReturnValue;taint", "java.io;Reader;true;read;();;Argument[-1];ReturnValue;taint", // arg to return + "java.nio;ByteBuffer;false;wrap;(byte[]);;Argument[0];ReturnValue;taint", "java.util;Base64$Encoder;false;encode;(byte[]);;Argument[0];ReturnValue;taint", "java.util;Base64$Encoder;false;encode;(ByteBuffer);;Argument[0];ReturnValue;taint", "java.util;Base64$Encoder;false;encodeToString;(byte[]);;Argument[0];ReturnValue;taint", diff --git a/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCheckOnSignature/Test.expected b/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCheckOnSignature/Test.expected new file mode 100644 index 00000000000..b6d3211a64e --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCheckOnSignature/Test.expected @@ -0,0 +1,15 @@ +edges +| Test.java:14:28:14:44 | doFinal(...) : byte[] | Test.java:15:43:15:51 | actualMac | +| Test.java:30:28:30:40 | sign(...) : byte[] | Test.java:31:40:31:48 | signature | +| Test.java:47:22:47:46 | doFinal(...) : byte[] | Test.java:48:40:48:42 | tag | +nodes +| Test.java:14:28:14:44 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] | +| Test.java:15:43:15:51 | actualMac | semmle.label | actualMac | +| Test.java:30:28:30:40 | sign(...) : byte[] | semmle.label | sign(...) : byte[] | +| Test.java:31:40:31:48 | signature | semmle.label | signature | +| Test.java:47:22:47:46 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] | +| Test.java:48:40:48:42 | tag | semmle.label | tag | +#select +| Test.java:15:43:15:51 | actualMac | Test.java:14:28:14:44 | doFinal(...) : byte[] | Test.java:15:43:15:51 | actualMac | Possible timing attack against $@ validation. | Test.java:14:28:14:44 | doFinal(...) : byte[] | MAC | +| Test.java:31:40:31:48 | signature | Test.java:30:28:30:40 | sign(...) : byte[] | Test.java:31:40:31:48 | signature | Possible timing attack against $@ validation. | Test.java:30:28:30:40 | sign(...) : byte[] | signature | +| Test.java:48:40:48:42 | tag | Test.java:47:22:47:46 | doFinal(...) : byte[] | Test.java:48:40:48:42 | tag | Possible timing attack against $@ validation. | Test.java:47:22:47:46 | doFinal(...) : byte[] | ciphertext | diff --git a/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCheckOnSignature/Test.java b/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCheckOnSignature/Test.java new file mode 100644 index 00000000000..7a4433e485d --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCheckOnSignature/Test.java @@ -0,0 +1,59 @@ +import java.security.Key; +import java.security.MessageDigest; +import java.security.PrivateKey; +import java.security.Signature; +import java.util.Arrays; +import javax.crypto.Cipher; +import javax.crypto.Mac; + +public class Test { + + // BAD: compare MACs using a not-constant time method + public boolean unsafeMacCheck(byte[] expectedMac, byte[] data) throws Exception { + Mac mac = Mac.getInstance("HmacSHA256"); + byte[] actualMac = mac.doFinal(data); + return Arrays.equals(expectedMac, actualMac); + } + + // GOOD: compare MACs using a constant time method + public boolean saferMacCheck(byte[] expectedMac, byte[] data) throws Exception { + Mac mac = Mac.getInstance("HmacSHA256"); + byte[] actualMac = mac.doFinal(data); + return MessageDigest.isEqual(expectedMac, actualMac); + } + + // BAD: compare signatures using a not-constant time method + public boolean unsafeCheckSignatures(byte[] expected, byte[] data, PrivateKey key) throws Exception { + Signature engine = Signature.getInstance("SHA256withRSA"); + engine.initSign(key); + engine.update(data); + byte[] signature = engine.sign(); + return Arrays.equals(expected, signature); + } + + // GOOD: compare signatures using a constant time method + public boolean saferCheckSignatures(byte[] expected, byte[] data, PrivateKey key) throws Exception { + Signature engine = Signature.getInstance("SHA256withRSA"); + engine.initSign(key); + engine.update(data); + byte[] signature = engine.sign(); + return MessageDigest.isEqual(expected, signature); + } + + // BAD: compare ciphertexts using a not-constant time method + public boolean unsafeCheckCustomMac(byte[] expected, byte[] plaintext, Key key) throws Exception { + Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); + cipher.init(Cipher.ENCRYPT_MODE, key); + byte[] tag = cipher.doFinal(plaintext); + return Arrays.equals(expected, tag); + } + + // GOOD: compare ciphertexts using a constant time method + public boolean saferCheckCustomMac(byte[] expected, byte[] plaintext, Key key) throws Exception { + Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); + cipher.init(Cipher.ENCRYPT_MODE, key); + byte[] tag = cipher.doFinal(plaintext); + return MessageDigest.isEqual(expected, tag); + } + +} \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCheckOnSignature/Test.qlref b/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCheckOnSignature/Test.qlref new file mode 100644 index 00000000000..3a113b0fd71 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCheckOnSignature/Test.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.ql diff --git a/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSignagure/Test.expected b/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSignagure/Test.expected new file mode 100644 index 00000000000..805be02abf3 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSignagure/Test.expected @@ -0,0 +1,44 @@ +edges +| Test.java:21:32:21:48 | doFinal(...) : byte[] | Test.java:23:47:23:55 | actualMac | +| Test.java:34:25:34:33 | actualMac : byte[] | Test.java:36:47:36:55 | actualMac | +| Test.java:59:32:59:44 | sign(...) : byte[] | Test.java:61:44:61:52 | signature | +| Test.java:73:25:73:33 | signature : byte[] | Test.java:75:44:75:52 | signature | +| Test.java:99:26:99:45 | doFinal(...) : byte[] | Test.java:101:49:101:51 | tag | +| Test.java:116:28:116:30 | tag : byte[] | Test.java:118:44:118:46 | tag | +| Test.java:134:56:134:58 | tag : ByteBuffer | Test.java:136:44:136:46 | tag : ByteBuffer | +| Test.java:136:44:136:46 | tag : ByteBuffer | Test.java:136:44:136:54 | array(...) | +| Test.java:148:56:148:58 | tag : ByteBuffer | Test.java:150:53:150:55 | tag | +| Test.java:174:26:174:50 | doFinal(...) : byte[] | Test.java:176:44:176:46 | tag | +| Test.java:201:34:201:50 | doFinal(...) : byte[] | Test.java:204:26:204:36 | computedTag | +nodes +| Test.java:21:32:21:48 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] | +| Test.java:23:47:23:55 | actualMac | semmle.label | actualMac | +| Test.java:34:25:34:33 | actualMac : byte[] | semmle.label | actualMac : byte[] | +| Test.java:36:47:36:55 | actualMac | semmle.label | actualMac | +| Test.java:59:32:59:44 | sign(...) : byte[] | semmle.label | sign(...) : byte[] | +| Test.java:61:44:61:52 | signature | semmle.label | signature | +| Test.java:73:25:73:33 | signature : byte[] | semmle.label | signature : byte[] | +| Test.java:75:44:75:52 | signature | semmle.label | signature | +| Test.java:99:26:99:45 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] | +| Test.java:101:49:101:51 | tag | semmle.label | tag | +| Test.java:116:28:116:30 | tag : byte[] | semmle.label | tag : byte[] | +| Test.java:118:44:118:46 | tag | semmle.label | tag | +| Test.java:134:56:134:58 | tag : ByteBuffer | semmle.label | tag : ByteBuffer | +| Test.java:136:44:136:46 | tag : ByteBuffer | semmle.label | tag : ByteBuffer | +| Test.java:136:44:136:54 | array(...) | semmle.label | array(...) | +| Test.java:148:56:148:58 | tag : ByteBuffer | semmle.label | tag : ByteBuffer | +| Test.java:150:53:150:55 | tag | semmle.label | tag | +| Test.java:174:26:174:50 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] | +| Test.java:176:44:176:46 | tag | semmle.label | tag | +| Test.java:201:34:201:50 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] | +| Test.java:204:26:204:36 | computedTag | semmle.label | computedTag | +#select +| Test.java:23:47:23:55 | actualMac | Test.java:21:32:21:48 | doFinal(...) : byte[] | Test.java:23:47:23:55 | actualMac | Timing attack against $@ validation. | Test.java:21:32:21:48 | doFinal(...) : byte[] | MAC | +| Test.java:36:47:36:55 | actualMac | Test.java:34:25:34:33 | actualMac : byte[] | Test.java:36:47:36:55 | actualMac | Timing attack against $@ validation. | Test.java:34:25:34:33 | actualMac : byte[] | MAC | +| Test.java:61:44:61:52 | signature | Test.java:59:32:59:44 | sign(...) : byte[] | Test.java:61:44:61:52 | signature | Timing attack against $@ validation. | Test.java:59:32:59:44 | sign(...) : byte[] | signature | +| Test.java:75:44:75:52 | signature | Test.java:73:25:73:33 | signature : byte[] | Test.java:75:44:75:52 | signature | Timing attack against $@ validation. | Test.java:73:25:73:33 | signature : byte[] | signature | +| Test.java:101:49:101:51 | tag | Test.java:99:26:99:45 | doFinal(...) : byte[] | Test.java:101:49:101:51 | tag | Timing attack against $@ validation. | Test.java:99:26:99:45 | doFinal(...) : byte[] | ciphertext | +| Test.java:118:44:118:46 | tag | Test.java:116:28:116:30 | tag : byte[] | Test.java:118:44:118:46 | tag | Timing attack against $@ validation. | Test.java:116:28:116:30 | tag : byte[] | ciphertext | +| Test.java:136:44:136:54 | array(...) | Test.java:134:56:134:58 | tag : ByteBuffer | Test.java:136:44:136:54 | array(...) | Timing attack against $@ validation. | Test.java:134:56:134:58 | tag : ByteBuffer | ciphertext | +| Test.java:150:53:150:55 | tag | Test.java:148:56:148:58 | tag : ByteBuffer | Test.java:150:53:150:55 | tag | Timing attack against $@ validation. | Test.java:148:56:148:58 | tag : ByteBuffer | ciphertext | +| Test.java:176:44:176:46 | tag | Test.java:174:26:174:50 | doFinal(...) : byte[] | Test.java:176:44:176:46 | tag | Timing attack against $@ validation. | Test.java:174:26:174:50 | doFinal(...) : byte[] | ciphertext | diff --git a/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSignagure/Test.java b/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSignagure/Test.java new file mode 100644 index 00000000000..0755f1fe668 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSignagure/Test.java @@ -0,0 +1,236 @@ +import java.io.InputStream; +import java.net.Socket; +import java.nio.ByteBuffer; +import java.security.Key; +import java.security.MessageDigest; +import java.security.PrivateKey; +import java.security.Signature; +import java.util.Arrays; +import java.util.Objects; +import javax.crypto.Cipher; +import javax.crypto.Mac; + +public class Test { + + // BAD: compare MACs using a non-constant-time method + public boolean unsafeMacCheckWithArrayEquals(Socket socket) throws Exception { + try (InputStream is = socket.getInputStream()) { + Mac mac = Mac.getInstance("HmacSHA256"); + byte[] data = new byte[1024]; + is.read(data); + byte[] actualMac = mac.doFinal(data); + byte[] expectedMac = is.readNBytes(32); + return Arrays.equals(expectedMac, actualMac); + } + } + + // BAD: compare MACs using a non-constant-time method + public boolean unsafeMacCheckWithDoFinalWithOutputArray(Socket socket) throws Exception { + try (InputStream is = socket.getInputStream()) { + byte[] data = is.readNBytes(100); + Mac mac = Mac.getInstance("HmacSHA256"); + byte[] actualMac = new byte[256]; + mac.update(data); + mac.doFinal(actualMac, 0); + byte[] expectedMac = socket.getInputStream().readNBytes(256); + return Arrays.equals(expectedMac, actualMac); + } + } + + // GOOD: compare MACs using a constant-time method + public boolean saferMacCheck(Socket socket) throws Exception { + try (InputStream is = socket.getInputStream()) { + Mac mac = Mac.getInstance("HmacSHA256"); + byte[] data = new byte[1024]; + is.read(data); + byte[] actualMac = mac.doFinal(data); + byte[] expectedMac = is.readNBytes(32); + return MessageDigest.isEqual(expectedMac, actualMac); + } + } + + // BAD: compare signatures using a non-constant-time method + public boolean unsafeCheckSignatures(Socket socket, PrivateKey key) throws Exception { + try (InputStream is = socket.getInputStream()) { + Signature engine = Signature.getInstance("SHA256withRSA"); + engine.initSign(key); + byte[] data = socket.getInputStream().readAllBytes(); + engine.update(data); + byte[] signature = engine.sign(); + byte[] expected = is.readNBytes(256); + return Arrays.equals(expected, signature); + } + } + + // BAD: compare signatures using a non-constant-time method + public boolean unsafeCheckSignaturesWithOutputArray(Socket socket, PrivateKey key) throws Exception { + try (InputStream is = socket.getInputStream()) { + Signature engine = Signature.getInstance("SHA256withRSA"); + engine.initSign(key); + byte[] data = socket.getInputStream().readAllBytes(); + engine.update(data); + byte[] signature = new byte[1024]; + engine.sign(signature, 0, 1024); + byte[] expected = is.readNBytes(256); + return Arrays.equals(expected, signature); + } + } + + // GOOD: compare signatures using a constant-time method + public boolean saferCheckSignatures(Socket socket, PrivateKey key) throws Exception { + try (InputStream is = socket.getInputStream()) { + Signature engine = Signature.getInstance("SHA256withRSA"); + engine.initSign(key); + byte[] data = socket.getInputStream().readAllBytes(); + engine.update(data); + byte[] signature = engine.sign(); + byte[] expected = is.readNBytes(256); + return MessageDigest.isEqual(expected, signature); + } + } + + // BAD: compare ciphertexts (custom MAC) using a non-constant-time method + public boolean unsafeCheckCiphertext(Socket socket, Key key) throws Exception { + try (InputStream is = socket.getInputStream()) { + byte[] plaintext = is.readNBytes(100); + byte[] hash = MessageDigest.getInstance("SHA-256").digest(plaintext); + Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); + cipher.init(Cipher.ENCRYPT_MODE, key); + byte[] tag = cipher.doFinal(hash); + byte[] expected = socket.getInputStream().readAllBytes(); + return Objects.deepEquals(expected, tag); + } + } + + // BAD: compare ciphertexts (custom MAC) using a non-constant-time method + public boolean unsafeCheckCiphertextWithOutputArray(Socket socket, Key key) throws Exception { + try (InputStream is = socket.getInputStream()) { + byte[] plaintext = socket.getInputStream().readAllBytes(); + MessageDigest md = MessageDigest.getInstance("SHA-512"); + md.update(plaintext); + byte[] hash = md.digest(); + Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); + cipher.init(Cipher.ENCRYPT_MODE, key); + cipher.update(hash); + byte[] tag = new byte[1024]; + cipher.doFinal(tag, 0); + byte[] expected = is.readNBytes(32); + return Arrays.equals(expected, tag); + } + } + + // BAD: compare ciphertexts (custom MAC) using a non-constant-time method + public boolean unsafeCheckCiphertextWithByteBuffer(Socket socket, Key key) throws Exception { + try (InputStream is = socket.getInputStream()) { + byte[] plaintext = is.readNBytes(300); + MessageDigest md = MessageDigest.getInstance("SHA-512"); + md.update(plaintext); + byte[] hash = new byte[1024]; + md.digest(hash, 0, hash.length); + Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); + cipher.init(Cipher.ENCRYPT_MODE, key); + cipher.update(hash); + ByteBuffer tag = ByteBuffer.wrap(new byte[1024]); + cipher.doFinal(ByteBuffer.wrap(plaintext), tag); + byte[] expected = socket.getInputStream().readNBytes(1024); + return Arrays.equals(expected, tag.array()); + } + } + + // BAD: compare ciphertexts (custom MAC) using a non-constant-time method + public boolean unsafeCheckCiphertextWithByteBufferEquals(Socket socket, Key key) throws Exception { + try (InputStream is = socket.getInputStream()) { + Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); + cipher.init(Cipher.ENCRYPT_MODE, key); + byte[] plaintext = socket.getInputStream().readAllBytes(); + cipher.update(plaintext); + ByteBuffer tag = ByteBuffer.wrap(new byte[1024]); + cipher.doFinal(ByteBuffer.wrap(plaintext), tag); + byte[] expected = is.readNBytes(32); + return ByteBuffer.wrap(expected).equals(tag); + } + } + + // GOOD: compare ciphertexts (custom MAC) using a constant-time method + public boolean saferCheckCiphertext(Socket socket, Key key) throws Exception { + try (InputStream is = socket.getInputStream()) { + byte[] plaintext = is.readNBytes(200); + Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); + cipher.init(Cipher.ENCRYPT_MODE, key); + byte[] hash = MessageDigest.getInstance("SHA-256").digest(plaintext); + byte[] tag = cipher.doFinal(hash); + byte[] expected = socket.getInputStream().readAllBytes(); + return MessageDigest.isEqual(expected, tag); + } + } + + // GOOD: compare ciphertexts using a constant-time method, but no user input + // but NonConstantTimeCheckOnSignature.ql still detects it + public boolean noUserInputWhenCheckingCiphertext(Socket socket, Key key) throws Exception { + try (InputStream is = socket.getInputStream()) { + byte[] plaintext = is.readNBytes(100); + Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); + cipher.init(Cipher.ENCRYPT_MODE, key); + byte[] tag = cipher.doFinal(plaintext); + byte[] expected = is.readNBytes(32); + return Arrays.equals(expected, tag); + } + } + + // GOOD: compare MAC with constant using a constant-time method + public boolean compareMacWithConstant(Socket socket) throws Exception { + try (InputStream is = socket.getInputStream()) { + Mac mac = Mac.getInstance("HmacSHA256"); + byte[] data = new byte[1024]; + socket.getInputStream().read(data); + byte[] actualMac = mac.doFinal(data); + return "constant".equals(new String(actualMac)); + } + } + + // BAD: compare MAC using a non-constant-time loop + public boolean unsafeMacCheckWithLoop(Socket socket) throws Exception { + try (InputStream is = socket.getInputStream()) { + byte[] data = new byte[256]; + byte[] tag = new byte[32]; + + is.read(data); + is.read(tag); + + Mac mac = Mac.getInstance("Hmac256"); + byte[] computedTag = mac.doFinal(data); + + for (int i = 0; i < computedTag.length; i++) { + byte a = computedTag[i]; + byte b = tag[i]; + if (a != b) { + return false; + } + } + + return true; + } + } + + // GOOD: compare MAC using a constant-time loop + public boolean safeMacCheckWithLoop(Socket socket) throws Exception { + try (InputStream is = socket.getInputStream()) { + byte[] data = new byte[256]; + byte[] tag = new byte[32]; + + is.read(data); + is.read(tag); + + Mac mac = Mac.getInstance("Hmac256"); + byte[] computedTag = mac.doFinal(data); + + int result = 0; + for (int i = 0; i < computedTag.length; i++) { + result |= computedTag[i] ^ tag[i]; + } + + return result == 0; + } + } + +} \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSignagure/Test.qlref b/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSignagure/Test.qlref new file mode 100644 index 00000000000..0ed112d4d1f --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-208/TimingAttackAgainstSignagure/Test.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-208/TimingAttackAgainstSignature.ql \ No newline at end of file