Skip to content

Commit 1f2a9cd

Browse files
artem-smotrakovartem-smotrakov
committed
Added taint propagation steps for hashes in NonConstantTimeCryptoComparison.ql
1 parent c96d939 commit 1f2a9cd

File tree

3 files changed

+98
-48
lines changed

3 files changed

+98
-48
lines changed

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

Lines changed: 54 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,55 @@ private class ProduceCiphertextCall extends ProduceCryptoCall {
7575
}
7676
}
7777

78+
/** Holds if `fromNode` to `toNode` is a dataflow step that updates a cryptographic operation. */
79+
private predicate updateCryptoOperationStep(DataFlow2::Node fromNode, DataFlow2::Node toNode) {
80+
exists(MethodAccess call, Method m |
81+
m = call.getMethod() and
82+
call.getQualifier() = toNode.asExpr() and
83+
call.getArgument(0) = fromNode.asExpr()
84+
|
85+
m.hasQualifiedName("java.security", "Signature", "update")
86+
or
87+
m.hasQualifiedName("javax.crypto", ["Mac", "Cipher"], "update")
88+
or
89+
m.hasQualifiedName("javax.crypto", ["Mac", "Cipher"], "doFinal") and
90+
not m.hasStringSignature("doFinal(byte[], int)")
91+
)
92+
}
93+
94+
/** Holds if `fromNode` to `toNode` is a dataflow step that creates a hash. */
95+
private predicate createMessageDigestStep(DataFlow2::Node fromNode, DataFlow2::Node toNode) {
96+
exists(MethodAccess ma, Method m | m = ma.getMethod() |
97+
m.getDeclaringType().hasQualifiedName("java.security", "MessageDigest") and
98+
m.hasStringSignature("digest()") and
99+
ma.getQualifier() = fromNode.asExpr() and
100+
ma = toNode.asExpr()
101+
)
102+
or
103+
exists(MethodAccess ma, Method m | m = ma.getMethod() |
104+
m.getDeclaringType().hasQualifiedName("java.security", "MessageDigest") and
105+
m.hasStringSignature("digest(byte[], int, int)") and
106+
ma.getQualifier() = fromNode.asExpr() and
107+
ma.getArgument(0) = toNode.asExpr()
108+
)
109+
or
110+
exists(MethodAccess ma, Method m | m = ma.getMethod() |
111+
m.getDeclaringType().hasQualifiedName("java.security", "MessageDigest") and
112+
m.hasStringSignature("digest(byte[])") and
113+
ma.getArgument(0) = fromNode.asExpr() and
114+
ma = toNode.asExpr()
115+
)
116+
}
117+
118+
/** Holds if `fromNode` to `toNode` is a dataflow step that updates a hash. */
119+
private predicate updateMessageDigestStep(DataFlow2::Node fromNode, DataFlow2::Node toNode) {
120+
exists(MethodAccess ma, Method m | m = ma.getMethod() |
121+
m.hasQualifiedName("java.security", "MessageDigest", "update") and
122+
ma.getArgument(0) = fromNode.asExpr() and
123+
ma.getQualifier() = toNode.asExpr()
124+
)
125+
}
126+
78127
/**
79128
* A config that tracks data flow from remote user input to a cryptographic operation
80129
* such as cipher, MAC or signature.
@@ -88,20 +137,12 @@ private class UserInputInCryptoOperationConfig extends TaintTracking2::Configura
88137
exists(ProduceCryptoCall call | call.getQualifier() = sink.asExpr())
89138
}
90139

91-
/** Holds if `fromNode` to `toNode` is a dataflow step that updates a cryptographic operation. */
92140
override predicate isAdditionalTaintStep(DataFlow2::Node fromNode, DataFlow2::Node toNode) {
93-
exists(MethodAccess call, Method m |
94-
m = call.getMethod() and
95-
call.getQualifier() = toNode.asExpr() and
96-
call.getArgument(0) = fromNode.asExpr()
97-
|
98-
m.hasQualifiedName("java.security", "Signature", "update")
99-
or
100-
m.hasQualifiedName("javax.crypto", ["Mac", "Cipher"], "update")
101-
or
102-
m.hasQualifiedName("javax.crypto", ["Mac", "Cipher"], "doFinal") and
103-
not m.hasStringSignature("doFinal(byte[], int)")
104-
)
141+
updateCryptoOperationStep(fromNode, toNode)
142+
or
143+
createMessageDigestStep(fromNode, toNode)
144+
or
145+
updateMessageDigestStep(fromNode, toNode)
105146
}
106147
}
107148

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

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,13 @@ edges
55
| NonConstantTimeCryptoComparison.java:37:21:37:29 | actualMac : byte[] | NonConstantTimeCryptoComparison.java:39:43:39:51 | actualMac |
66
| NonConstantTimeCryptoComparison.java:57:28:57:40 | sign(...) : byte[] | NonConstantTimeCryptoComparison.java:58:40:58:48 | signature |
77
| NonConstantTimeCryptoComparison.java:68:21:68:29 | signature : byte[] | NonConstantTimeCryptoComparison.java:69:40:69:48 | signature |
8-
| NonConstantTimeCryptoComparison.java:86:22:86:46 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:88:45:88:47 | tag |
9-
| NonConstantTimeCryptoComparison.java:98:24:98:26 | tag : byte[] | NonConstantTimeCryptoComparison.java:99:40:99:42 | tag |
10-
| NonConstantTimeCryptoComparison.java:108:52:108:54 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:110:40:110:42 | tag : ByteBuffer |
11-
| NonConstantTimeCryptoComparison.java:110:40:110:42 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:110:40:110:50 | array(...) |
12-
| NonConstantTimeCryptoComparison.java:120:52:120:54 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:121:49:121:51 | tag |
13-
| NonConstantTimeCryptoComparison.java:137:22:137:46 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:138:40:138:42 | tag |
14-
| NonConstantTimeCryptoComparison.java:168:34:168:50 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:171:26:171:36 | computedTag |
8+
| NonConstantTimeCryptoComparison.java:87:22:87:41 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:89:45:89:47 | tag |
9+
| NonConstantTimeCryptoComparison.java:102:24:102:26 | tag : byte[] | NonConstantTimeCryptoComparison.java:103:40:103:42 | tag |
10+
| NonConstantTimeCryptoComparison.java:116:52:116:54 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:118:40:118:42 | tag : ByteBuffer |
11+
| NonConstantTimeCryptoComparison.java:118:40:118:42 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:118:40:118:50 | array(...) |
12+
| NonConstantTimeCryptoComparison.java:128:52:128:54 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:129:49:129:51 | tag |
13+
| NonConstantTimeCryptoComparison.java:146:22:146:46 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:147:40:147:42 | tag |
14+
| NonConstantTimeCryptoComparison.java:177:34:177:50 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:180:26:180:36 | computedTag |
1515
nodes
1616
| NonConstantTimeCryptoComparison.java:20:28:20:44 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] |
1717
| NonConstantTimeCryptoComparison.java:21:43:21:51 | actualMac | semmle.label | actualMac |
@@ -24,27 +24,27 @@ nodes
2424
| NonConstantTimeCryptoComparison.java:58:40:58:48 | signature | semmle.label | signature |
2525
| NonConstantTimeCryptoComparison.java:68:21:68:29 | signature : byte[] | semmle.label | signature : byte[] |
2626
| NonConstantTimeCryptoComparison.java:69:40:69:48 | signature | semmle.label | signature |
27-
| NonConstantTimeCryptoComparison.java:86:22:86:46 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] |
28-
| NonConstantTimeCryptoComparison.java:88:45:88:47 | tag | semmle.label | tag |
29-
| NonConstantTimeCryptoComparison.java:98:24:98:26 | tag : byte[] | semmle.label | tag : byte[] |
30-
| NonConstantTimeCryptoComparison.java:99:40:99:42 | tag | semmle.label | tag |
31-
| NonConstantTimeCryptoComparison.java:108:52:108:54 | tag : ByteBuffer | semmle.label | tag : ByteBuffer |
32-
| NonConstantTimeCryptoComparison.java:110:40:110:42 | tag : ByteBuffer | semmle.label | tag : ByteBuffer |
33-
| NonConstantTimeCryptoComparison.java:110:40:110:50 | array(...) | semmle.label | array(...) |
34-
| NonConstantTimeCryptoComparison.java:120:52:120:54 | tag : ByteBuffer | semmle.label | tag : ByteBuffer |
35-
| NonConstantTimeCryptoComparison.java:121:49:121:51 | tag | semmle.label | tag |
36-
| NonConstantTimeCryptoComparison.java:137:22:137:46 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] |
37-
| NonConstantTimeCryptoComparison.java:138:40:138:42 | tag | semmle.label | tag |
38-
| NonConstantTimeCryptoComparison.java:168:34:168:50 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] |
39-
| NonConstantTimeCryptoComparison.java:171:26:171:36 | computedTag | semmle.label | computedTag |
27+
| NonConstantTimeCryptoComparison.java:87:22:87:41 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] |
28+
| NonConstantTimeCryptoComparison.java:89:45:89:47 | tag | semmle.label | tag |
29+
| NonConstantTimeCryptoComparison.java:102:24:102:26 | tag : byte[] | semmle.label | tag : byte[] |
30+
| NonConstantTimeCryptoComparison.java:103:40:103:42 | tag | semmle.label | tag |
31+
| NonConstantTimeCryptoComparison.java:116:52:116:54 | tag : ByteBuffer | semmle.label | tag : ByteBuffer |
32+
| NonConstantTimeCryptoComparison.java:118:40:118:42 | tag : ByteBuffer | semmle.label | tag : ByteBuffer |
33+
| NonConstantTimeCryptoComparison.java:118:40:118:50 | array(...) | semmle.label | array(...) |
34+
| NonConstantTimeCryptoComparison.java:128:52:128:54 | tag : ByteBuffer | semmle.label | tag : ByteBuffer |
35+
| NonConstantTimeCryptoComparison.java:129:49:129:51 | tag | semmle.label | tag |
36+
| NonConstantTimeCryptoComparison.java:146:22:146:46 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] |
37+
| NonConstantTimeCryptoComparison.java:147:40:147:42 | tag | semmle.label | tag |
38+
| NonConstantTimeCryptoComparison.java:177:34:177:50 | doFinal(...) : byte[] | semmle.label | doFinal(...) : byte[] |
39+
| NonConstantTimeCryptoComparison.java:180:26:180:36 | computedTag | semmle.label | computedTag |
4040
#select
4141
| NonConstantTimeCryptoComparison.java:21:43:21:51 | actualMac | NonConstantTimeCryptoComparison.java:20:28:20:44 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:21:43:21:51 | actualMac | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
4242
| NonConstantTimeCryptoComparison.java:30:66:30:93 | castToObjectArray(...) | NonConstantTimeCryptoComparison.java:29:28:29:40 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:30:66:30:93 | castToObjectArray(...) | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
4343
| NonConstantTimeCryptoComparison.java:39:43:39:51 | actualMac | NonConstantTimeCryptoComparison.java:37:21:37:29 | actualMac : byte[] | NonConstantTimeCryptoComparison.java:39:43:39:51 | actualMac | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
4444
| NonConstantTimeCryptoComparison.java:58:40:58:48 | signature | NonConstantTimeCryptoComparison.java:57:28:57:40 | sign(...) : byte[] | NonConstantTimeCryptoComparison.java:58:40:58:48 | signature | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
4545
| NonConstantTimeCryptoComparison.java:69:40:69:48 | signature | NonConstantTimeCryptoComparison.java:68:21:68:29 | signature : byte[] | NonConstantTimeCryptoComparison.java:69:40:69:48 | signature | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
46-
| NonConstantTimeCryptoComparison.java:88:45:88:47 | tag | NonConstantTimeCryptoComparison.java:86:22:86:46 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:88:45:88:47 | tag | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
47-
| NonConstantTimeCryptoComparison.java:99:40:99:42 | tag | NonConstantTimeCryptoComparison.java:98:24:98:26 | tag : byte[] | NonConstantTimeCryptoComparison.java:99:40:99:42 | tag | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
48-
| NonConstantTimeCryptoComparison.java:110:40:110:50 | array(...) | NonConstantTimeCryptoComparison.java:108:52:108:54 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:110:40:110:50 | array(...) | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
49-
| NonConstantTimeCryptoComparison.java:121:49:121:51 | tag | NonConstantTimeCryptoComparison.java:120:52:120:54 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:121:49:121:51 | tag | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
50-
| NonConstantTimeCryptoComparison.java:171:26:171:36 | computedTag | NonConstantTimeCryptoComparison.java:168:34:168:50 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:171:26:171:36 | computedTag | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
46+
| NonConstantTimeCryptoComparison.java:89:45:89:47 | tag | NonConstantTimeCryptoComparison.java:87:22:87:41 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:89:45:89:47 | tag | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
47+
| NonConstantTimeCryptoComparison.java:103:40:103:42 | tag | NonConstantTimeCryptoComparison.java:102:24:102:26 | tag : byte[] | NonConstantTimeCryptoComparison.java:103:40:103:42 | tag | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
48+
| NonConstantTimeCryptoComparison.java:118:40:118:50 | array(...) | NonConstantTimeCryptoComparison.java:116:52:116:54 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:118:40:118:50 | array(...) | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
49+
| NonConstantTimeCryptoComparison.java:129:49:129:51 | tag | NonConstantTimeCryptoComparison.java:128:52:128:54 | tag : ByteBuffer | NonConstantTimeCryptoComparison.java:129:49:129:51 | tag | Using a non-constant-time algorithm for comparing results of a cryptographic operation. |
50+
| NonConstantTimeCryptoComparison.java:180:26:180:36 | computedTag | NonConstantTimeCryptoComparison.java:177:34:177:50 | doFinal(...) : byte[] | NonConstantTimeCryptoComparison.java:180:26:180:36 | computedTag | 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: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -79,38 +79,46 @@ public boolean saferCheckSignatures(byte[] expected, Socket socket, PrivateKey k
7979
return MessageDigest.isEqual(expected, signature);
8080
}
8181

