False Positive: BrokenCryptoAlgorithm.ql reports nearby string literals even when the cipher call itself stays on AES.

從 Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql 開始，檢查查詢如何判定到達 Cipher.getInstance(...) 的轉換。使用 NegCase1_Var5.java、NegCase3.java 和 NegCase4_Var5.java 重現報告，然後驗證安全的 AES 呼叫不再被報告，同時真正不安全的轉換仍會被偵測到。

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.

// 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.

// 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.

// 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.