diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/NotConstantTimeCryptoComparison.qhelp b/java/ql/src/experimental/Security/CWE/CWE-208/NotConstantTimeCryptoComparison.qhelp index 37558f5c7d4..ae0b54d12f7 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-208/NotConstantTimeCryptoComparison.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-208/NotConstantTimeCryptoComparison.qhelp @@ -3,7 +3,7 @@

-When comparing results of cryptographic operations, such as MAC or cryptographic hash, +When comparing results of cryptographic operations, such as MAC or digital signature, a constant time algorithm should be used. In other words, the comparison time should not depend on the content of the input. Otherwise, an attacker may be able to implement a timing attack. A successful timing attack may result in leaking secrets or authentication bypass. @@ -20,15 +20,15 @@ and does not depend on the contents of the arrays.

-The following example uses Arrays.equals() method for comparing cryptographic hashes. +The following example uses Arrays.equals() method for comparing MAC. This method implements a not-constant time algorithm:

- +

-The next example example uses a safe not-constant time algorithm for comparing cryptographic hashes: +The next example example uses a safe not-constant time algorithm for comparing MAC:

- + diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/NotConstantTimeCryptoComparison.ql b/java/ql/src/experimental/Security/CWE/CWE-208/NotConstantTimeCryptoComparison.ql index 0096cc67427..8251c210331 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-208/NotConstantTimeCryptoComparison.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-208/NotConstantTimeCryptoComparison.ql @@ -26,9 +26,6 @@ private class ReturnCryptoOperatinoResultMethod extends Method { or getDeclaringType().hasQualifiedName("java.security", "Signature") and hasName("sign") - or - getDeclaringType().hasQualifiedName("java.security", "MessageDigest") and - hasName("digest") } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/SafeCryptoHashComparison.java b/java/ql/src/experimental/Security/CWE/CWE-208/SafeCryptoHashComparison.java deleted file mode 100644 index 563069ff654..00000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-208/SafeCryptoHashComparison.java +++ /dev/null @@ -1,5 +0,0 @@ -public boolean checkHash(byte[] expectedHash, byte[] data) throws Exception { - MessageDigest md = MessageDigest.getInstance("SHA-256"); - byte[] actualHash = md.digest(data); - return MessageDigest.isEqual(expectedHash, actualHash); -} \ No newline at end of file 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..f2cc5f95ef8 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/SafeMacComparison.java @@ -0,0 +1,6 @@ +public boolean check(byte[] expected, byte[] data, SecretKey key) throws Exception { + Mac mac = Mac.getInstance("HmacSHA256"); + mac.init(new SecretKeySpec(key.getEncoded(), "HmacSHA256")); + byte[] actual = mac.doFinal(data); + return MessageDigest.isEqual(expected, actual); +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-208/UnsafeCryptoHashComparison.java b/java/ql/src/experimental/Security/CWE/CWE-208/UnsafeCryptoHashComparison.java deleted file mode 100644 index c145cc0f3c2..00000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-208/UnsafeCryptoHashComparison.java +++ /dev/null @@ -1,5 +0,0 @@ -public boolean checkHash(byte[] expectedHash, byte[] data) throws Exception { - MessageDigest md = MessageDigest.getInstance("SHA-256"); - byte[] actualHash = md.digest(data); - return Arrays.equals(expectedHash, actualHash); -} \ No newline at end of file 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..ec5f423aa4b --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-208/UnsafeMacComparison.java @@ -0,0 +1,6 @@ +public boolean check(byte[] expected, byte[] data, SecretKey key) throws Exception { + Mac mac = Mac.getInstance("HmacSHA256"); + mac.init(new SecretKeySpec(key.getEncoded(), "HmacSHA256")); + byte[] actual = mac.doFinal(data); + return Arrays.equals(expected, actual); +} \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCryptoComparison.expected b/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCryptoComparison.expected index 23083ff7d18..e2efbbe1a59 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCryptoComparison.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCryptoComparison.expected @@ -1,19 +1,15 @@ edges | NotConstantTimeCryptoComparison.java:14:28:14:44 | doFinal(...) : byte[] | NotConstantTimeCryptoComparison.java:15:43:15:51 | actualMac | -| NotConstantTimeCryptoComparison.java:28:36:28:50 | digest(...) : byte[] | NotConstantTimeCryptoComparison.java:29:16:29:21 | actual | -| NotConstantTimeCryptoComparison.java:44:28:44:40 | sign(...) : byte[] | NotConstantTimeCryptoComparison.java:45:40:45:48 | signature | -| NotConstantTimeCryptoComparison.java:61:22:61:46 | doFinal(...) : byte[] | NotConstantTimeCryptoComparison.java:62:40:62:42 | tag | +| NotConstantTimeCryptoComparison.java:30:28:30:40 | sign(...) : byte[] | NotConstantTimeCryptoComparison.java:31:40:31:48 | signature | +| NotConstantTimeCryptoComparison.java:47:22:47:46 | doFinal(...) : byte[] | NotConstantTimeCryptoComparison.java:48:40:48:42 | tag | nodes | NotConstantTimeCryptoComparison.java:14:28:14:44 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] | | NotConstantTimeCryptoComparison.java:15:43:15:51 | actualMac | semmle.label | actualMac | -| NotConstantTimeCryptoComparison.java:28:36:28:50 | digest(...) : byte[] | semmle.label | digest(...) : byte[] | -| NotConstantTimeCryptoComparison.java:29:16:29:21 | actual | semmle.label | actual | -| NotConstantTimeCryptoComparison.java:44:28:44:40 | sign(...) : byte[] | semmle.label | sign(...) : byte[] | -| NotConstantTimeCryptoComparison.java:45:40:45:48 | signature | semmle.label | signature | -| NotConstantTimeCryptoComparison.java:61:22:61:46 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] | -| NotConstantTimeCryptoComparison.java:62:40:62:42 | tag | semmle.label | tag | +| NotConstantTimeCryptoComparison.java:30:28:30:40 | sign(...) : byte[] | semmle.label | sign(...) : byte[] | +| NotConstantTimeCryptoComparison.java:31:40:31:48 | signature | semmle.label | signature | +| NotConstantTimeCryptoComparison.java:47:22:47:46 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] | +| NotConstantTimeCryptoComparison.java:48:40:48:42 | tag | semmle.label | tag | #select | NotConstantTimeCryptoComparison.java:15:43:15:51 | actualMac | NotConstantTimeCryptoComparison.java:14:28:14:44 | doFinal(...) : byte[] | NotConstantTimeCryptoComparison.java:15:43:15:51 | actualMac | Using a not-constant time algorithm for comparison results of a cryptographic operation. | -| NotConstantTimeCryptoComparison.java:29:16:29:21 | actual | NotConstantTimeCryptoComparison.java:28:36:28:50 | digest(...) : byte[] | NotConstantTimeCryptoComparison.java:29:16:29:21 | actual | Using a not-constant time algorithm for comparison results of a cryptographic operation. | -| NotConstantTimeCryptoComparison.java:45:40:45:48 | signature | NotConstantTimeCryptoComparison.java:44:28:44:40 | sign(...) : byte[] | NotConstantTimeCryptoComparison.java:45:40:45:48 | signature | Using a not-constant time algorithm for comparison results of a cryptographic operation. | -| NotConstantTimeCryptoComparison.java:62:40:62:42 | tag | NotConstantTimeCryptoComparison.java:61:22:61:46 | doFinal(...) : byte[] | NotConstantTimeCryptoComparison.java:62:40:62:42 | tag | Using a not-constant time algorithm for comparison results of a cryptographic operation. | +| NotConstantTimeCryptoComparison.java:31:40:31:48 | signature | NotConstantTimeCryptoComparison.java:30:28:30:40 | sign(...) : byte[] | NotConstantTimeCryptoComparison.java:31:40:31:48 | signature | Using a not-constant time algorithm for comparison results of a cryptographic operation. | +| NotConstantTimeCryptoComparison.java:48:40:48:42 | tag | NotConstantTimeCryptoComparison.java:47:22:47:46 | doFinal(...) : byte[] | NotConstantTimeCryptoComparison.java:48:40:48:42 | tag | Using a not-constant time algorithm for comparison results of a cryptographic operation. | diff --git a/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCryptoComparison.java b/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCryptoComparison.java index 995c342d776..0e43103fffd 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCryptoComparison.java +++ b/java/ql/test/experimental/query-tests/security/CWE-208/NotConstantTimeCryptoComparison.java @@ -22,20 +22,6 @@ public class NotConstantTimeCryptoComparison { return MessageDigest.isEqual(expectedMac, actualMac); } - // BAD: compare hashes using a not-constant time method - public boolean unsafeCheckMessageDigest(String expectedHash, byte[] data) throws Exception { - MessageDigest md = MessageDigest.getInstance("SHA-256"); - String actual = new String(md.digest(data)); - return actual.equals(expectedHash); - } - - // GOOD: compare hashes using a constant time method - public boolean saferCheckMessageDigest(byte[] expected, byte[] data) throws Exception { - MessageDigest md = MessageDigest.getInstance("SHA-256"); - byte[] actual = md.digest(data); - return MessageDigest.isEqual(expected, actual); - } - // 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");