Skip to content

Commit e6409e1

Browse files
owen-mcowen-mc
committed
Give reason why crypto algorithm is insecure
1 parent bcb7901 commit e6409e1

File tree

8 files changed

+63
-31
lines changed

8 files changed

+63
-31
lines changed

‎java/ql/lib/semmle/code/java/security/Encryption.qll

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -198,19 +198,32 @@ private string algorithmRegex(string algorithmString) {
198198
}
199199

200200
/**
201-
* Gets the name of an algorithm that is known to be insecure.
201+
* Holds if `name` is the name of an algorithm that is known to be insecure and
202+
* `reason` explains why it is insecure.
202203
*/
203-
string getAnInsecureAlgorithmName() {
204-
result =
205-
[
206-
"DES", "RC2", "RC4", "RC5",
207-
// ARCFOUR is a variant of RC4
208-
"ARCFOUR",
209-
// Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks
210-
"ECB",
211-
// CBC mode of operation with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks
212-
"AES/CBC/PKCS[57]Padding"
213-
]
204+
predicate insecureAlgorithm(string name, string reason) {
205+
name = "DES" and
206+
reason =
207+
"It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead."
208+
or
209+
name = "RC2" and
210+
reason = "It is vulnerable to related-key attacks. Consider using AES instead."
211+
or
212+
// ARCFOUR is a variant of RC4
213+
name = ["RC4", "ARCFOUR"] and
214+
reason =
215+
"It has multiple vulnerabilities, including biases in its output and susceptibility to several attacks. Consider using AES instead."
216+
or
217+
name = "RC5" and
218+
reason = "It is vulnerable to differential and related-key attacks. Consider using AES instead."
219+
or
220+
name = "ECB" and
221+
reason =
222+
"Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks. Consider using AES instead."
223+
or
224+
name = "AES/CBC/PKCS[57]Padding" and
225+
reason =
226+
"CBC mode of operation with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using AES instead."
214227
}
215228

216229
/**
@@ -223,7 +236,7 @@ string getAnInsecureHashAlgorithmName() {
223236
}
224237

225238
private string rankedInsecureAlgorithm(int i) {
226-
result = rank[i](string s | s = getAnInsecureAlgorithmName())
239+
result = rank[i](string name | insecureAlgorithm(name, _))
227240
}
228241

229242
private string insecureAlgorithmString(int i) {
@@ -240,6 +253,12 @@ string getInsecureAlgorithmRegex() {
240253
result = algorithmRegex(insecureAlgorithmString(max(int i | exists(rankedInsecureAlgorithm(i)))))
241254
}
242255

256+
/** Gets the reason why `input` is an insecure algorithm, if any. */
257+
bindingset[input]
258+
string getInsecureAlgorithmReason(string input) {
259+
exists(string name | insecureAlgorithm(name, result) | input.regexpMatch(algorithmRegex(name)))
260+
}
261+
243262
/**
244263
* Gets the name of an algorithm that is known to be secure.
245264
*/

‎java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,11 @@ import InsecureCryptoFlow::PathGraph
1818

1919
from
2020
InsecureCryptoFlow::PathNode source, InsecureCryptoFlow::PathNode sink, CryptoAlgoSpec spec,
21-
BrokenAlgoLiteral algo
21+
BrokenAlgoLiteral algo, string reason
2222
where
2323
sink.getNode().asExpr() = spec.getAlgoSpec() and
2424
source.getNode().asExpr() = algo and
25+
reason = getInsecureAlgorithmReason(algo.getValue()) and
2526
InsecureCryptoFlow::flowPath(source, sink)
26-
select spec, source, sink, "Cryptographic algorithm $@ is weak and should not be used.", algo,
27+
select spec, source, sink, "Cryptographic algorithm $@ is insecure. " + reason, algo,
2728
algo.getValue()

‎java/ql/test/library-tests/Encryption/Test.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,10 @@ class Test {
1111
"des_function",
1212
"function_using_des",
1313
"EncryptWithDES",
14+
"RC2",
15+
"RC4",
16+
"ARCFOUR",
17+
"RC5",
1418
"AES/ECB/NoPadding",
1519
"AES/CBC/PKCS5Padding");
1620

‎java/ql/test/library-tests/Encryption/cryptoalgospec.expected

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
| Test.java:37:4:37:17 | super(...) | Test.java:37:10:37:15 | "some" |
2-
| Test.java:41:3:41:38 | getInstance(...) | Test.java:41:29:41:37 | "another" |
1+
| Test.java:41:4:41:17 | super(...) | Test.java:41:10:41:15 | "some" |
2+
| Test.java:45:3:45:38 | getInstance(...) | Test.java:45:29:45:37 | "another" |

‎java/ql/test/library-tests/Encryption/insecure.expected

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
1-
| Test.java:9:4:9:8 | "DES" |
2-
| Test.java:10:4:10:8 | "des" |
3-
| Test.java:11:4:11:17 | "des_function" |
4-
| Test.java:12:4:12:23 | "function_using_des" |
5-
| Test.java:13:4:13:19 | "EncryptWithDES" |
6-
| Test.java:14:4:14:22 | "AES/ECB/NoPadding" |
7-
| Test.java:15:4:15:25 | "AES/CBC/PKCS5Padding" |
1+
| Test.java:9:4:9:8 | "DES" | It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead. |
2+
| Test.java:10:4:10:8 | "des" | It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead. |
3+
| Test.java:11:4:11:17 | "des_function" | It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead. |
4+
| Test.java:12:4:12:23 | "function_using_des" | It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead. |
5+
| Test.java:13:4:13:19 | "EncryptWithDES" | It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead. |
6+
| Test.java:14:4:14:8 | "RC2" | It is vulnerable to related-key attacks. Consider using AES instead. |
7+
| Test.java:15:4:15:8 | "RC4" | It has multiple vulnerabilities, including biases in its output and susceptibility to several attacks. Consider using AES instead. |
8+
| Test.java:16:4:16:12 | "ARCFOUR" | It has multiple vulnerabilities, including biases in its output and susceptibility to several attacks. Consider using AES instead. |
9+
| Test.java:17:4:17:8 | "RC5" | It is vulnerable to differential and related-key attacks. Consider using AES instead. |
10+
| Test.java:18:4:18:22 | "AES/ECB/NoPadding" | Encryption mode ECB like AES/ECB/NoPadding is vulnerable to replay and other attacks. Consider using AES instead. |
11+
| Test.java:19:4:19:25 | "AES/CBC/PKCS5Padding" | CBC mode of operation with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using AES instead. |

‎java/ql/test/library-tests/Encryption/insecure.ql

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
import default
22
import semmle.code.java.security.Encryption
33

4-
from StringLiteral s
5-
where s.getValue().regexpMatch(getInsecureAlgorithmRegex())
6-
select s
4+
from StringLiteral s, string reason
5+
where
6+
s.getValue().regexpMatch(getInsecureAlgorithmRegex()) and
7+
if exists(getInsecureAlgorithmReason(s.getValue()))
8+
then reason = getInsecureAlgorithmReason(s.getValue())
9+
else reason = "<no reason>"
10+
select s, reason

‎java/ql/test/library-tests/Encryption/secure.expected

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
| Test.java:18:4:18:8 | "AES" |
2-
| Test.java:19:4:19:17 | "AES_function" |
1+
| Test.java:22:4:22:8 | "AES" |
2+
| Test.java:23:4:23:17 | "AES_function" |

‎java/ql/test/query-tests/security/CWE-327/semmle/tests/BrokenCryptoAlgorithm.expected

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
#select
2-
| Test.java:19:20:19:50 | getInstance(...) | Test.java:19:45:19:49 | "DES" | Test.java:19:45:19:49 | "DES" | Cryptographic algorithm $@ is weak and should not be used. | Test.java:19:45:19:49 | "DES" | DES |
3-
| Test.java:42:14:42:38 | getInstance(...) | Test.java:42:33:42:37 | "RC2" | Test.java:42:33:42:37 | "RC2" | Cryptographic algorithm $@ is weak and should not be used. | Test.java:42:33:42:37 | "RC2" | RC2 |
2+
| Test.java:19:20:19:50 | getInstance(...) | Test.java:19:45:19:49 | "DES" | Test.java:19:45:19:49 | "DES" | Cryptographic algorithm $@ is insecure. It has a short key length of 56 bits, making it vulnerable to brute-force attacks. Consider using AES instead. | Test.java:19:45:19:49 | "DES" | DES |
3+
| Test.java:42:14:42:38 | getInstance(...) | Test.java:42:33:42:37 | "RC2" | Test.java:42:33:42:37 | "RC2" | Cryptographic algorithm $@ is insecure. It is vulnerable to related-key attacks. Consider using AES instead. | Test.java:42:33:42:37 | "RC2" | RC2 |
44
edges
55
nodes
66
| Test.java:19:45:19:49 | "DES" | semmle.label | "DES" |

0 commit comments

Comments
 (0)