Skip to content

Commit 0e624f5

Browse files
bdrodesbdrodes
committed
Crypto: Adding bad decrypt then mac order query. Fixes to BadMacOrderMacOnEncryptPlaintext as well.
1 parent 8c277bd commit 0e624f5

File tree

9 files changed

+341
-47
lines changed

9 files changed

+341
-47
lines changed

‎java/ql/integration-tests/java/query-suite/not_included_in_qls.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,7 @@ ql/java/ql/src/experimental/Security/CWE/CWE-665/InsecureRmiJmxEnvironmentConfig
228228
ql/java/ql/src/experimental/Security/CWE/CWE-755/NFEAndroidDoS.ql
229229
ql/java/ql/src/experimental/Security/CWE/CWE-759/HashWithoutSalt.ql
230230
ql/java/ql/src/experimental/Security/CWE/CWE-939/IncorrectURLVerification.ql
231+
ql/java/ql/src/experimental/quantum/Examples/BadMacOrderDecryptThenMac.ql
231232
ql/java/ql/src/experimental/quantum/Examples/BadMacOrderDecryptToMac.ql
232233
ql/java/ql/src/experimental/quantum/Examples/BadMacOrderMacOnEncryptPlaintext.ql
233234
ql/java/ql/src/experimental/quantum/Examples/BrokenCrypto.ql

‎java/ql/src/experimental/quantum/Examples/BadMacOrder.qll‎

