False Positive: BrokenCryptoAlgorithm.ql reports nearby string literals even when the cipher call itself stays on AES.
- Lenguaje dominante
- CodeQL
- Estrellas
- 10.1k
- Forks
- 2.1k
- Merge medio
- 2 d 15 h
- PR fusionados (30 d)
- 141
Descripción
Version
codeql 2.24.3
## Checker
- Checker id: `Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql`
- Checker description: This checker detects insecure cryptographic algorithms (e.g., DES, RC4, ECB mode) being used as the algorithm specification in cryptographic operations.
## Description of the false positive
These samples all keep the actual cryptographic operation on a safe algorithm. What changes is that insecure-looking strings appear somewhere nearby, or an intermediate computation happens to mention part of an insecure transformation name.
That should not be enough to report `Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql`. The rule should care about the transformation that reaches `Cipher.getInstance(...)`, not unrelated literals in the same method.
## Affected test cases
### `NegCase1_Var5.java`
This case requests `AES/CBC/PKCS5Padding`. There is no insecure algorithm at the sink, so this should stay unreported.
```java
// A program with no string literal matching the insecure cryptographic algorithm pattern should not be flagged as insecure crypto usage.
package scensct.var.neg;
public class NegCase1_Var5 {
// Variant 5: Lexical and idiomatic - different secure algorithm from same family
public static void main(String[] args) throws Exception {
// Using another secure algorithm: AES/CBC/PKCS5Padding (still secure if used with proper IV)
// This tests that the checker only flags insecure ones, not all algorithms.
String secureAlg = "AES/CBC/PKCS5Padding";
javax.crypto.Cipher cipher = javax.crypto.Cipher.getInstance(secureAlg); // [REPORTED LINE]
System.out.println("Cipher created with: " + secureAlg);
}
}
```
### `NegCase3.java`
This sample keeps an `RC4`-looking string around, but the cipher call still uses `AES/CBC/PKCS5Padding`. Reporting this would mean the query is matching on lexical proximity rather than the actual call argument.
```java
// A string literal matching the insecure pattern and a cryptographic call exist, but the call uses a different secure algorithm string should not be flagged as insecure crypto usage.
package scensct.core.neg;
public class NegCase3 {
public static void main(String[] args) throws Exception {
String unusedLiteral = "RC4_NOT_USED"; // Changed value to avoid matching insecure pattern
String secureAlg = "AES/CBC/PKCS5Padding"; // Different secure algorithm
javax.crypto.Cipher cipher = javax.crypto.Cipher.getInstance(secureAlg); // Crypto call uses secure string // [REPORTED LINE]
System.out.println("Insecure literal unused: " + unusedLiteral);
}
}
```
### `NegCase4_Var5.java`
Here the code derives `"DES"` from a string, but that value never becomes the transformation passed to `Cipher.getInstance(...)`. The live sink still uses AES, so this is not a real broken-crypto finding.
```java
// A string literal matching the insecure pattern flows along a path but does not reach the algorithm parameter of a cryptographic operation should not be flagged as insecure crypto usage.
package scensct.var.neg;
public class NegCase4_Var5 {
public static void main(String[] args) throws Exception {
String alg = "DES/ECB/PKCS5Padding";
String extracted = null;
try {
extracted = alg.toUpperCase().substring(0, 3);
} catch (RuntimeException e) {
// ignore, won't happen
}
javax.crypto.Cipher cipher = javax.crypto.Cipher.getInstance("AES/CBC/PKCS5Padding"); // [REPORTED LINE]
System.out.println("Extracted: " + extracted);
}
}
```
## Cause analysis
The common failure mode is that the query seems to treat suspicious string constants as if they were equivalent to the transformation that reaches the crypto API. That is too loose for `Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql`.
In all three samples, the value at `Cipher.getInstance(...)` is still a safe AES transformation. The reported issue only appears if the query keeps too much context from nearby literals or partial string computations instead of following the exact argument that reaches the sink.
Guía de contribución
Línea de trabajo
Start with Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql and inspect how the query determines the transformation reaching Cipher.getInstance(...). Reproduce the reports with NegCase1_Var5.java, NegCase3.java, and NegCase4_Var5.java, then verify that safe AES calls are no longer reported while genuine insecure transformations remain detected.
Escrito por el modelo de indexación a partir del texto del issue.
Evaluación
- Stack tecnológico
- java
- Área
- security
- Tipo de issue
- Error
- Dificultad
- 3/5
- Tiempo estimado
- 1-2 días
- Estado de actividad
- Tranquilo
- Claridad
- Bastante claro
- Aptitud para principiantes
- 52/100