Skip to content

Commit 400d4c4

Browse files
committed
fix(java): match mixed-case weak crypto algorithm strings
1 parent 96852da commit 400d4c4

5 files changed

Lines changed: 26 additions & 4 deletions

File tree

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The `java/weak-cryptographic-algorithm` and `java/potentially-weak-cryptographic-algorithm` queries now match cryptographic algorithm names and modes case-insensitively (for example, `AES/CBC/PKCS5PADDING` and `AES/cbc/Pkcs5Padding`).

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ class BrokenAlgoLiteral extends ShortStringLiteral {
1717
BrokenAlgoLiteral() {
1818
this.getValue().regexpMatch(getInsecureAlgorithmRegex()) and
1919
// Exclude RSA/ECB/.* ciphers.
20-
not this.getValue().regexpMatch("RSA/ECB.*") and
20+
not this.getValue().regexpMatch("(?i)RSA/ECB.*") and
2121
// Exclude German and French sentences.
2222
not this.getValue().regexpMatch(".*\\p{IsLowercase} des \\p{IsLetter}.*")
2323
}

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -189,15 +189,18 @@ bindingset[algorithmString]
189189
private string algorithmRegex(string algorithmString) {
190190
// Algorithms usually appear in names surrounded by characters that are not
191191
// alphabetical characters in the same case. This handles the upper and lower
192-
// case cases.
192+
// case cases, plus mixed-case algorithm strings bounded by non-alphabetic
193+
// characters (such as `AES/cbc/Pkcs5Padding`).
193194
result =
194195
"((^|.*[^A-Z])(" + algorithmString + ")([^A-Z].*|$))" +
195196
// or...
196197
"|" +
197198
// For lowercase, we want to be careful to avoid being confused by camelCase
198199
// hence we require two preceding uppercase letters to be sure of a case switch,
199200
// or a preceding non-alphabetic character
200-
"((^|.*[A-Z]{2}|.*[^a-zA-Z])(" + algorithmString.toLowerCase() + ")([^a-z].*|$))"
201+
"((^|.*[A-Z]{2}|.*[^a-zA-Z])(" + algorithmString.toLowerCase() + ")([^a-z].*|$))" +
202+
// or...
203+
"|" + "((^|.*[^a-zA-Z])(?i)(" + algorithmString + ")([^a-zA-Z].*|$))"
201204
}
202205

203206
/**
@@ -258,7 +261,7 @@ string getInsecureAlgorithmReason(string input) {
258261
string getASecureAlgorithmName() {
259262
result =
260263
[
261-
"RSA", "SHA-?(256|384|512)", "CCM", "GCM", "AES(?![^a-zA-Z](ECB|CBC/PKCS[57]Padding))",
264+
"RSA", "SHA-?(256|384|512)", "CCM", "GCM", "AES(?![^a-zA-Z](?i)(ECB|CBC/PKCS[57]Padding))",
262265
"Blowfish", "ECIES", "SHA3-(256|384|512)",
263266
// Elliptic Curve algorithms: EC (key generation), ECDSA (signatures), ECDH (key agreement),
264267
// EdDSA/Ed25519/Ed448 (Edwards-curve signatures), XDH/X25519/X448 (key agreement).
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,16 @@
11
#select
22
| 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 |
33
| 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 |
4+
| Test.java:46:14:46:55 | getInstance(...) | Test.java:46:33:46:54 | "AES/CBC/PKCS5Padding" | Test.java:46:33:46:54 | "AES/CBC/PKCS5Padding" | Cryptographic algorithm $@ is insecure. CBC mode with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using GCM instead. | Test.java:46:33:46:54 | "AES/CBC/PKCS5Padding" | AES/CBC/PKCS5Padding |
5+
| Test.java:47:14:47:55 | getInstance(...) | Test.java:47:33:47:54 | "AES/CBC/PKCS5PADDING" | Test.java:47:33:47:54 | "AES/CBC/PKCS5PADDING" | Cryptographic algorithm $@ is insecure. CBC mode with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using GCM instead. | Test.java:47:33:47:54 | "AES/CBC/PKCS5PADDING" | AES/CBC/PKCS5PADDING |
6+
| Test.java:48:14:48:55 | getInstance(...) | Test.java:48:33:48:54 | "aes/cbc/pkcs5padding" | Test.java:48:33:48:54 | "aes/cbc/pkcs5padding" | Cryptographic algorithm $@ is insecure. CBC mode with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using GCM instead. | Test.java:48:33:48:54 | "aes/cbc/pkcs5padding" | aes/cbc/pkcs5padding |
7+
| Test.java:49:14:49:55 | getInstance(...) | Test.java:49:33:49:54 | "AES/cbc/Pkcs5Padding" | Test.java:49:33:49:54 | "AES/cbc/Pkcs5Padding" | Cryptographic algorithm $@ is insecure. CBC mode with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using GCM instead. | Test.java:49:33:49:54 | "AES/cbc/Pkcs5Padding" | AES/cbc/Pkcs5Padding |
48
edges
59
nodes
610
| Test.java:19:45:19:49 | "DES" | semmle.label | "DES" |
711
| Test.java:42:33:42:37 | "RC2" | semmle.label | "RC2" |
12+
| Test.java:46:33:46:54 | "AES/CBC/PKCS5Padding" | semmle.label | "AES/CBC/PKCS5Padding" |
13+
| Test.java:47:33:47:54 | "AES/CBC/PKCS5PADDING" | semmle.label | "AES/CBC/PKCS5PADDING" |
14+
| Test.java:48:33:48:54 | "aes/cbc/pkcs5padding" | semmle.label | "aes/cbc/pkcs5padding" |
15+
| Test.java:49:33:49:54 | "AES/cbc/Pkcs5Padding" | semmle.label | "AES/cbc/Pkcs5Padding" |
816
subpaths

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,13 @@ public void test() {
4141
// BAD: RC2 is a weak algorithm
4242
cipher = Cipher.getInstance("RC2"); // $ Alert[java/weak-cryptographic-algorithm]
4343
}
44+
{
45+
// BAD: AES/CBC/PKCS5Padding is vulnerable to padding oracle attacks, regardless of case
46+
cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); // $ Alert[java/weak-cryptographic-algorithm]
47+
cipher = Cipher.getInstance("AES/CBC/PKCS5PADDING"); // $ Alert[java/weak-cryptographic-algorithm]
48+
cipher = Cipher.getInstance("aes/cbc/pkcs5padding"); // $ Alert[java/weak-cryptographic-algorithm]
49+
cipher = Cipher.getInstance("AES/cbc/Pkcs5Padding"); // $ Alert[java/weak-cryptographic-algorithm]
50+
}
4451
// GOOD: ECIES is a strong algorithm
4552
cipher = Cipher.getInstance("ECIES");
4653
cipher.init(Cipher.ENCRYPT_MODE, secretKeySpec);

0 commit comments

Comments
 (0)