Lines changed: 215 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import codeql.util.Option
77
* that flows to the input artifact of a mac operation.
88
*/
99
predicate isDecryptToMacFlow(ArtifactFlow::PathNode src, ArtifactFlow::PathNode sink) {
10+
// Simply flow from decrypt output to a mac input
1011
ArtifactFlow::flowPath(src, sink) and
1112
exists(Crypto::CipherOperationNode cipherOp |
1213
cipherOp.getKeyOperationSubtype() = Crypto::TDecryptMode() and
@@ -17,30 +18,163 @@ predicate isDecryptToMacFlow(ArtifactFlow::PathNode src, ArtifactFlow::PathNode
1718
)
1819
}
1920

21+
/**
22+
* Experimental interface for graph generation, supply the
23+
* node to determine if a issue exists, and if so
24+
* the graph can add a property on the node.
25+
*/
2026
predicate isDecryptToMacNode(Crypto::ArtifactNode node) {
2127
exists(ArtifactFlow::PathNode src |
2228
isDecryptToMacFlow(src, _) and
2329
node.asElement() = src.getNode().asExpr()
2430
)
2531
}
2632

33+
predicate isDecryptThenMacFlow(DecryptThenMacFlow::PathNode src, DecryptThenMacFlow::PathNode sink) {
34+
DecryptThenMacFlow::flowPath(src, sink)
35+
}
36+
2737
/**
2838
* Holds when the src node is used as plaintext input to both
2939
* an encryption operation and a mac operation, via the
3040
* argument represented by InterimArg.
3141
*/
3242
predicate isPlaintextInEncryptionAndMac(
3343
PlaintextUseAsMacAndCipherInputFlow::PathNode src,
34-
PlaintextUseAsMacAndCipherInputFlow::PathNode sink, InterimArg arg
44+
PlaintextUseAsMacAndCipherInputFlow::PathNode sink, EncryptOrMacCallArg arg
3545
) {
3646
PlaintextUseAsMacAndCipherInputFlow::flowPath(src, sink) and
37-
arg = sink.getState().asSome()
47+
arg = sink.getState().asSome() and
48+
// the above pathing adds flow steps that may not have consideration for the calling context
49+
// TODO: this is something we want to look into to improving, but for now
50+
// we can filter bad flows with one additional flow check, that the source goes to both
51+
// src and sink through a generic flow
52+
// note that the flow path above ensures src gets to the interim arg, so we just need to
53+
// verify the source to sink.
54+
// TODO: having to copy the generic data flow config into a use-use variant
55+
// should we fix this at the language level to allow use use more intuitively?
56+
// Seems to be a common issue.
57+
GenericDataSourceFlowUseUseFlow::flow(src.getNode(), sink.getNode())
3858
}
3959

40-
module ArgToSinkConfig implements DataFlow::ConfigSig {
60+
/**
61+
* A copy of GenericDataSourceFlow but with use-use flows enabled by removing the barrier out
62+
*/
63+
module GenericDataSourceFlowUseUseConfig implements DataFlow::ConfigSig {
64+
predicate isSource(DataFlow::Node source) {
65+
source = any(Crypto::GenericSourceInstance i).getOutputNode()
66+
}
67+
68+
predicate isSink(DataFlow::Node sink) {
69+
sink = any(Crypto::FlowAwareElement other).getInputNode()
70+
}
71+
72+
predicate isBarrierIn(DataFlow::Node node) {
73+
node = any(Crypto::FlowAwareElement element).getOutputNode()
74+
}
75+
76+
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
77+
node1.(AdditionalFlowInputStep).getOutput() = node2
78+
or
79+
exists(MethodCall m |
80+
m.getMethod().hasQualifiedName("java.lang", "String", "getBytes") and
81+
node1.asExpr() = m.getQualifier() and
82+
node2.asExpr() = m
83+
)
84+
}
85+
}
86+
87+
module GenericDataSourceFlowUseUseFlow = TaintTracking::Global<GenericDataSourceFlowUseUseConfig>;
88+
89+
module WrapperArgFlowConfig implements DataFlow::ConfigSig {
90+
predicate isSource(DataFlow::Node source) {
91+
// Start from a parameter and not a call to avoid flow going out of
92+
// the call. We want to flow down a call, so start from a parameter
93+
// and barrier flows through returns
94+
exists(Method m | m.getParameter(_) = source.asParameter())
95+
}
96+
97+
predicate isSink(DataFlow::Node sink) {
98+
exists(Crypto::CipherOperationNode cipherOp |
99+
cipherOp.getAnInputArtifact().asElement() = sink.asExpr()
100+
)
101+
or
102+
exists(Crypto::MacOperationNode macOp | macOp.getAnInputArtifact().asElement() = sink.asExpr())
103+
}
104+
105+
predicate isBarrierIn(DataFlow::Node node) {
106+
node = any(Crypto::FlowAwareElement element).getOutputNode()
107+
}
108+
109+
predicate isBarrierOut(DataFlow::Node node) {
110+
// stop all flow out of a call return
111+
// TODO: this might be too strict and remove taint flow, need to reassess
112+
exists(Call c | c = node.asExpr()) or
113+
node = any(Crypto::FlowAwareElement element).getInputNode()
114+
}
115+
116+
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
117+
node1.(AdditionalFlowInputStep).getOutput() = node2
118+
or
119+
exists(MethodCall m |
120+
m.getMethod().hasQualifiedName("java.lang", "String", "getBytes") and
121+
node1.asExpr() = m.getQualifier() and
122+
node2.asExpr() = m
123+
)
124+
}
125+
}
126+
127+
module WrapperArgFlow = TaintTracking::Global<WrapperArgFlowConfig>;
128+
129+
predicate encryptWrapperArg(DataFlow::Node n, DataFlow::Node sink) {
130+
exists(Crypto::CipherOperationNode cipherOp |
131+
cipherOp.getKeyOperationSubtype() = Crypto::TEncryptMode() and
132+
cipherOp.getAnInputArtifact().asElement() = sink.asExpr()
133+
) and
134+
(
135+
exists(Parameter p, DataFlow::Node src |
136+
p = src.asParameter() and
137+
WrapperArgFlow::flow(src, sink) and
138+
n.asExpr() = p.getAnArgument()
139+
)
140+
or
141+
n = sink // the call the target operation is considered a wrapper arg to itself
142+
)
143+
}
144+
145+
predicate decryptWrapperArg(DataFlow::Node n, DataFlow::Node sink) {
146+
exists(Crypto::CipherOperationNode cipherOp |
147+
cipherOp.getKeyOperationSubtype() = Crypto::TDecryptMode() and
148+
cipherOp.getAnInputArtifact().asElement() = sink.asExpr()
149+
) and
150+
(
151+
exists(Parameter p, DataFlow::Node src |
152+
p = src.asParameter() and
153+
WrapperArgFlow::flow(src, sink) and
154+
n.asExpr() = p.getAnArgument()
155+
)
156+
or
157+
n = sink // the call the target operation is considered a wrapper arg to itself
158+
)
159+
}
160+
161+
predicate macWrapperArg(DataFlow::Node n, DataFlow::Node sink) {
162+
exists(Crypto::MacOperationNode macOp | macOp.getAnInputArtifact().asElement() = sink.asExpr()) and
163+
(
164+
exists(Parameter p, DataFlow::Node src |
165+
p = src.asParameter() and
166+
WrapperArgFlow::flow(src, sink) and
167+
n.asExpr() = p.getAnArgument()
168+
)
169+
or
170+
n = sink // the call the target operation is considered a wrapper arg to itself
171+
)
172+
}
173+
174+
module ArgToEncryptOrMacConfig implements DataFlow::ConfigSig {
41175
predicate isSource(DataFlow::Node source) { exists(Call c | c.getAnArgument() = source.asExpr()) }
42176

43-
predicate isSink(DataFlow::Node sink) { targetSinks(sink) }
177+
predicate isSink(DataFlow::Node sink) { encryptOrMacSink(sink) }
44178

45179
// Don't go in to a known out node, this will prevent the plaintext
46180
// from tracing out of cipher operations for example, we just want to trace
@@ -62,12 +196,12 @@ module ArgToSinkConfig implements DataFlow::ConfigSig {
62196
}
63197
}
64198

