diff --git a/java/ql/lib/ext/java.security.spec.model.yml b/java/ql/lib/ext/java.security.spec.model.yml index 2318fa11f917..adb62372a824 100644 --- a/java/ql/lib/ext/java.security.spec.model.yml +++ b/java/ql/lib/ext/java.security.spec.model.yml @@ -3,6 +3,11 @@ extensions: pack: codeql/java-all extensible: sinkModel data: + - ["java.security.spec", "DSAPrivateKeySpec", False, "DSAPrivateKeySpec", "", "", "Argument[0..3]", "credentials-key", "manual"] + - ["java.security.spec", "ECPrivateKeySpec", False, "ECPrivateKeySpec", "", "", "Argument[0]", "credentials-key", "manual"] - ["java.security.spec", "EncodedKeySpec", False, "EncodedKeySpec", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"] - ["java.security.spec", "PKCS8EncodedKeySpec", False, "PKCS8EncodedKeySpec", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"] + - ["java.security.spec", "RSAMultiPrimePrivateCrtKeySpec", False, "RSAMultiPrimePrivateCrtKeySpec", "", "", "Argument[2]", "credentials-key", "manual"] + - ["java.security.spec", "RSAPrivateCrtKeySpec", False, "RSAPrivateCrtKeySpec", "", "", "Argument[2]", "credentials-key", "manual"] + - ["java.security.spec", "RSAPrivateKeySpec", False, "RSAPrivateKeySpec", "", "", "Argument[1]", "credentials-key", "manual"] - ["java.security.spec", "X509EncodedKeySpec", False, "X509EncodedKeySpec", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"] diff --git a/java/ql/lib/ext/javax.crypto.spec.model.yml b/java/ql/lib/ext/javax.crypto.spec.model.yml index 9bc4f3cc1746..0f879c1f900c 100644 --- a/java/ql/lib/ext/javax.crypto.spec.model.yml +++ b/java/ql/lib/ext/javax.crypto.spec.model.yml @@ -11,9 +11,6 @@ extensions: pack: codeql/java-all extensible: sinkModel data: - - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "(char[])", "", "Argument[0]", "credentials-password", "hq-generated"] - - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "(char[],byte[],int)", "", "Argument[0]", "credentials-password", "hq-generated"] - - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "(char[],byte[],int,int)", "", "Argument[0]", "credentials-password", "hq-generated"] - ["javax.crypto.spec", "DESKeySpec", False, "DESKeySpec", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"] - ["javax.crypto.spec", "DESKeySpec", False, "DESKeySpec", "(byte[],int)", "", "Argument[0]", "credentials-key", "hq-generated"] - ["javax.crypto.spec", "DESKeySpec", False, "isParityAdjusted", "(byte[],int)", "", "Argument[0]", "credentials-key", "hq-generated"] @@ -21,5 +18,11 @@ extensions: - ["javax.crypto.spec", "DESedeKeySpec", False, "DESedeKeySpec", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"] - ["javax.crypto.spec", "DESedeKeySpec", False, "DESedeKeySpec", "(byte[],int)", "", "Argument[0]", "credentials-key", "hq-generated"] - ["javax.crypto.spec", "DESedeKeySpec", False, "isParityAdjusted", "(byte[],int)", "", "Argument[0]", "credentials-key", "hq-generated"] + - ["javax.crypto.spec", "DHPrivateKeySpec", False, "DHPrivateKeySpec", "", "", "Argument[0]", "credentials-key", "manual"] + - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "", "", "Argument[1]", "encryption-salt", "manual"] + - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "(char[])", "", "Argument[0]", "credentials-password", "hq-generated"] + - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "(char[],byte[],int)", "", "Argument[0]", "credentials-password", "hq-generated"] + - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "(char[],byte[],int,int)", "", "Argument[0]", "credentials-password", "hq-generated"] + - ["javax.crypto.spec", "PBEParameterSpec", False, "PBEParameterSpec", "", "", "Argument[0]", "encryption-salt", "manual"] - ["javax.crypto.spec", "SecretKeySpec", False, "SecretKeySpec", "(byte[],String)", "", "Argument[0]", "credentials-key", "hq-generated"] - ["javax.crypto.spec", "SecretKeySpec", False, "SecretKeySpec", "(byte[],int,int,String)", "", "Argument[0]", "credentials-key", "hq-generated"] diff --git a/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll new file mode 100644 index 000000000000..d24d44e3805d --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll @@ -0,0 +1,97 @@ +/** Provides classes and predicates for reasoning about insecure randomness. */ + +import java +private import semmle.code.java.frameworks.Servlets +private import semmle.code.java.security.SensitiveActions +private import semmle.code.java.security.SensitiveApi +private import semmle.code.java.dataflow.TaintTracking +private import semmle.code.java.dataflow.ExternalFlow +private import semmle.code.java.security.RandomQuery + +/** + * A node representing a source of insecure randomness. + * + * For example, use of `java.util.Random` or `java.lang.Math.random`. + */ +abstract class InsecureRandomnessSource extends DataFlow::Node { } + +private class RandomMethodSource extends InsecureRandomnessSource { + RandomMethodSource() { + exists(RandomDataSource s | this.asExpr() = s.getOutput() | + not s.getQualifier().getType() instanceof SafeRandomImplementation + ) + } +} + +/** + * A type which is an implementation of `java.util.Random` but considered to be safe. + * + * For example, `java.security.SecureRandom`. + */ +abstract private class SafeRandomImplementation extends RefType { } + +private class TypeSecureRandom extends SafeRandomImplementation instanceof SecureRandomNumberGenerator +{ } + +private class TypeHadoopOsSecureRandom extends SafeRandomImplementation { + TypeHadoopOsSecureRandom() { + this.hasQualifiedName("org.apache.hadoop.crypto.random", "OsSecureRandom") + } +} + +/** + * A node representing an operation which should not use a Insecurely random value. + */ +abstract class InsecureRandomnessSink extends DataFlow::Node { } + +/** + * A node which sets the value of a cookie. + */ +private class CookieSink extends InsecureRandomnessSink { + CookieSink() { + exists(Call c | + c.(ClassInstanceExpr).getConstructedType() instanceof TypeCookie and + this.asExpr() = c.getArgument(1) + or + c.(MethodCall).getMethod().getDeclaringType() instanceof TypeCookie and + c.(MethodCall).getMethod().hasName("setValue") and + this.asExpr() = c.getArgument(0) + ) + } +} + +private class SensitiveActionSink extends InsecureRandomnessSink { + SensitiveActionSink() { this.asExpr() instanceof SensitiveExpr } +} + +private class CredentialsSink extends InsecureRandomnessSink instanceof CredentialsSinkNode { } + +/** + * A taint-tracking configuration for Insecure randomness. + */ +module InsecureRandomnessConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src instanceof InsecureRandomnessSource } + + predicate isSink(DataFlow::Node sink) { sink instanceof InsecureRandomnessSink } + + predicate isBarrierIn(DataFlow::Node n) { isSource(n) } + + predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { + n1.asExpr() = n2.asExpr().(BinaryExpr).getAnOperand() + or + n1.asExpr() = n2.asExpr().(UnaryExpr).getExpr() + or + exists(MethodCall mc, string methodName | + mc.getMethod().hasQualifiedName("org.owasp.esapi", "Encoder", methodName) and + methodName.matches("encode%") + | + n1.asExpr() = mc.getArgument(0) and + n2.asExpr() = mc + ) + } +} + +/** + * Taint-tracking flow of a Insecurely random value into a sensitive sink. + */ +module InsecureRandomnessFlow = TaintTracking::Global; diff --git a/java/ql/lib/semmle/code/java/security/RandomDataSource.qll b/java/ql/lib/semmle/code/java/security/RandomDataSource.qll index 7d67032e3925..05ba28ba450c 100644 --- a/java/ql/lib/semmle/code/java/security/RandomDataSource.qll +++ b/java/ql/lib/semmle/code/java/security/RandomDataSource.qll @@ -107,6 +107,15 @@ class StdlibRandomSource extends RandomDataSource { } } +/** + * A method access calling the `random` of `java.lang.Math`. + */ +class MathRandomSource extends RandomDataSource { + MathRandomSource() { this.getMethod().hasQualifiedName("java.lang", "Math", "random") } + + override Expr getOutput() { result = this } +} + /** * A method access calling a method declared on `org.apache.commons.lang3.RandomUtils` * that returns random data or writes random data to an argument. @@ -143,3 +152,19 @@ class ApacheCommonsRandomSource extends RandomDataSource { override Expr getOutput() { result = this } } + +/** + * A method access calling a method declared on `org.apache.commons.lang3.RandomStringUtils` + */ +class ApacheCommonsRandomStringSource extends RandomDataSource { + ApacheCommonsRandomStringSource() { + exists(Method m | m = this.getMethod() | + m.getName().matches("random%") and + m.getDeclaringType() + .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], + "RandomStringUtils") + ) + } + + override Expr getOutput() { result = this } +} diff --git a/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp b/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp new file mode 100644 index 000000000000..414e32520577 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp @@ -0,0 +1,64 @@ + + + +

+ If you use a cryptographically weak pseudo-random number generator to generate security-sensitive values, + such as passwords, attackers can more easily predict those values. +

+

+ Pseudo-random number generators generate a sequence of numbers that only approximates the properties + of random numbers. The sequence is not truly random because it is completely determined by a + relatively small set of initial values (the seed). If the random number generator is + cryptographically weak, then this sequence may be easily predictable through outside observations. +

+ + + +

+ The java.util.Random random number generator is not cryptographically secure. Use a secure random number generator such as java.security.SecureRandom instead. +

+

+ Use a cryptographically secure pseudo-random number generator if the output is to be used in a + security-sensitive context. As a general rule, a value should be considered "security-sensitive" + if predicting it would allow the attacker to perform an action that they would otherwise be unable + to perform. For example, if an attacker could predict the random password generated for a new user, + they would be able to log in as that new user. +

+ + + + +

+ The following examples show different ways of generating a cookie with a random value. +

+ +

+ In the first (BAD) case, we generate a fresh cookie by appending a random integer to the end of a static + string. The random number generator used (Random) is not cryptographically secure, + so it may be possible for an attacker to predict the generated cookie. +

+ + + +

+ In the second (GOOD) case, we generate a fresh cookie by appending a random integer to the end of a static + string. The random number generator used (SecureRandom) is cryptographically secure, + so it is not possible for an attacker to predict the generated cookie. +

+ + + + + + +
  • Wikipedia: Pseudo-random number generator.
    • +
  • + Java Docs: Random. +
    • +
  • + Java Docs: SecureRandom. +
    • +     +     diff --git a/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.ql b/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.ql new file mode 100644 index 000000000000..2b916fef1b6b --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.ql @@ -0,0 +1,23 @@ +/** + * @name Insecure randomness + * @description Using a cryptographically Insecure pseudo-random number generator to generate a + * security-sensitive value may allow an attacker to predict what value will + * be generated. + * @kind path-problem + * @problem.severity warning + * @security-severity 7.8 + * @precision high + * @id java/insecure-randomness + * @tags security + * external/cwe/cwe-330 + * external/cwe/cwe-338 + */ + +import java +import semmle.code.java.security.InsecureRandomnessQuery +import InsecureRandomnessFlow::PathGraph + +from InsecureRandomnessFlow::PathNode source, InsecureRandomnessFlow::PathNode sink +where InsecureRandomnessFlow::flowPath(source, sink) +select sink.getNode(), source, sink, "Potential Insecure randomness due to a $@.", source.getNode(), + "Insecure randomness source." diff --git a/java/ql/src/Security/CWE/CWE-330/examples/InsecureRandomnessCookie.java b/java/ql/src/Security/CWE/CWE-330/examples/InsecureRandomnessCookie.java new file mode 100644 index 000000000000..151f5cddc298 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-330/examples/InsecureRandomnessCookie.java @@ -0,0 +1,9 @@ +Random r = new Random(); + +byte[] bytes = new byte[16]; +r.nextBytes(bytes); + +String cookieValue = encode(bytes); + +Cookie cookie = new Cookie("name", cookieValue); +response.addCookie(cookie); diff --git a/java/ql/src/Security/CWE/CWE-330/examples/SecureRandomnessCookie.java b/java/ql/src/Security/CWE/CWE-330/examples/SecureRandomnessCookie.java new file mode 100644 index 000000000000..62395a7f0863 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-330/examples/SecureRandomnessCookie.java @@ -0,0 +1,9 @@ +SecureRandom r = new SecureRandom(); + +byte[] bytes = new byte[16]; +r.nextBytes(bytes); + +String cookieValue = encode(bytes); + +Cookie cookie = new Cookie("name", cookieValue); +response.addCookie(cookie); diff --git a/java/ql/src/change-notes/2023-11-08-weak-randomness-query.md b/java/ql/src/change-notes/2023-11-08-weak-randomness-query.md new file mode 100644 index 000000000000..9022f825af6e --- /dev/null +++ b/java/ql/src/change-notes/2023-11-08-weak-randomness-query.md @@ -0,0 +1,5 @@ +--- +category: newQuery +--- +* Added the `java/insecure-randomness` query to detect uses of weakly random values which an attacker may be able to predict. Also added the `crypto-parameter` sink kind for sinks which represent the parameters and keys of cryptographic operations. + diff --git a/java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.expected b/java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.expected new file mode 100644 index 000000000000..48de9172b362 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.expected @@ -0,0 +1,2 @@ +failures +testFailures diff --git a/java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.ql b/java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.ql new file mode 100644 index 000000000000..a2b6f329ae80 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.ql @@ -0,0 +1,19 @@ +import java +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.security.InsecureRandomnessQuery +import TestUtilities.InlineExpectationsTest + +module WeakRandomTest implements TestSig { + string getARelevantTag() { result = "hasWeakRandomFlow" } + + predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasWeakRandomFlow" and + exists(DataFlow::Node sink | InsecureRandomnessFlow::flowTo(sink) | + sink.getLocation() = location and + element = sink.toString() and + value = "" + ) + } +} + +import MakeTest diff --git a/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java b/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java new file mode 100644 index 000000000000..514783c95fb8 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java @@ -0,0 +1,61 @@ +import java.io.IOException; +import java.util.Random; +import java.util.concurrent.ThreadLocalRandom; +import java.security.SecureRandom; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.Cookie; +import org.apache.commons.lang3.RandomStringUtils; +import org.owasp.esapi.Encoder; + +public class WeakRandomCookies extends HttpServlet { + HttpServletResponse response; + + public void doGet() { + Random r = new Random(); + + int c = r.nextInt(); + // BAD: The cookie value may be predictable. + Cookie cookie = new Cookie("name", Integer.toString(c)); // $hasWeakRandomFlow + + Encoder enc = null; + int c2 = r.nextInt(); + String value = enc.encodeForHTML(Integer.toString(c2)); + // BAD: The cookie value may be predictable. + Cookie cookie2 = new Cookie("name", value); // $hasWeakRandomFlow + + byte[] bytes = new byte[16]; + r.nextBytes(bytes); + // BAD: The cookie value may be predictable. + Cookie cookie3 = new Cookie("name", new String(bytes)); // $hasWeakRandomFlow + + SecureRandom sr = new SecureRandom(); + + byte[] bytes2 = new byte[16]; + sr.nextBytes(bytes2); + // GOOD: The cookie value is unpredictable. + Cookie cookie4 = new Cookie("name", new String(bytes2)); + + ThreadLocalRandom tlr = ThreadLocalRandom.current(); + + Cookie cookie5 = new Cookie("name", Integer.toString(tlr.nextInt())); // $hasWeakRandomFlow + + Cookie cookie6 = new Cookie("name", RandomStringUtils.random(10)); // $hasWeakRandomFlow + + Cookie cookie7 = new Cookie("name", RandomStringUtils.randomAscii(10)); // $hasWeakRandomFlow + + long c3 = r.nextLong(); + // BAD: The cookie value may be predictable. + Cookie cookie8 = new Cookie("name", Long.toString(c3 * 5)); // $hasWeakRandomFlow + + double c4 = Math.random(); + // BAD: The cookie value may be predictable. + Cookie cookie9 = new Cookie("name", Double.toString(c4)); // $hasWeakRandomFlow + + double c5 = Math.random(); + // BAD: The cookie value may be predictable. + Cookie cookie10 = new Cookie("name", Double.toString(++c5)); // $hasWeakRandomFlow + } +} diff --git a/java/ql/test/query-tests/security/CWE-330/options b/java/ql/test/query-tests/security/CWE-330/options new file mode 100644 index 000000000000..5977a9ee7105 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-330/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/servlet-api-2.4:${testdir}/../../../stubs/apache-commons-lang3-3.7:${testdir}/../../../stubs/esapi-2.0.1 \ No newline at end of file diff --git a/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/Encoder.java b/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/Encoder.java index e0200207e760..dd4ba8b95ad8 100644 --- a/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/Encoder.java +++ b/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/Encoder.java @@ -2,4 +2,6 @@ public interface Encoder { String encodeForLDAP(String input); + + String encodeForHTML(String untrustedData); } diff --git a/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/reference/DefaultEncoder.java b/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/reference/DefaultEncoder.java index 8a1169f073a4..3713bde31877 100644 --- a/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/reference/DefaultEncoder.java +++ b/java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/reference/DefaultEncoder.java @@ -5,4 +5,5 @@ public class DefaultEncoder implements Encoder { public static Encoder getInstance() { return null; } public String encodeForLDAP(String input) { return null; } + public String encodeForHTML(String untrustedData) { return null; } }