82-
// BAD: compare ciphertexts using a non-constant-time method
82+
// BAD: compare ciphertexts (custom MAC) using a non-constant-time method
8383
public boolean unsafeCheckCiphertext(Socket socket, byte[] plaintext, Key key) throws Exception {
84+
byte[] hash = MessageDigest.getInstance("SHA-256").digest(plaintext);
8485
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
8586
cipher.init(Cipher.ENCRYPT_MODE, key);
86-
byte[] tag = cipher.doFinal(plaintext);
87+
byte[] tag = cipher.doFinal(hash);
8788
byte[] expected = socket.getInputStream().readAllBytes();
8889
return Objects.deepEquals(expected, tag);
8990
}
9091

91-
// BAD: compare ciphertexts using a non-constant-time method
92+
// BAD: compare ciphertexts (custom MAC) using a non-constant-time method
9293
public boolean unsafeCheckCiphertextWithOutputArray(byte[] expected, Socket socket, Key key) throws Exception {
94+
byte[] plaintext = socket.getInputStream().readAllBytes();
95+
MessageDigest md = MessageDigest.getInstance("SHA-512");
96+
md.update(plaintext);
97+
byte[] hash = md.digest();
9398
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
9499
cipher.init(Cipher.ENCRYPT_MODE, key);
95-
byte[] plaintext = socket.getInputStream().readAllBytes();
96-
cipher.update(plaintext);
100+
cipher.update(hash);
97101
byte[] tag = new byte[1024];
98102
cipher.doFinal(tag, 0);
99103
return Arrays.equals(expected, tag);
100104
}
101105