65-
module ArgToSinkFlow = TaintTracking::Global<ArgToSinkConfig>;
199+
module ArgToEncryptOrMacFlow = TaintTracking::Global<ArgToEncryptOrMacConfig>;
66200

67201
/**
68202
* Target sinks for this query are either encryption operations or mac operation message inputs
69203
*/
70-
predicate targetSinks(DataFlow::Node n) {
204+
predicate encryptOrMacSink(DataFlow::Node n) {
71205
exists(Crypto::CipherOperationNode cipherOp |
72206
cipherOp.getKeyOperationSubtype() = Crypto::TEncryptMode() and
73207
cipherOp.getAnInputArtifact().asElement() = n.asExpr()
@@ -77,44 +211,48 @@ predicate targetSinks(DataFlow::Node n) {
77211
}
78212

79213
/**
80-
* An argument of a target sink or a parent call whose parameter flows to a target sink
214+
* Target sinks for decryption operations
81215
*/
82-
class InterimArg extends DataFlow::Node {
83-
DataFlow::Node targetSink;
84-
85-
InterimArg() {
86-
targetSinks(targetSink) and
87-
(
88-
this = targetSink
89-
or
90-
ArgToSinkFlow::flow(this, targetSink) and
91-
this.getEnclosingCallable().calls+(targetSink.getEnclosingCallable())
92-
)
93-
}
94-
95-
DataFlow::Node getTargetSink() { result = targetSink }
216+
predicate decryptSink(DataFlow::Node n) {
217+
exists(Crypto::CipherOperationNode cipherOp |
218+
cipherOp.getKeyOperationSubtype() = Crypto::TDecryptMode() and
219+
cipherOp.getAnInputArtifact().asElement() = n.asExpr()
220+
)
96221
}
97222

98-
/**
99-
* A wrapper class to represent a target argument dataflow node.
100-
*/
101-
class TargetArg extends DataFlow::Node {
102-
TargetArg() { targetSinks(this) }
223+
// /**
224+
// * An argument of a target sink or a parent call whose parameter flows to a target sink
225+
// */
226+
// class EncryptOrMacPartialFlowArg extends DataFlow::Node {
227+
// DataFlow::Node targetSink;
228+
// EncryptOrMacPartialFlowArg() {
229+
// encryptWrapperArg(this, targetSink)
230+
// or
231+
// macWrapperArg(this, targetSink)
232+
// }
233+
// DataFlow::Node getTargetSink() { result = targetSink }
234+
// }
235+
class EncryptOrMacCallArg extends DataFlow::Node {
236+
boolean isEncryption;
103237

104-
predicate isCipher() {
238+
EncryptOrMacCallArg() {
105239
exists(Crypto::CipherOperationNode cipherOp |
106240
cipherOp.getKeyOperationSubtype() = Crypto::TEncryptMode() and
107241
cipherOp.getAnInputArtifact().asElement() = this.asExpr()
108-
)
242+
) and
243+
isEncryption = true
244+
or
245+
exists(Crypto::MacOperationNode macOp | macOp.getAnInputArtifact().asElement() = this.asExpr()) and
246+
isEncryption = false
109247
}
110248

111-
predicate isMac() {
112-
exists(Crypto::MacOperationNode macOp | macOp.getAnInputArtifact().asElement() = this.asExpr())
113-
}
249+
predicate isEncryption() { isEncryption = true }
250+
251+
predicate isMac() { isEncryption = false }
114252
}
115253

116254
module PlaintextUseAsMacAndCipherInputConfig implements DataFlow::StateConfigSig {
117-
class FlowState = Option<TargetArg>::Option;
255+
class FlowState = Option<EncryptOrMacCallArg>::Option;
118256

119257
// TODO: can we approximate a message source better?
120258
predicate isSource(DataFlow::Node source, FlowState state) {
@@ -128,12 +266,9 @@ module PlaintextUseAsMacAndCipherInputConfig implements DataFlow::StateConfigSig
128266
}
129267

130268
predicate isSink(DataFlow::Node sink, FlowState state) {
131-
sink instanceof TargetArg and
132-
(
133-
sink.(TargetArg).isMac() and state.asSome().isCipher()
134-
or
135-
sink.(TargetArg).isCipher() and state.asSome().isMac()
136-
)
269+
sink.(EncryptOrMacCallArg).isMac() and state.asSome().isEncryption()
270+
or
271+
sink.(EncryptOrMacCallArg).isEncryption() and state.asSome().isMac()
137272
}
138273

139274
predicate isBarrierOut(DataFlow::Node node, FlowState state) {
@@ -163,16 +298,51 @@ module PlaintextUseAsMacAndCipherInputConfig implements DataFlow::StateConfigSig
163298
predicate isAdditionalFlowStep(
164299
DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
165300
) {
166-
(exists(state1.asSome()) or state1.isNone()) and
167-
targetSinks(node1) and
168-
node1 instanceof TargetArg and
169-
//use-use flow, either flow directly from the node1 use
170-
//or find a parent call in the call in the call stack
171-
//and continue flow from that parameter
172-
node2.(InterimArg).getTargetSink() = node1 and
301+
// TODO: should we consider isSome cases?
302+
state1.isNone() and
303+
(
304+
encryptWrapperArg(node2, node1)
305+
or
306+
macWrapperArg(node2, node1)
307+
) and
173308
state2.asSome() = node1
174309
}
175310
}
176311

177312
module PlaintextUseAsMacAndCipherInputFlow =
178313
TaintTracking::GlobalWithState<PlaintextUseAsMacAndCipherInputConfig>;
314+
315+
module DecryptThenMacConfig implements DataFlow::ConfigSig {
316+
predicate isSource(DataFlow::Node source) {
317+
exists(Crypto::CipherOperationNode cipherOp |
318+
cipherOp.getKeyOperationSubtype() = Crypto::TDecryptMode() and
319+
cipherOp.getAnInputArtifact().asElement() = source.asExpr()
320+
)
321+
}
322+
323+
predicate isSink(DataFlow::Node sink) {
324+
exists(Crypto::MacOperationNode macOp | macOp.getAnInputArtifact().asElement() = sink.asExpr())
325+
}
326+
327+
// Don't go in to a known out node, prevents
328+
// from tracing out of an operation
329+
// NOTE: we are not using a barrier out on input nodes, because
330+
// that would remove 'use-use' flows, which we need
331+
predicate isBarrierIn(DataFlow::Node node) {
332+
node = any(Crypto::FlowAwareElement element).getOutputNode()
333+
}
334+
335+
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
336+
node1.(AdditionalFlowInputStep).getOutput() = node2
337+
or
338+
exists(MethodCall m |
339+
m.getMethod().hasQualifiedName("java.lang", "String", "getBytes") and
340+
node1.asExpr() = m.getQualifier() and
341+
node2.asExpr() = m
342+
)
343+
or
344+
decryptWrapperArg(node2, node1)
345+
}
346+
}
347+
348+
module DecryptThenMacFlow = TaintTracking::Global<DecryptThenMacConfig>;

‎java/ql/src/experimental/quantum/Examples/BadMacOrderDecryptThenMac.ql‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
/**
2+
* @name Bad MAC order: decrypt then mac
3+
* @description Decryption on cipher text, then MAC on ciopher text, is incorrect order
4+
* @id java/quantum/examples/bad-mac-order-decrypt-then-mac
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @tags quantum
8+
* experimental
9+
*/
10+
11+
import java
12+
import BadMacOrder
13+
import DecryptThenMacFlow::PathGraph
14+
15+
from DecryptThenMacFlow::PathNode src, DecryptThenMacFlow::PathNode sink
16+
where isDecryptThenMacFlow(src, sink)
17+
select sink, src, sink,
18+
"Incorrect decryption and MAC order: " +
19+
"Decryption of cipher text occurs before validation of MAC on cipher text."

‎java/ql/src/experimental/quantum/Examples/BadMacOrderMacOnEncryptPlaintext.ql‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import PlaintextUseAsMacAndCipherInputFlow::PathGraph
1414

1515
from
1616
PlaintextUseAsMacAndCipherInputFlow::PathNode src,
17-
PlaintextUseAsMacAndCipherInputFlow::PathNode sink, InterimArg arg
17+
PlaintextUseAsMacAndCipherInputFlow::PathNode sink, EncryptOrMacCallArg arg
1818
where isPlaintextInEncryptionAndMac(src, sink, arg)
1919
select sink, src, sink,
2020
"Incorrect MAC usage: Encryption plaintext also used for MAC. Flow shows plaintext to final use through intermediate mac or encryption operation here $@",

0 commit comments

Comments
 (0)