diff --git a/java/src/qlpack.yml b/java/src/qlpack.yml index e6de8799..c17a4089 100644 --- a/java/src/qlpack.yml +++ b/java/src/qlpack.yml @@ -1,6 +1,6 @@ library: false name: githubsecuritylab/codeql-java-queries -version: 0.7.5 +version: 0.8.0 suites: suites defaultSuiteFile: suites/java.qls dependencies: diff --git a/java/src/security/CWE-338/HeuristicInsecureRandomness.java b/java/src/security/CWE-338/HeuristicInsecureRandomness.java new file mode 100644 index 00000000..0aa674fc --- /dev/null +++ b/java/src/security/CWE-338/HeuristicInsecureRandomness.java @@ -0,0 +1,23 @@ +import java.security.SecureRandom; +import java.util.Random; + +public class HeuristicInsecureRandomness { + + private static final int AUTHORIZATION_CODE_BOUND = 1_000_000; + + // BAD: java.util.Random is used to generate an authorization code. The method name + // "generateAuthorizationCode" indicates the result is security-sensitive. + public String generateAuthorizationCode(String username) { + Random predictableRandom = new Random(username.hashCode()); + int code = predictableRandom.nextInt(AUTHORIZATION_CODE_BOUND); + return String.format("%06d", code); + } + + // GOOD: java.security.SecureRandom is a cryptographically strong PRNG, so its output + // cannot be predicted even though the method name still looks security-sensitive. + public String generateAuthorizationCodeGood(String username) { + SecureRandom secureRandom = new SecureRandom(); + int code = secureRandom.nextInt(AUTHORIZATION_CODE_BOUND); + return String.format("%06d", code); + } +} diff --git a/java/src/security/CWE-338/HeuristicInsecureRandomness.qhelp b/java/src/security/CWE-338/HeuristicInsecureRandomness.qhelp new file mode 100644 index 00000000..a4b27928 --- /dev/null +++ b/java/src/security/CWE-338/HeuristicInsecureRandomness.qhelp @@ -0,0 +1,47 @@ + + + + +

Cryptographically weak pseudo-random number generators (PRNGs), such as + java.util.Random or Math.random(), produce output that an attacker + can predict or reproduce, for example by observing a few outputs or brute-forcing the seed. + This is not a problem in most contexts, but if the resulting value is used somewhere that + must be unpredictable, such as an authentication token, a password-reset code, an API key, or + a session identifier, an attacker who can predict the value may be able to bypass the security + control it was meant to protect.

+

This query is a heuristic: rather than tracing the random value all the way to a known + security-sensitive sink, it flags any value produced by a weak PRNG whose surrounding code + (the enclosing method or constructor name, the variable it is assigned to, or the variable + holding the Random instance) has a name that suggests a security-sensitive + purpose, such as token, secret, otp, or + authorizationCode. Review each result to confirm that the value is genuinely used + in a security-sensitive way.

+
+ + +

Use java.security.SecureRandom instead of java.util.Random or + Math.random() to generate any value that must be unpredictable.

+
+ + +

In the following example, an authorization code is generated using + java.util.Random. Because java.util.Random is seeded from a value + that is often predictable (or is easy to brute-force even when it is not), an attacker may be + able to predict or reproduce the generated code. The fixed version uses + java.security.SecureRandom instead.

+ +
+ + +
  • + OWASP: + Insecure Randomness +
  • +
  • + CWE: + CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG) +
  • +
    +
    diff --git a/java/src/security/CWE-338/HeuristicInsecureRandomness.ql b/java/src/security/CWE-338/HeuristicInsecureRandomness.ql new file mode 100644 index 00000000..3952c0bf --- /dev/null +++ b/java/src/security/CWE-338/HeuristicInsecureRandomness.ql @@ -0,0 +1,69 @@ +/** + * @name Heuristic insecure randomness + * @description Using a cryptographically weak pseudo-random number generator, such as + * `java.util.Random`, to produce a value whose surrounding names suggest it is + * security-sensitive may allow an attacker to predict that value. + * @kind problem + * @problem.severity recommendation + * @security-severity 3.7 + * @precision low + * @id githubsecuritylab/java/heuristic-insecure-randomness + * @tags security + * external/cwe/cwe-338 + */ + +import java +import semmle.code.java.security.RandomQuery +import semmle.code.java.security.InsecureRandomnessQuery + +/** + * Gets a "suspicious" name pattern that suggests a value is security-sensitive. + * + * This extends the `suspicious()` terms from + * `semmle.code.java.security.SensitiveActions` with additional terms that are commonly + * used to name values which must be unpredictable, such as tokens, secrets, keys, + * nonces, salts, and authorization or verification codes. + */ +private string sus() { + result = + [ + // Terms taken from the `suspicious()` predicate in `SensitiveActions.qll`. + "%password%", "%passwd%", "pwd", "%account%", "%accnt%", "%trusted%", "%refresh%token%", + "%secret%token", + // Additional terms for values that are expected to be unpredictable. + "%token%", "%secret%", "%nonce%", "%salt%", "%otp%", "%passcode%", "%passphrase%", + "%credential%", "%apikey%", "%api_key%", "%sessionid%", "%session_id%", "%csrf%", "%xsrf%", + "%secretkey%", "%privatekey%", "%signingkey%", "%encryptionkey%", "%authoriz%", "%authcode%", + "%verification%code%", "%verificationcode%", "%confirmation%code%", "%confirmationcode%", + "%security%code%", "%securitycode%", "%activation%code%", "%activationcode%", "%pincode%" + ] +} + +/** + * Gets a lowercased identifier name related to the random value produced at `source` + * that may indicate the value is used for a security-sensitive purpose. + */ +private string getASensitiveContextName(RandomDataSource source) { + // The name of the method or constructor that produces the random value. + result = source.getEnclosingCallable().getName().toLowerCase() + or + // The name of a variable that the random value is directly assigned to. + exists(Variable v | v.getAnAssignedValue() = source.getOutput() | + result = v.getName().toLowerCase() + ) + or + // The name of the variable holding the `Random` instance that produced the value. + exists(Variable v | source.getQualifier() = v.getAnAccess() | result = v.getName().toLowerCase()) +} + +from RandomDataSource source, string name +where + // Restrict to insecure sources of randomness (e.g. `java.util.Random`, `Math.random`), + // excluding safe implementations such as `java.security.SecureRandom`. + source.getOutput() = any(InsecureRandomnessSource s).asExpr() and + // Report each source once, using the alphabetically-first matching name. + name = min(string n | n = getASensitiveContextName(source) and n.matches(sus()) | n) +select source, + "Insecure randomness: this value is produced by a cryptographically weak random number generator, but '" + + name + + "' suggests it is used in a security-sensitive context. Use java.security.SecureRandom instead." diff --git a/java/test/security/CWE-338/HeuristicInsecureRandomness.expected b/java/test/security/CWE-338/HeuristicInsecureRandomness.expected new file mode 100644 index 00000000..3f315e1a --- /dev/null +++ b/java/test/security/CWE-338/HeuristicInsecureRandomness.expected @@ -0,0 +1,4 @@ +| HeuristicInsecureRandomness.java:19:16:19:66 | nextInt(...) | Insecure randomness: this value is produced by a cryptographically weak random number generator, but 'generateauthorizationcode' suggests it is used in a security-sensitive context. Use java.security.SecureRandom instead. | +| HeuristicInsecureRandomness.java:26:17:26:33 | nextInt(...) | Insecure randomness: this value is produced by a cryptographically weak random number generator, but 'token' suggests it is used in a security-sensitive context. Use java.security.SecureRandom instead. | +| HeuristicInsecureRandomness.java:33:13:33:37 | nextInt(...) | Insecure randomness: this value is produced by a cryptographically weak random number generator, but 'otpgenerator' suggests it is used in a security-sensitive context. Use java.security.SecureRandom instead. | +| HeuristicInsecureRandomness.java:39:21:39:33 | random(...) | Insecure randomness: this value is produced by a cryptographically weak random number generator, but 'secret' suggests it is used in a security-sensitive context. Use java.security.SecureRandom instead. | diff --git a/java/test/security/CWE-338/HeuristicInsecureRandomness.java b/java/test/security/CWE-338/HeuristicInsecureRandomness.java new file mode 100644 index 00000000..16933995 --- /dev/null +++ b/java/test/security/CWE-338/HeuristicInsecureRandomness.java @@ -0,0 +1,56 @@ +import java.security.SecureRandom; +import java.util.Random; + +/** + * Test cases for the `HeuristicInsecureRandomness` query. + * + * Methods prefixed with a security-sensitive name, or that assign the random value to a + * security-sensitive variable, are expected to be reported. Uses of `java.util.Random` + * in a non-sensitive context, and any use of `java.security.SecureRandom`, are not. + */ +public class HeuristicInsecureRandomness { + + private static final int AUTHORIZATION_CODE_BOUND = 1_000_000; + + // BAD: java.util.Random is used to generate an authorization code (the issue example). + // Flagged because the enclosing method name matches a suspicious term ("authoriz"). + public String generateAuthorizationCode(String username) { + Random predictableRandom = new Random(username.hashCode()); + int code = predictableRandom.nextInt(AUTHORIZATION_CODE_BOUND); + return String.format("%06d", code); + } + + // BAD: the random value is assigned to a variable named "token". + public int nextValue() { + Random rng = new Random(); + int token = rng.nextInt(1000); + return token; + } + + // BAD: the Random instance is stored in a variable named "otpGenerator". + public int compute() { + Random otpGenerator = new Random(); + int n = otpGenerator.nextInt(100); + return n; + } + + // BAD: Math.random() (which is backed by java.util.Random) produces a value named "secret". + public double weakSecret() { + double secret = Math.random(); + return secret; + } + + // GOOD: SecureRandom is a safe implementation, so it must not be flagged even though the + // method name and variable name both look security-sensitive. + public String generatePasswordResetToken() { + SecureRandom secureRandom = new SecureRandom(); + int token = secureRandom.nextInt(1_000_000); + return String.format("%06d", token); + } + + // GOOD: java.util.Random used in a context that is not security-sensitive. + public int rollDice() { + Random rng = new Random(); + return rng.nextInt(6); + } +} diff --git a/java/test/security/CWE-338/HeuristicInsecureRandomness.qlref b/java/test/security/CWE-338/HeuristicInsecureRandomness.qlref new file mode 100644 index 00000000..44ea6e9e --- /dev/null +++ b/java/test/security/CWE-338/HeuristicInsecureRandomness.qlref @@ -0,0 +1 @@ +query: security/CWE-338/HeuristicInsecureRandomness.ql