CodeQL incorrect documentation encourages vulnerable Cryptographic Java API Usage

[LordAmit]

Hi,

My team is conducting academic research on Java Cryptography API based misuse using your tool. We found that we could not detect some potential cryptographic misuses, as CodeQL (And LGTM) has incorrect documentation (and rule) that encourages Java cryptographic API misuse resulting in vulnerabilities.

In https://lgtm.com/rules/7870098/ and https://github.com/github/codeql/blob/main/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.java, it is stated that "Using broken or weak cryptographic algorithms can allow an attacker to compromise security." and two examples are given, one using "DES", and the other using "AES".

However, in the case of SunJCE provider, the Cipher.getInstance("AES") and Cipher.getInstance("AES/ECB/PKCS5Padding") are equivalent as documented in Oracle's Java Cryptography Architecture (JCA) Reference Guide (https://docs.oracle.com/en/java/javase/11/security/java-cryptography-architecture-jca-reference-guide.html#GUID-2BCFDD85-D533-4E6C-8CE9-29990DEB0190). Note that this is the default provider for many Java Developers.

ECB mode is considered insecure as this is vulnerable to simple attacks such as frequency analysis (https://rules.sonarsource.com/java/tag/cwe).

We also found that in the query file Encryption.ql for java, ECB mode is not mentioned under insecure algorithm names (

codeql/java/ql/src/semmle/code/java/security/Encryption.qll

Line 95 in 768e519

string getAnInsecureAlgorithmName() {

)

All of these will result in developers being encouraged to use "AES" with "ECB" mode for encryption when they use LGTM/CodeQL, resulting in cryptographic vulnerabilities.

[aschackmull]

Thanks for reporting this. Should be fixed by #4833.

[intrigus-lgtm]

@aschackmull
I don't think #4833 fully fixed this.
https://github.com/github/codeql/blob/main/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.java
still recommends to use Cipher.getInstance("AES") which by default will choose Cipher.getInstance("AES/ECB/PKCS5Padding") (at least on most JREs).
And "AES/ECB/PKCS5Padding"is not considered secure.

[LordAmit]

Indeed - I do not consider this to be fixed, only partially addressed as the documentation continues to encourage crypto API misuse, even though the queries are being changed.

[github-actions]

This issue is stale because it has been open 14 days with no activity. Comment or remove the stale label in order to avoid having this issue closed in 7 days.