Skip to content

Commit d01dc35

Browse files
artem-smotrakovartem-smotrakov
committed
Less duplicate code in java/non-constant-time-crypto-comparison
1 parent 40e513b commit d01dc35

File tree

1 file changed

+103
-110
lines changed

1 file changed

+103
-110
lines changed

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

Lines changed: 103 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -17,40 +17,88 @@ import semmle.code.java.dataflow.TaintTracking2
1717
import semmle.code.java.dataflow.FlowSources
1818
import DataFlow::PathGraph
1919

20+
private class ByteBuffer extends Class {
21+
ByteBuffer() { hasQualifiedName("java.nio", "ByteBuffer") }
22+
}
23+
24+
abstract private class ProduceCryptoCall extends MethodAccess {
25+
Expr output;
26+
27+
Expr output() { result = output }
28+
}
29+
30+
private class ProduceMacCall extends ProduceCryptoCall {
31+
ProduceMacCall() {
32+
getMethod().hasQualifiedName("javax.crypto", "Mac", "doFinal") and
33+
(
34+
getMethod().getReturnType() instanceof Array and this = output
35+
or
36+
getMethod().getParameterType(0) instanceof Array and getArgument(0) = output
37+
)
38+
}
39+
}
40+
41+
private class ProduceSignatureCall extends ProduceCryptoCall {
42+
ProduceSignatureCall() {
43+
getMethod().hasQualifiedName("java.security", "Signature", "sign") and
44+
(
45+
getMethod().getReturnType() instanceof Array and this = output
46+
or
47+
getMethod().getParameterType(0) instanceof Array and getArgument(0) = output
48+
)
49+
}
50+
}
51+
52+
private class ProduceCiphertextCall extends ProduceCryptoCall {
53+
ProduceCiphertextCall() {
54+
getMethod().hasQualifiedName("javax.crypto", "Cipher", "doFinal") and
55+
(
56+
getMethod().getReturnType() instanceof Array and this = output
57+
or
58+
getMethod().getParameterType([0, 3]) instanceof Array and getArgument([0, 3]) = output
59+
or
60+
getMethod().getParameterType(1) instanceof ByteBuffer and
61+
getArgument(1) = output
62+
)
63+
}
64+
}
65+
2066
private class UserInputInCryptoOperationConfig extends TaintTracking2::Configuration {
2167
UserInputInCryptoOperationConfig() { this = "UserInputInCryptoOperationConfig" }
2268

2369
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
2470

2571
override predicate isSink(DataFlow::Node sink) {
26-
exists(MethodAccess ma, Method m | m = ma.getMethod() |
27-
(
28-
(
29-
m.hasQualifiedName("javax.crypto", ["Mac", "Cipher"], "doFinal") or
30-
m.hasQualifiedName("java.security", "Signature", "sign")
31-
) and
32-
ma.getQualifier() = sink.asExpr()
33-
)
34-
)
72+
exists(ProduceCryptoCall call | call.getQualifier() = sink.asExpr())
3573
}
3674

3775
override predicate isAdditionalTaintStep(DataFlow2::Node fromNode, DataFlow2::Node toNode) {
38-
exists(MethodAccess ma, Method m | m = ma.getMethod() |
76+
exists(MethodAccess call, Method m |
77+
m = call.getMethod() and
78+
call.getQualifier() = toNode.asExpr() and
79+
call.getArgument(0) = fromNode.asExpr()
80+
|
3981
(
40-
(
41-
m.hasQualifiedName("javax.crypto", ["Mac", "Cipher"], ["doFinal", "update"]) or
42-
m.hasQualifiedName("java.security", "Signature", ["sign", "update"])
43-
) and
44-
ma.getAnArgument() = fromNode.asExpr() and
45-
ma.getQualifier() = toNode.asExpr()
82+
m.hasQualifiedName("java.security", "Signature", "update")
83+
or
84+
m.hasQualifiedName("javax.crypto", ["Mac", "Cipher"], "update")
85+
or
86+
m.hasQualifiedName("javax.crypto", ["Mac", "Cipher"], "doFinal") and
87+
not m.hasStringSignature("doFinal(byte[],int)")
4688
)
4789
)
4890
}
4991
}
5092

51-
abstract private class CryptoOperationSource extends DataFlow::Node {
93+
private class CryptoOperationSource extends DataFlow::Node {
5294
Expr cryptoOperation;
5395

96+
CryptoOperationSource() {
97+
exists(ProduceCryptoCall call | call.output() = this.asExpr() |
98+
cryptoOperation = call.getQualifier()
99+
)
100+
}
101+
54102
predicate includesUserInput() {
55103
exists(
56104
DataFlow2::PathNode source, DataFlow2::PathNode sink, UserInputInCryptoOperationConfig config
@@ -62,121 +110,68 @@ abstract private class CryptoOperationSource extends DataFlow::Node {
62110
}
63111
}
64112

65-
/**
66-
* A source that produces a MAC.
67-
*/
68-
private class MacSource extends CryptoOperationSource {
69-
MacSource() {
70-
exists(MethodAccess ma, Method m | m = ma.getMethod() |
71-
m.hasQualifiedName("javax.crypto", "Mac", "doFinal") and
72-
(
73-
m.getReturnType() instanceof Array and ma = this.asExpr()
74-
or
75-
m.getParameterType(0) instanceof Array and ma.getArgument(0) = this.asExpr()
76-
) and
77-
cryptoOperation = ma.getQualifier()
78-
)
79-
}
80-
}
81-
82-
/**
83-
* A source that produces a signature.
84-
*/
85-
private class SignatureSource extends CryptoOperationSource {
86-
SignatureSource() {
87-
exists(MethodAccess ma, Method m | m = ma.getMethod() |
88-
m.hasQualifiedName("java.security", "Signature", "sign") and
89-
(
90-
m.getReturnType() instanceof Array and ma = this.asExpr()
91-
or
92-
m.getParameterType(0) instanceof Array and ma.getArgument(0) = this.asExpr()
93-
) and
94-
cryptoOperation = ma.getQualifier()
95-
)
113+
private class NonConstantTimeEqualsCall extends MethodAccess {
114+
NonConstantTimeEqualsCall() {
115+
getMethod()
116+
.hasQualifiedName("java.lang", "String", ["equals", "contentEquals", "equalsIgnoreCase"]) or
117+
getMethod().hasQualifiedName("java.nio", "ByteBuffer", ["equals", "compareTo"])
96118
}
97119
}
98120

99-
/**
100-
* A source that produces a ciphertext.
101-
*/
102-
private class CiphertextSource extends CryptoOperationSource {
103-
CiphertextSource() {
104-
exists(MethodAccess ma, Method m | m = ma.getMethod() |
105-
m.hasQualifiedName("javax.crypto", "Cipher", "doFinal") and
106-
(
107-
m.getReturnType() instanceof Array and ma = this.asExpr()
108-
or
109-
m.getParameterType([0, 3]) instanceof Array and ma.getArgument([0, 3]) = this.asExpr()
110-
or
111-
m.getParameterType(1).(RefType).hasQualifiedName("java.nio", "ByteBuffer") and
112-
ma.getArgument(1) = this.asExpr()
113-
) and
114-
cryptoOperation = ma.getQualifier()
115-
)
121+
private class NonConstantTimeComparisonCall extends StaticMethodAccess {
122+
NonConstantTimeComparisonCall() {
123+
getMethod().hasQualifiedName("java.util", "Arrays", ["equals", "deepEquals"]) or
124+
getMethod().hasQualifiedName("java.util", "Objects", "deepEquals") or
125+
getMethod()
126+
.hasQualifiedName("org.apache.commons.lang3", "StringUtils",
127+
["equals", "equalsAny", "equalsAnyIgnoreCase", "equalsIgnoreCase"])
116128
}
117129
}
118130

119-
private class UserInputComparisonConfig extends TaintTracking2::Configuration {
120-
UserInputComparisonConfig() { this = "UserInputComparisonConfig" }
131+
private class UserInputInComparisonConfig extends TaintTracking2::Configuration {
132+
UserInputInComparisonConfig() { this = "UserInputInComparisonConfig" }
121133

122134
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
123135

124136
override predicate isSink(DataFlow::Node sink) {
125-
exists(MethodAccess ma, Method m | m = ma.getMethod() |
126-
(
127-
m.hasQualifiedName("java.lang", "String", ["equals", "contentEquals", "equalsIgnoreCase"]) or
128-
m.hasQualifiedName("java.nio", "ByteBuffer", ["equals", "compareTo"]) or
129-
m.hasQualifiedName("java.util", "Arrays", ["equals", "deepEquals"]) or
130-
m.hasQualifiedName("java.util", "Objects", "deepEquals") or
131-
m.hasQualifiedName("org.apache.commons.lang3", "StringUtils",
132-
["equals", "equalsAny", "equalsAnyIgnoreCase", "equalsIgnoreCase"])
133-
) and
134-
[ma.getAnArgument(), ma.getQualifier()] = sink.asExpr()
137+
exists(NonConstantTimeEqualsCall call |
138+
sink.asExpr() = [call.getAnArgument(), call.getQualifier()]
135139
)
140+
or
141+
exists(NonConstantTimeComparisonCall call | sink.asExpr() = call.getAnArgument())
136142
}
137143
}
138144

139145
/**
140146
* A sink that compares input using a non-constant time algorithm.
141147
*/
142-
private class KnownNonConstantTimeComparisonSink extends DataFlow::Node {
148+
private class NonConstantTimeComparisonSink extends DataFlow::Node {
143149
Expr anotherParameter;
144150

145-
KnownNonConstantTimeComparisonSink() {
146-
exists(MethodAccess ma, Method m | m = ma.getMethod() |
147-
(
148-
m.hasQualifiedName("java.lang", "String", ["equals", "contentEquals", "equalsIgnoreCase"]) or
149-
m.hasQualifiedName("java.nio", "ByteBuffer", ["equals", "compareTo"])
150-
) and
151-
(
152-
this.asExpr() = ma.getQualifier() and
153-
anotherParameter = ma.getAnArgument() and
154-
not ma.getAnArgument().isCompileTimeConstant()
151+
NonConstantTimeComparisonSink() {
152+
not anotherParameter.isCompileTimeConstant() and
153+
(
154+
exists(NonConstantTimeEqualsCall call |
155+
this.asExpr() = call.getQualifier() and
156+
anotherParameter = call.getAnArgument()
155157
or
156-
this.asExpr() = ma.getAnArgument() and
157-
anotherParameter = ma.getQualifier() and
158-
not ma.getQualifier().isCompileTimeConstant()
158+
this.asExpr() = call.getAnArgument() and
159+
anotherParameter = call.getQualifier()
159160
)
160-
)
161-
or
162-
exists(StaticMethodAccess ma, Method m | m = ma.getMethod() |
163-
(
164-
m.hasQualifiedName("java.util", "Arrays", ["equals", "deepEquals"]) or
165-
m.hasQualifiedName("java.util", "Objects", "deepEquals") or
166-
m.hasQualifiedName("org.apache.commons.lang3", "StringUtils",
167-
["equals", "equalsAny", "equalsAnyIgnoreCase", "equalsIgnoreCase"])
168-
) and
169-
ma.getAnArgument() = this.asExpr() and
170-
(
171-
this.asExpr() = ma.getArgument(0) and anotherParameter = ma.getArgument(1)
172-
or
173-
this.asExpr() = ma.getArgument(1) and anotherParameter = ma.getArgument(0)
161+
or
162+
exists(NonConstantTimeComparisonCall call |
163+
call.getAnArgument() = this.asExpr() and
164+
(
165+
this.asExpr() = call.getArgument(0) and anotherParameter = call.getArgument(1)
166+
or
167+
this.asExpr() = call.getArgument(1) and anotherParameter = call.getArgument(0)
168+
)
174169
)
175170
)
176171
}
177172

178173
predicate includesUserInput() {
179-
exists(UserInputComparisonConfig config |
174+
exists(UserInputInComparisonConfig config |
180175
config.hasFlowTo(DataFlow2::exprNode(anotherParameter))
181176
)
182177
}
@@ -191,17 +186,15 @@ private class NonConstantTimeCryptoComparisonConfig extends TaintTracking::Confi
191186

192187
override predicate isSource(DataFlow::Node source) { source instanceof CryptoOperationSource }
193188

194-
override predicate isSink(DataFlow::Node sink) {
195-
sink instanceof KnownNonConstantTimeComparisonSink
196-
}
189+
override predicate isSink(DataFlow::Node sink) { sink instanceof NonConstantTimeComparisonSink }
197190
}
198191

199192
from DataFlow::PathNode source, DataFlow::PathNode sink, NonConstantTimeCryptoComparisonConfig conf
200193
where
201194
conf.hasFlowPath(source, sink) and
202195
(
203196
source.getNode().(CryptoOperationSource).includesUserInput() or
204-
sink.getNode().(KnownNonConstantTimeComparisonSink).includesUserInput()
197+
sink.getNode().(NonConstantTimeComparisonSink).includesUserInput()
205198
)
206199
select sink.getNode(), source, sink,
207200
"Using a non-constant time algorithm for comparing results of a cryptographic operation."

0 commit comments

Comments
 (0)