Skip to content

Commit 758759a

Browse files
bdrodesbdrodes
committed
Crypto: Reused nonce query updates and test updates to address false positives.
1 parent fba8087 commit 758759a

File tree

4 files changed

+115
-17
lines changed

4 files changed

+115
-17
lines changed

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

Lines changed: 46 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import experimental.quantum.Language
88
* NOTE: TODO: need to handle call by refernece for now. Need to re-evaluate (see notes below)
99
* Such functions may be 'wrappers' for some derived value.
1010
*/
11-
private module WrapperConfig implements DataFlow::ConfigSig {
11+
private module ArtifactGeneratingWrapperConfig implements DataFlow::ConfigSig {
1212
predicate isSource(DataFlow::Node source) {
1313
source.asExpr() instanceof Call and
1414
// not handling references yet, I think we want to flat say references are only ok
@@ -28,25 +28,41 @@ private module WrapperConfig implements DataFlow::ConfigSig {
2828
predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(Crypto::ArtifactNode i).asElement() }
2929
}
3030

31-
module WrapperFlow = DataFlow::Global<WrapperConfig>;
31+
module ArtifactGeneratingWrapperFlow = TaintTracking::Global<ArtifactGeneratingWrapperConfig>;
3232

3333
/**
3434
* Using a set approach to determine if reuse of an artifact exists.
3535
* This predicate produces a set of 'wrappers' that flow to the artifact node.
3636
* This set can be compared with the set to another artifact node to determine if they are the same.
3737
*/
38-
private DataFlow::Node getWrapperSet(Crypto::NonceArtifactNode a) {
39-
WrapperFlow::flow(result, DataFlow::exprNode(a.asElement()))
38+
private DataFlow::Node getGeneratingWrapperSet(Crypto::NonceArtifactNode a) {
39+
ArtifactGeneratingWrapperFlow::flow(result, DataFlow::exprNode(a.asElement()))
4040
or
4141
result.asExpr() = a.getSourceElement()
4242
}
4343

44+
private predicate ancestorOfArtifact(
45+
Crypto::ArtifactNode a, Callable enclosingCallable, ControlFlow::Node midOrTarget
46+
) {
47+
a.asElement().(Expr).getEnclosingCallable() = enclosingCallable and
48+
(
49+
midOrTarget.asExpr() = a.asElement() or
50+
midOrTarget.asExpr().(Call).getCallee().calls*(a.asElement().(Expr).getEnclosingCallable())
51+
)
52+
}
53+
4454
/**
4555
* Two different artifact nodes are considered reuse if any of the following conditions are met:
4656
* 1. The source for artifact `a` and artifact `b` are the same and the source is a literal.
4757
* 2. The source for artifact `a` and artifact `b` are not the same and the source is a literal of the same value.
48-
* 3. For all 'wrappers' that return the source of artifact `a`, and that wrapper also exists for artifact `b`.
49-
* 4. For all 'wrappers' that return the source of artifact `b`, and that wrapper also exists for artifact `a`.
58+
* 3. For all 'wrappers' that return the source of artifact `a`, and each wrapper also exists for artifact `b`.
59+
* 4. For all 'wrappers' that return the source of artifact `b`, and each wrapper also exists for artifact `a`.
60+
*
61+
* The above conditions determine that the use of the IV is from the same source, but the use may
62+
* be on separate code paths that do not occur sequentially. We must therefore also find a common callable ancestor
63+
* for both uses, and in that ancestor, there must be control flow from the call or use of one to the call or use of the other.
64+
* Note that if no shared ancestor callable exists, it means the flow is more nuanced and ignore the shared ancestor
65+
* use flow.
5066
*/
5167
predicate isArtifactReuse(Crypto::ArtifactNode a, Crypto::ArtifactNode b) {
5268
a != b and
@@ -55,12 +71,32 @@ predicate isArtifactReuse(Crypto::ArtifactNode a, Crypto::ArtifactNode b) {
5571
or
5672
a.getSourceElement().(Literal).getValue() = b.getSourceElement().(Literal).getValue()
5773
or
58-
forex(DataFlow::Node e | e = getWrapperSet(a) |
59-
exists(DataFlow::Node e2 | e2 = getWrapperSet(b) | e = e2)
74+
forex(DataFlow::Node e | e = getGeneratingWrapperSet(a) |
75+
exists(DataFlow::Node e2 | e2 = getGeneratingWrapperSet(b) | e = e2)
6076
)
6177
or
62-
forex(DataFlow::Node e | e = getWrapperSet(b) |
63-
exists(DataFlow::Node e2 | e2 = getWrapperSet(a) | e = e2)
78+
forex(DataFlow::Node e | e = getGeneratingWrapperSet(b) |
79+
exists(DataFlow::Node e2 | e2 = getGeneratingWrapperSet(a) | e = e2)
80+
)
81+
) and
82+
// If there is a common parent, there is control flow between the two uses in the parent
83+
// TODO: this logic needs to be examined/revisited to ensure it is correct
84+
(
85+
exists(Callable commonParent |
86+
ancestorOfArtifact(a, commonParent, _) and
87+
ancestorOfArtifact(b, commonParent, _)
88+
)
89+
implies
90+
exists(Callable commonParent, ControlFlow::Node aMid, ControlFlow::Node bMid |
91+
ancestorOfArtifact(a, commonParent, aMid) and
92+
ancestorOfArtifact(b, commonParent, bMid) and
93+
a instanceof Crypto::NonceArtifactNode and
94+
b instanceof Crypto::NonceArtifactNode and
95+
(
96+
aMid.getASuccessor*() = bMid
97+
or
98+
bMid.getASuccessor*() = aMid
99+
)
64100
)
65101
)
66102
}

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

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,40 @@
1212
import java
1313
import ArtifactReuse
1414

15-
from Crypto::NonceArtifactNode nonce1, Crypto::NonceArtifactNode nonce2
16-
where isArtifactReuse(nonce1, nonce2)
17-
select nonce1, "Reuse with nonce $@", nonce2, nonce2.toString()
15+
from Crypto::NonceArtifactNode nonce1, Crypto::NonceArtifactNode nonce2, Crypto::NodeBase sourceNode
16+
where
17+
isArtifactReuse(nonce1, nonce2) and
18+
// NOTE: in general we may not know a source, but see possible reuse,
19+
// we are not detecting these cases here (only where the source is the same).
20+
sourceNode = nonce1.getSourceNode() and
21+
sourceNode = nonce2.getSourceNode() and
22+
// Null literals are typically used for initialization, and if two 'nulls'
23+
// are reused, it is likely an uninitialization path that would result in a NullPointerException.
24+
not sourceNode.asElement() instanceof NullLiteral and
25+
// if the nonce is used in an encryption and decryption, ignore that reuse
26+
not exists(Crypto::CipherOperationNode op1, Crypto::CipherOperationNode op2 |
27+
op1 != op2 and
28+
op1.getANonce() = nonce1 and
29+
op2.getANonce() = nonce2 and
30+
(
31+
(
32+
op1.getKeyOperationSubtype() instanceof Crypto::TEncryptMode or
33+
op1.getKeyOperationSubtype() instanceof Crypto::TWrapMode
34+
) and
35+
(
36+
op2.getKeyOperationSubtype() instanceof Crypto::TDecryptMode or
37+
op2.getKeyOperationSubtype() instanceof Crypto::TUnwrapMode
38+
)
39+
or
40+
(
41+
op2.getKeyOperationSubtype() instanceof Crypto::TEncryptMode or
42+
op2.getKeyOperationSubtype() instanceof Crypto::TWrapMode
43+
) and
44+
(
45+
op1.getKeyOperationSubtype() instanceof Crypto::TDecryptMode or
46+
op1.getKeyOperationSubtype() instanceof Crypto::TUnwrapMode
47+
)
48+
)
49+
)
50+
select sourceNode, "Nonce source is reused, see $@ and $@", nonce1, nonce1.toString(), nonce2,
51+
nonce2.toString()

‎java/ql/test/experimental/query-tests/quantum/examples/NonceReuse/NonceReuse.expected‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
| Test.java:40:47:40:52 | Nonce | Reuse with nonce $@ | Test.java:49:47:49:52 | Nonce | Nonce |
2-
| Test.java:49:47:49:52 | Nonce | Reuse with nonce $@ | Test.java:40:47:40:52 | Nonce | Nonce |
3-
| Test.java:76:48:76:54 | Nonce | Reuse with nonce $@ | Test.java:82:49:82:55 | Nonce | Nonce |
4-
| Test.java:82:49:82:55 | Nonce | Reuse with nonce $@ | Test.java:76:48:76:54 | Nonce | Nonce |
1+
| Test.java:19:38:19:40 | RandomNumberGeneration | Nonce source is reused, see $@ and $@ | Test.java:40:47:40:52 | Nonce | Nonce | Test.java:49:47:49:52 | Nonce | Nonce |
2+
| Test.java:19:38:19:40 | RandomNumberGeneration | Nonce source is reused, see $@ and $@ | Test.java:49:47:49:52 | Nonce | Nonce | Test.java:40:47:40:52 | Nonce | Nonce |
3+
| Test.java:19:38:19:40 | RandomNumberGeneration | Nonce source is reused, see $@ and $@ | Test.java:76:48:76:54 | Nonce | Nonce | Test.java:82:49:82:55 | Nonce | Nonce |
4+
| Test.java:19:38:19:40 | RandomNumberGeneration | Nonce source is reused, see $@ and $@ | Test.java:82:49:82:55 | Nonce | Nonce | Test.java:76:48:76:54 | Nonce | Nonce |

‎java/ql/test/experimental/query-tests/quantum/examples/NonceReuse/Test.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,34 @@ private static void funcA3() throws Exception {
8383
byte[] ciphertext2 = cipher2.doFinal("Simple Test Data".getBytes());
8484
}
8585

86+
public void falsePositive1() throws Exception {
87+
byte[] iv = null;
88+
new SecureRandom().nextBytes(iv);
89+
IvParameterSpec ivSpec = new IvParameterSpec(iv);
90+
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
91+
SecretKey key = generateAESKey();
92+
if (iv != null) {
93+
cipher.init(Cipher.ENCRYPT_MODE, key, ivSpec); // GOOD
94+
byte[] ciphertext = cipher.doFinal("Simple Test Data".getBytes());
95+
} else if(iv.length > 0) {
96+
cipher.init(Cipher.ENCRYPT_MODE, key, ivSpec); // GOOD
97+
byte[] ciphertext = cipher.doFinal("Simple Test Data".getBytes());
98+
}
99+
}
100+
101+
public void falsePositive2() throws Exception {
102+
byte[] iv = null;
103+
new SecureRandom().nextBytes(iv);
104+
IvParameterSpec ivSpec = new IvParameterSpec(iv);
105+
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
106+
SecretKey key = generateAESKey();
107+
cipher.init(Cipher.ENCRYPT_MODE, key, ivSpec); // GOOD
108+
byte[] ciphertext = cipher.doFinal("Simple Test Data".getBytes());
109+
110+
cipher.init(Cipher.DECRYPT_MODE, key, ivSpec); // GOOD
111+
byte[] decryptedData = cipher.doFinal(ciphertext);
112+
}
113+
86114
public static void main(String[] args) {
87115
try {
88116
funcA2();

0 commit comments

Comments
 (0)