Skip to content

Commit 8e6d227

Browse files
artem-smotrakovartem-smotrakov
committed
More sinks for java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCryptoComparison.ql
1 parent dfa3b52 commit 8e6d227

File tree

3 files changed

+188
-54
lines changed

3 files changed

+188
-54
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCryptoComparison.ql

Lines changed: 97 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
* @precision high
99
* @id java/non-constant-time-crypto-comparison
1010
* @tags security
11-
* external/cwe/cwe-385
1211
* external/cwe/cwe-208
1312
*/
1413

@@ -17,53 +16,121 @@ import semmle.code.java.dataflow.TaintTracking
1716
import DataFlow::PathGraph
1817

1918
/**
20-
* A method that returns the result of a cryptographic operation
21-
* such as encryption, decryption, signing, etc.
19+
* A source that produces a MAC.
2220
*/
23-
private class ReturnCryptoOperationResultMethod extends Method {
24-
ReturnCryptoOperationResultMethod() {
25-
getDeclaringType().hasQualifiedName("javax.crypto", ["Mac", "Cipher"]) and
26-
hasName("doFinal")
27-
or
28-
getDeclaringType().hasQualifiedName("java.security", "Signature") and
29-
hasName("sign")
21+
private class MacSource extends DataFlow::Node {
22+
MacSource() {
23+
exists(MethodAccess ma, Method m | m = ma.getMethod() |
24+
m.hasQualifiedName("javax.crypto", "Mac", "doFinal") and
25+
(
26+
m.getReturnType() instanceof Array and ma = this.asExpr()
27+
or
28+
m.getParameterType(0) instanceof Array and ma.getArgument(0) = this.asExpr()
29+
)
30+
)
3031
}
3132
}
3233

3334
/**
34-
* A configuration that tracks data flow from cryptographic operations
35-
* to methods that compare data using a non-constant time algorithm.
35+
* A source that produces a signature.
3636
*/
37-
private class NonConstantTimeCryptoComparisonConfig extends TaintTracking::Configuration {
38-
NonConstantTimeCryptoComparisonConfig() { this = "NonConstantTimeCryptoComparisonConfig" }
37+
private class SignatureSource extends DataFlow::Node {
38+
SignatureSource() {
39+
exists(MethodAccess ma, Method m | m = ma.getMethod() |
40+
m.hasQualifiedName("java.security", "Signature", "sign") and
41+
(
42+
m.getReturnType() instanceof Array and ma = this.asExpr()
43+
or
44+
m.getParameterType(0) instanceof Array and ma.getArgument(0) = this.asExpr()
45+
)
46+
)
47+
}
48+
}
3949

40-
override predicate isSource(DataFlow::Node source) {
41-
exists(MethodAccess ma | ma.getMethod() instanceof ReturnCryptoOperationResultMethod |
42-
ma = source.asExpr()
50+
/**
51+
* A source that produces a ciphertext.
52+
*/
53+
private class CiphertextSource extends DataFlow::Node {
54+
CiphertextSource() {
55+
exists(MethodAccess ma, Method m | m = ma.getMethod() |
56+
m.hasQualifiedName("javax.crypto", "Cipher", "doFinal") and
57+
(
58+
m.getReturnType() instanceof Array and ma = this.asExpr()
59+
or
60+
m.getParameterType([0, 3]) instanceof Array and ma.getArgument([0, 3]) = this.asExpr()
61+
or
62+
m.getParameterType(1).(RefType).hasQualifiedName("java.nio", "ByteBuffer") and
63+
ma.getArgument(1) = this.asExpr()
64+
)
4365
)
4466
}
67+
}
4568

46-
override predicate isSink(DataFlow::Node sink) {
69+
/**
70+
* A sink that compares input using a non-constant time algorithm.
71+
*/
72+
private class KnownNonConstantTimeComparisonSink extends DataFlow::Node {
73+
KnownNonConstantTimeComparisonSink() {
4774
exists(MethodAccess ma, Method m | m = ma.getMethod() |
48-
m.getDeclaringType() instanceof TypeString and
49-
m.hasName(["equals", "contentEquals", "equalsIgnoreCase"]) and
50-
sink.asExpr() = [ma.getQualifier(), ma.getAnArgument()]
75+
m.hasQualifiedName("java.lang", "String", ["equals", "contentEquals", "equalsIgnoreCase"]) and
76+
this.asExpr() = [ma.getQualifier(), ma.getAnArgument()]
77+
or
78+
m.hasQualifiedName("java.nio", "ByteBuffer", ["equals", "compareTo"]) and
79+
this.asExpr() = [ma.getQualifier(), ma.getAnArgument()]
5180
or
52-
m.getDeclaringType().hasQualifiedName("java.util", "Arrays") and
53-
m.hasName(["equals", "deepEquals"]) and
54-
ma.getAnArgument() = sink.asExpr()
81+
m.hasQualifiedName("java.util", "Arrays", ["equals", "deepEquals"]) and
82+
ma.getAnArgument() = this.asExpr()
5583
or
56-
m.getDeclaringType().hasQualifiedName("java.util", "Objects") and
57-
m.hasName("deepEquals") and
58-
ma.getAnArgument() = sink.asExpr()
84+
m.hasQualifiedName("java.util", "Objects", "deepEquals") and
85+
ma.getAnArgument() = this.asExpr()
5986
or
60-
m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "StringUtils") and
61-
m.hasName(["equals", "equalsAny", "equalsAnyIgnoreCase", "equalsIgnoreCase"]) and
62-
ma.getAnArgument() = sink.asExpr()
87+
m.hasQualifiedName("org.apache.commons.lang3", "StringUtils",
88+
["equals", "equalsAny", "equalsAnyIgnoreCase", "equalsIgnoreCase"]) and
89+
ma.getAnArgument() = this.asExpr()
6390
)
6491
}
6592
}
6693

94+
/**
95+
* Holds if `fromNode` to `toNode` is a dataflow step
96+
* that converts a `ByteBuffer` to a byte array and vice versa.
97+
*/
98+
private predicate convertByteBufferStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
99+
exists(MethodAccess ma, Method m | m = ma.getMethod() |
100+
m.hasQualifiedName("java.nio", "ByteBuffer", "array") and
101+
ma.getQualifier() = fromNode.asExpr() and
102+
ma = toNode.asExpr()
103+
or
104+
m.hasQualifiedName("java.nio", "ByteBuffer", "wrap") and
105+
ma.getAnArgument() = fromNode.asExpr() and
106+
ma = toNode.asExpr()
107+
)
108+
}
109+
110+
/**
111+
* A configuration that tracks data flow from cryptographic operations
112+
* to methods that compare data using a non-constant time algorithm.
113+
*/
114+
private class NonConstantTimeCryptoComparisonConfig extends TaintTracking::Configuration {
115+
NonConstantTimeCryptoComparisonConfig() { this = "NonConstantTimeCryptoComparisonConfig" }
116+
117+
override predicate isSource(DataFlow::Node source) {
118+
source instanceof MacSource
119+
or
120+
source instanceof SignatureSource
121+
or
122+
source instanceof CiphertextSource
123+
}
124+
125+
override predicate isSink(DataFlow::Node sink) {
126+
sink instanceof KnownNonConstantTimeComparisonSink
127+
}
128+
129+
override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
130+
convertByteBufferStep(fromNode, toNode)
131+
}
132+
}
133+
67134
from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeCryptoComparisonConfig conf
68135
where conf.hasFlowPath(source, sink)
69136
select sink.getNode(), source, sink,

‎java/ql/test/experimental/query-tests/security/CWE-208/NonConstantTimeCryptoComparison.expected

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,41 @@
11
edges
2-
| NonConstantTimeCryptoComparison.java:15:28:15:44 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:16:43:16:51 | actualMac |
3-
| NonConstantTimeCryptoComparison.java:23:28:23:44 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:24:58:24:66 | actualMac : byte[] |
4-
| NonConstantTimeCryptoComparison.java:24:58:24:66 | actualMac : byte[] | NonConstantTimeCryptoComparison.java:24:53:24:67 | cast(...) |
2+
| NonConstantTimeCryptoComparison.java:16:28:16:44 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:17:43:17:51 | actualMac |
3+
| NonConstantTimeCryptoComparison.java:23:28:23:44 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:24:84:24:92 | actualMac : byte[] |
4+
| NonConstantTimeCryptoComparison.java:24:84:24:92 | actualMac : byte[] | NonConstantTimeCryptoComparison.java:24:66:24:93 | castToObjectArray(...) |
5+
| NonConstantTimeCryptoComparison.java:31:21:31:29 | actualMac : byte[] | NonConstantTimeCryptoComparison.java:32:43:32:51 | actualMac |
56
| NonConstantTimeCryptoComparison.java:47:28:47:40 | sign(...) : byte[] | NonConstantTimeCryptoComparison.java:48:40:48:48 | signature |
6-
| NonConstantTimeCryptoComparison.java:64:22:64:46 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:65:45:65:47 | tag |
7+
| NonConstantTimeCryptoComparison.java:56:21:56:29 | signature : byte[] | NonConstantTimeCryptoComparison.java:57:40:57:48 | signature |
8+
| NonConstantTimeCryptoComparison.java:73:22:73:46 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:74:45:74:47 | tag |
9+
| NonConstantTimeCryptoComparison.java:83:24:83:26 | tag : byte[] | NonConstantTimeCryptoComparison.java:84:40:84:42 | tag |
10+
| NonConstantTimeCryptoComparison.java:93:52:93:54 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:94:40:94:50 | array(...) |
11+
| NonConstantTimeCryptoComparison.java:103:52:103:54 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:104:49:104:51 | tag |
712
nodes
8-
| NonConstantTimeCryptoComparison.java:15:28:15:44 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] |
9-
| NonConstantTimeCryptoComparison.java:16:43:16:51 | actualMac | semmle.label | actualMac |
13+
| NonConstantTimeCryptoComparison.java:16:28:16:44 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] |
14+
| NonConstantTimeCryptoComparison.java:17:43:17:51 | actualMac | semmle.label | actualMac |
1015
| NonConstantTimeCryptoComparison.java:23:28:23:44 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] |
11-
| NonConstantTimeCryptoComparison.java:24:53:24:67 | cast(...) | semmle.label | cast(...) |
12-
| NonConstantTimeCryptoComparison.java:24:58:24:66 | actualMac : byte[] | semmle.label | actualMac : byte[] |
16+
| NonConstantTimeCryptoComparison.java:24:66:24:93 | castToObjectArray(...) | semmle.label | castToObjectArray(...) |
17+
| NonConstantTimeCryptoComparison.java:24:84:24:92 | actualMac : byte[] | semmle.label | actualMac : byte[] |
18+
| NonConstantTimeCryptoComparison.java:31:21:31:29 | actualMac : byte[] | semmle.label | actualMac : byte[] |
19+
| NonConstantTimeCryptoComparison.java:32:43:32:51 | actualMac | semmle.label | actualMac |
1320
| NonConstantTimeCryptoComparison.java:47:28:47:40 | sign(...) : byte[] | semmle.label | sign(...) : byte[] |
1421
| NonConstantTimeCryptoComparison.java:48:40:48:48 | signature | semmle.label | signature |
15-
| NonConstantTimeCryptoComparison.java:64:22:64:46 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] |
16-
| NonConstantTimeCryptoComparison.java:65:45:65:47 | tag | semmle.label | tag |
22+
| NonConstantTimeCryptoComparison.java:56:21:56:29 | signature : byte[] | semmle.label | signature : byte[] |
23+
| NonConstantTimeCryptoComparison.java:57:40:57:48 | signature | semmle.label | signature |
24+
| NonConstantTimeCryptoComparison.java:73:22:73:46 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] |
25+
| NonConstantTimeCryptoComparison.java:74:45:74:47 | tag | semmle.label | tag |
26+
| NonConstantTimeCryptoComparison.java:83:24:83:26 | tag : byte[] | semmle.label | tag : byte[] |
27+
| NonConstantTimeCryptoComparison.java:84:40:84:42 | tag | semmle.label | tag |
28+
| NonConstantTimeCryptoComparison.java:93:52:93:54 | tag : ByteBuffer | semmle.label | tag : ByteBuffer |
29+
| NonConstantTimeCryptoComparison.java:94:40:94:50 | array(...) | semmle.label | array(...) |
30+
| NonConstantTimeCryptoComparison.java:103:52:103:54 | tag : ByteBuffer | semmle.label | tag : ByteBuffer |
31+
| NonConstantTimeCryptoComparison.java:104:49:104:51 | tag | semmle.label | tag |
1732
#select
18-
| NonConstantTimeCryptoComparison.java:16:43:16:51 | actualMac | NonConstantTimeCryptoComparison.java:15:28:15:44 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:16:43:16:51 | actualMac | Using a non-constant time algorithm for comparing results of a cryptographic operation. |
19-
| NonConstantTimeCryptoComparison.java:24:53:24:67 | cast(...) | NonConstantTimeCryptoComparison.java:23:28:23:44 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:24:53:24:67 | cast(...) | Using a non-constant time algorithm for comparing results of a cryptographic operation. |
33+
| NonConstantTimeCryptoComparison.java:17:43:17:51 | actualMac | NonConstantTimeCryptoComparison.java:16:28:16:44 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:17:43:17:51 | actualMac | Using a non-constant time algorithm for comparing results of a cryptographic operation. |
34+
| NonConstantTimeCryptoComparison.java:24:66:24:93 | castToObjectArray(...) | NonConstantTimeCryptoComparison.java:23:28:23:44 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:24:66:24:93 | castToObjectArray(...) | Using a non-constant time algorithm for comparing results of a cryptographic operation. |
35+
| NonConstantTimeCryptoComparison.java:32:43:32:51 | actualMac | NonConstantTimeCryptoComparison.java:31:21:31:29 | actualMac : byte[] | NonConstantTimeCryptoComparison.java:32:43:32:51 | actualMac | Using a non-constant time algorithm for comparing results of a cryptographic operation. |
2036
| NonConstantTimeCryptoComparison.java:48:40:48:48 | signature | NonConstantTimeCryptoComparison.java:47:28:47:40 | sign(...) : byte[] | NonConstantTimeCryptoComparison.java:48:40:48:48 | signature | Using a non-constant time algorithm for comparing results of a cryptographic operation. |
21-
| NonConstantTimeCryptoComparison.java:65:45:65:47 | tag | NonConstantTimeCryptoComparison.java:64:22:64:46 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:65:45:65:47 | tag | Using a non-constant time algorithm for comparing results of a cryptographic operation. |
37+
| NonConstantTimeCryptoComparison.java:57:40:57:48 | signature | NonConstantTimeCryptoComparison.java:56:21:56:29 | signature : byte[] | NonConstantTimeCryptoComparison.java:57:40:57:48 | signature | Using a non-constant time algorithm for comparing results of a cryptographic operation. |
38+
| NonConstantTimeCryptoComparison.java:74:45:74:47 | tag | NonConstantTimeCryptoComparison.java:73:22:73:46 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:74:45:74:47 | tag | Using a non-constant time algorithm for comparing results of a cryptographic operation. |
39+
| NonConstantTimeCryptoComparison.java:84:40:84:42 | tag | NonConstantTimeCryptoComparison.java:83:24:83:26 | tag : byte[] | NonConstantTimeCryptoComparison.java:84:40:84:42 | tag | Using a non-constant time algorithm for comparing results of a cryptographic operation. |
40+
| NonConstantTimeCryptoComparison.java:94:40:94:50 | array(...) | NonConstantTimeCryptoComparison.java:93:52:93:54 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:94:40:94:50 | array(...) | Using a non-constant time algorithm for comparing results of a cryptographic operation. |
41+
| NonConstantTimeCryptoComparison.java:104:49:104:51 | tag | NonConstantTimeCryptoComparison.java:103:52:103:54 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:104:49:104:51 | tag | Using a non-constant time algorithm for comparing results of a cryptographic operation. |