102-
// BAD: compare ciphertexts using a non-constant-time method
106+
// BAD: compare ciphertexts (custom MAC) using a non-constant-time method
103107
public boolean unsafeCheckCiphertextWithByteBuffer(Socket socket, byte[] plaintext, Key key) throws Exception {
108+
MessageDigest md = MessageDigest.getInstance("SHA-512");
109+
md.update(plaintext);
110+
byte[] hash = new byte[1024];
111+
md.digest(hash, 0, hash.length);
104112
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
105113
cipher.init(Cipher.ENCRYPT_MODE, key);
106-
cipher.update(plaintext);
114+
cipher.update(hash);
107115
ByteBuffer tag = ByteBuffer.wrap(new byte[1024]);
108116
cipher.doFinal(ByteBuffer.wrap(plaintext), tag);
109117
byte[] expected = socket.getInputStream().readNBytes(1024);
110118
return Arrays.equals(expected, tag.array());
111119
}
112120

113-
// BAD: compare ciphertexts using a non-constant-time method
121+
// BAD: compare ciphertexts (custom MAC) using a non-constant-time method
114122
public boolean unsafeCheckCiphertextWithByteBufferEquals(byte[] expected, Socket socket, Key key) throws Exception {
115123
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
116124
cipher.init(Cipher.ENCRYPT_MODE, key);
@@ -121,11 +129,12 @@ public boolean unsafeCheckCiphertextWithByteBufferEquals(byte[] expected, Socket
121129
return ByteBuffer.wrap(expected).equals(tag);
122130
}
123131

124-
// GOOD: compare ciphertexts using a constant-time method
132+
// GOOD: compare ciphertexts (custom MAC) using a constant-time method
125133
public boolean saferCheckCiphertext(Socket socket, byte[] plaintext, Key key) throws Exception {
126134
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
127135
cipher.init(Cipher.ENCRYPT_MODE, key);
128-
byte[] tag = cipher.doFinal(plaintext);
136+
byte[] hash = MessageDigest.getInstance("SHA-256").digest(plaintext);
137+
byte[] tag = cipher.doFinal(hash);
129138
byte[] expected = socket.getInputStream().readAllBytes();
130139
return MessageDigest.isEqual(expected, tag);
131140
}

0 commit comments

Comments
 (0)