‎java/ql/test/experimental/query-tests/security/CWE-208/NonConstantTimeCryptoComparison.java

Lines changed: 58 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import java.nio.ByteBuffer;
12
import java.security.Key;
23
import java.security.MessageDigest;
34
import java.security.PrivateKey;
@@ -10,26 +11,25 @@
1011
public class NonConstantTimeCryptoComparison {
1112

1213
// BAD: compare MACs using a non-constant time method
13-
public boolean unsafeMacCheck(byte[] expectedMac, byte[] data) throws Exception {
14+
public boolean unsafeMacCheckWithArrayEquals(byte[] expectedMac, byte[] data) throws Exception {
1415
Mac mac = Mac.getInstance("HmacSHA256");
1516
byte[] actualMac = mac.doFinal(data);
1617
return Arrays.equals(expectedMac, actualMac);
1718
}
1819

19-
2020
// BAD: compare MACs using a non-constant time method
2121
public boolean unsafeMacCheckWithArraysDeepEquals(byte[] expectedMac, byte[] data) throws Exception {
2222
Mac mac = Mac.getInstance("HmacSHA256");
2323
byte[] actualMac = mac.doFinal(data);
24-
return Arrays.deepEquals(cast(expectedMac), cast(actualMac));
24+
return Arrays.deepEquals(castToObjectArray(expectedMac), castToObjectArray(actualMac));
2525
}
2626

27-
private static Object[] cast(byte[] array) {
28-
Object[] result = new Object[array.length];
29-
for (int i = 0; i < array.length; i++) {
30-
result[i] = array[i];
31-
}
32-
return result;
27+
// BAD: compare MACs using a non-constant time method
28+
public boolean unsafeMacCheckWithDoFinalWithOutputArray(byte[] expectedMac, byte[] data) throws Exception {
29+
Mac mac = Mac.getInstance("HmacSHA256");
30+
byte[] actualMac = new byte[256];
31+
mac.doFinal(actualMac, 0);
32+
return Arrays.equals(expectedMac, actualMac);
3333
}
3434

3535
// GOOD: compare MACs using a constant time method
@@ -48,6 +48,15 @@ public boolean unsafeCheckSignatures(byte[] expected, byte[] data, PrivateKey ke
4848
return Arrays.equals(expected, signature);
4949
}
5050

51+
// BAD: compare signatures using a non-constant time method
52+
public boolean unsafeCheckSignaturesWithOutputArray(byte[] expected, byte[] data, PrivateKey key) throws Exception {
53+
Signature engine = Signature.getInstance("SHA256withRSA");
54+
engine.initSign(key);
55+
byte[] signature = new byte[1024];
56+
engine.sign(signature, 0, 1024);
57+
return Arrays.equals(expected, signature);
58+
}
59+
5160
// GOOD: compare signatures using a constant time method
5261
public boolean saferCheckSignatures(byte[] expected, byte[] data, PrivateKey key) throws Exception {
5362
Signature engine = Signature.getInstance("SHA256withRSA");
@@ -58,19 +67,57 @@ public boolean saferCheckSignatures(byte[] expected, byte[] data, PrivateKey key
5867
}
5968

6069
// BAD: compare ciphertexts using a non-constant time method
61-
public boolean unsafeCheckCustomMac(byte[] expected, byte[] plaintext, Key key) throws Exception {
70+
public boolean unsafeCheckCiphertext(byte[] expected, byte[] plaintext, Key key) throws Exception {
6271
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
6372
cipher.init(Cipher.ENCRYPT_MODE, key);
6473
byte[] tag = cipher.doFinal(plaintext);
6574
return Objects.deepEquals(expected, tag);
6675
}
6776

77+
// BAD: compare ciphertexts using a non-constant time method
78+
public boolean unsafeCheckCiphertextWithOutputArray(byte[] expected, byte[] plaintext, Key key) throws Exception {
79+
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
80+
cipher.init(Cipher.ENCRYPT_MODE, key);
81+
cipher.update(plaintext);
82+
byte[] tag = new byte[1024];
83+
cipher.doFinal(tag, 0);
84+
return Arrays.equals(expected, tag);
85+
}
86+
87+
// BAD: compare ciphertexts using a non-constant time method
88+
public boolean unsafeCheckCiphertextWithByteBuffer(byte[] expected, byte[] plaintext, Key key) throws Exception {
89+
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
90+
cipher.init(Cipher.ENCRYPT_MODE, key);
91+
cipher.update(plaintext);
92+
ByteBuffer tag = ByteBuffer.wrap(new byte[1024]);
93+
cipher.doFinal(ByteBuffer.wrap(plaintext), tag);
94+
return Arrays.equals(expected, tag.array());
95+
}
96+
97+
// BAD: compare ciphertexts using a non-constant time method
98+
public boolean unsafeCheckCiphertextWithByteBufferEquals(byte[] expected, byte[] plaintext, Key key) throws Exception {
99+
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
100+
cipher.init(Cipher.ENCRYPT_MODE, key);
101+
cipher.update(plaintext);
102+
ByteBuffer tag = ByteBuffer.wrap(new byte[1024]);
103+
cipher.doFinal(ByteBuffer.wrap(plaintext), tag);
104+
return ByteBuffer.wrap(expected).equals(tag);
105+
}
106+
68107
// GOOD: compare ciphertexts using a constant time method
69-
public boolean saferCheckCustomMac(byte[] expected, byte[] plaintext, Key key) throws Exception {
108+
public boolean saferCheckCiphertext(byte[] expected, byte[] plaintext, Key key) throws Exception {
70109
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
71110
cipher.init(Cipher.ENCRYPT_MODE, key);
72111
byte[] tag = cipher.doFinal(plaintext);
73112
return MessageDigest.isEqual(expected, tag);
74113
}
114+
115+
private static Object[] castToObjectArray(byte[] array) {
116+
Object[] result = new Object[array.length];
117+
for (int i = 0; i < array.length; i++) {
118+
result[i] = array[i];
119+
}
120+
return result;
121+
}
75122

76123
}

0 commit comments

Comments
 (0)