From 9f986ca527361e5109c3bebad40e52689fe7717b Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Thu, 15 Jun 2023 11:19:59 -0400 Subject: [PATCH 01/29] Add Weak Randomness Query --- .../Security/CWE/CWE-330/WeakRandomness.qhelp | 65 ++++++++++ .../Security/CWE/CWE-330/WeakRandomness.ql | 113 ++++++++++++++++++ .../examples/InsecureRandomnessCookie.java | 9 ++ .../examples/SecureRandomnessCookie.java | 9 ++ 4 files changed, 196 insertions(+) create mode 100644 java/ql/src/Security/CWE/CWE-330/WeakRandomness.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql create mode 100644 java/ql/src/Security/CWE/CWE-330/examples/InsecureRandomnessCookie.java create mode 100644 java/ql/src/Security/CWE/CWE-330/examples/SecureRandomnessCookie.java diff --git a/java/ql/src/Security/CWE/CWE-330/WeakRandomness.qhelp b/java/ql/src/Security/CWE/CWE-330/WeakRandomness.qhelp new file mode 100644 index 000000000000..2de585b753c7 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-330/WeakRandomness.qhelp @@ -0,0 +1,65 @@ + + + +

+ Using a cryptographically weak pseudo-random number generator to generate a security-sensitive value, + such as a password, makes it easier for an attacker to predict the value. +

+

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

+ + + +

+ Use a cryptographically secure pseudo-random number generator if the output is to be used in a + security-sensitive context. As a rule of thumb, 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. +

+ +

+ For Java, java.util.Random is not cryptographically secure. Use java.security.SecureRandom instead. +

+ + + + +

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

+ +

+ In the first 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 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/WeakRandomness.ql b/java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql new file mode 100644 index 000000000000..f590c6abd0fd --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql @@ -0,0 +1,113 @@ +/** + * @name Weak Randomness + * @description Using a weak source of randomness may allow an attacker to predict the generated values. + * @kind path-problem + * @problem.severity error + * @security-severity 8.6 + * @precision high + * @id java/weak-randomness + * @tags security + * external/cwe/cwe-330 + * external/cwe/cwe-338 + */ + +import java +import semmle.code.java.frameworks.Servlets +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.RandomQuery +import WeakRandomnessFlow::PathGraph + +/** + * The `java.util.Random` class. + */ +class TypeRandom extends RefType { + TypeRandom() { this.hasQualifiedName("java.util", "Random") } +} + +abstract class WeakRandomnessSource extends DataFlow::Node { } + +private class JavaRandomSource extends WeakRandomnessSource { + JavaRandomSource() { + this.asExpr().getType() instanceof TypeRandom and this.asExpr() instanceof ConstructorCall + } +} + +private class MathRandomMethodAccess extends WeakRandomnessSource { + MathRandomMethodAccess() { + exists(MethodAccess ma | this.asExpr() = ma | + ma.getMethod().hasName("random") and + ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Math") + ) + } +} + +abstract private class SafeRandomImplementation extends RefType { } + +private class TypeSecureRandom extends SafeRandomImplementation { + TypeSecureRandom() { this.hasQualifiedName("java.security", "SecureRandom") } +} + +private class TypeHadoopOsSecureRandom extends SafeRandomImplementation { + TypeHadoopOsSecureRandom() { + this.hasQualifiedName("org.apache.hadoop.crypto.random", "OsSecureRandom") + } +} + +abstract class WeakRandomnessAdditionalTaintStep extends Unit { + abstract predicate step(DataFlow::Node node1, DataFlow::Node node2); +} + +abstract class WeakRandomnessSink extends DataFlow::Node { } + +private class CookieSink extends WeakRandomnessSink { + CookieSink() { + this.asExpr().getType() instanceof TypeCookie and + exists(MethodAccess ma | ma.getMethod().hasName("addCookie") | + ma.getArgument(0) = this.asExpr() + ) + } +} + +/** + * Holds if there is a method access which converts `bytes` to the string `str`. + */ +private predicate covertsBytesToString(DataFlow::Node bytes, DataFlow::Node str) { + bytes.getType().(Array).getElementType().(PrimitiveType).hasName("byte") and + str.getType() instanceof TypeString and + exists(MethodAccess ma | ma = str.asExpr() | bytes.asExpr() = ma.getAnArgument()) +} + +/** + * A taint-tracking configuration for weak randomness. + */ +module WeakRandomnessConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src instanceof WeakRandomnessSource } + + predicate isSink(DataFlow::Node sink) { sink instanceof WeakRandomnessSink } + + predicate isBarrier(DataFlow::Node n) { n.getTypeBound() instanceof SafeRandomImplementation } + + predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess ma, Method m | + n1.asExpr() = ma.getQualifier() and + ma.getMethod() = m and + m.getDeclaringType() instanceof TypeRandom and + ( + m.hasName(["nextInt", "nextLong", "nextFloat", "nextDouble", "nextBoolean", "nextGaussian"]) and + n2.asExpr() = ma + or + m.hasName("nextBytes") and + n2.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = ma.getArgument(0) + ) + ) + or + covertsBytesToString(n1, n2) + } +} + +module WeakRandomnessFlow = TaintTracking::Global; + +from WeakRandomnessFlow::PathNode source, WeakRandomnessFlow::PathNode sink +where WeakRandomnessFlow::flowPath(source, sink) +select sink.getNode(), source, sink, "Potential weak randomness due to a $@.", source.getNode(), + "weak 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); From e69ff7b601cde09a84dd68f633181ac69244c6cf Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 28 Jun 2023 12:36:16 -0400 Subject: [PATCH 02/29] Move to library and add docs --- .../java/security/WeakRandomnessQuery.qll | 128 ++++++++++++++++++ .../Security/CWE/CWE-330/WeakRandomness.ql | 94 +------------ 2 files changed, 129 insertions(+), 93 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll new file mode 100644 index 000000000000..abb226453c0f --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -0,0 +1,128 @@ +/** Provides classes and predicates for reasoning about weak randomness. */ + +import java +import semmle.code.java.frameworks.Servlets +import semmle.code.java.security.SensitiveActions +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.RandomQuery + +/** + * The `java.util.Random` class. + */ +class TypeRandom extends RefType { + TypeRandom() { this.hasQualifiedName("java.util", "Random") } +} + +/** + * A node representing a source of weak randomness. + * + * For example, use of `java.util.Random` or `java.lang.Math.random`. + */ +abstract class WeakRandomnessSource extends DataFlow::Node { } + +/** + * A node representing a call to a constructor of `java.util.Random`. + */ +private class JavaRandomSource extends WeakRandomnessSource { + JavaRandomSource() { + this.asExpr().getType() instanceof TypeRandom and this.asExpr() instanceof ConstructorCall + } +} + +/** + * The `random` method of `java.lang.Math`. + */ +private class MathRandomMethodAccess extends WeakRandomnessSource { + MathRandomMethodAccess() { + exists(MethodAccess ma | this.asExpr() = ma | + ma.getMethod().hasName("random") and + ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Math") + ) + } +} + +/** + * 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 { + TypeSecureRandom() { this.hasQualifiedName("java.security", "SecureRandom") } +} + +private class TypeHadoopOsSecureRandom extends SafeRandomImplementation { + TypeHadoopOsSecureRandom() { + this.hasQualifiedName("org.apache.hadoop.crypto.random", "OsSecureRandom") + } +} + +/** + * A node representing an operation which should not use a weakly random value. + */ +abstract class WeakRandomnessSink extends DataFlow::Node { } + +/** + * A node which creates a cookie. + */ +private class CookieSink extends WeakRandomnessSink { + CookieSink() { + this.asExpr().getType() instanceof TypeCookie and + exists(MethodAccess ma | ma.getMethod().hasName("addCookie") | + ma.getArgument(0) = this.asExpr() + ) + } +} + +private class SensitiveActionSink extends WeakRandomnessSink { + SensitiveActionSink() { this.asExpr() instanceof SensitiveExpr } +} + +/** + * Holds if there is a method access which converts `bytes` to the string `str`. + */ +private predicate covertsBytesToString(DataFlow::Node bytes, DataFlow::Node str) { + bytes.getType().(Array).getElementType().(PrimitiveType).hasName("byte") and + str.getType() instanceof TypeString and + exists(MethodAccess ma | ma = str.asExpr() | bytes.asExpr() = ma.getAnArgument()) +} + +/** + * A taint-tracking configuration for weak randomness. + */ +module WeakRandomnessConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src instanceof WeakRandomnessSource } + + predicate isSink(DataFlow::Node sink) { sink instanceof WeakRandomnessSink } + + predicate isBarrier(DataFlow::Node n) { n.getTypeBound() instanceof SafeRandomImplementation } + + 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(MethodAccess ma, Method m | + n1.asExpr() = ma.getQualifier() and + ma.getMethod() = m and + m.getDeclaringType() instanceof TypeRandom and + ( + m.hasName(["nextInt", "nextLong", "nextFloat", "nextDouble", "nextBoolean", "nextGaussian"]) and + n2.asExpr() = ma + or + m.hasName("nextBytes") and + n2.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = ma.getArgument(0) + ) + ) + or + covertsBytesToString(n1, n2) + } +} + +/** + * Taint-tracking flow of a weakly random value into a sensitive sink. + */ +module WeakRandomnessFlow = TaintTracking::Global; diff --git a/java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql b/java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql index f590c6abd0fd..6b3dfa99712b 100644 --- a/java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql +++ b/java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql @@ -12,101 +12,9 @@ */ import java -import semmle.code.java.frameworks.Servlets -import semmle.code.java.dataflow.TaintTracking -import semmle.code.java.security.RandomQuery +import semmle.code.java.security.WeakRandomnessQuery import WeakRandomnessFlow::PathGraph -/** - * The `java.util.Random` class. - */ -class TypeRandom extends RefType { - TypeRandom() { this.hasQualifiedName("java.util", "Random") } -} - -abstract class WeakRandomnessSource extends DataFlow::Node { } - -private class JavaRandomSource extends WeakRandomnessSource { - JavaRandomSource() { - this.asExpr().getType() instanceof TypeRandom and this.asExpr() instanceof ConstructorCall - } -} - -private class MathRandomMethodAccess extends WeakRandomnessSource { - MathRandomMethodAccess() { - exists(MethodAccess ma | this.asExpr() = ma | - ma.getMethod().hasName("random") and - ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Math") - ) - } -} - -abstract private class SafeRandomImplementation extends RefType { } - -private class TypeSecureRandom extends SafeRandomImplementation { - TypeSecureRandom() { this.hasQualifiedName("java.security", "SecureRandom") } -} - -private class TypeHadoopOsSecureRandom extends SafeRandomImplementation { - TypeHadoopOsSecureRandom() { - this.hasQualifiedName("org.apache.hadoop.crypto.random", "OsSecureRandom") - } -} - -abstract class WeakRandomnessAdditionalTaintStep extends Unit { - abstract predicate step(DataFlow::Node node1, DataFlow::Node node2); -} - -abstract class WeakRandomnessSink extends DataFlow::Node { } - -private class CookieSink extends WeakRandomnessSink { - CookieSink() { - this.asExpr().getType() instanceof TypeCookie and - exists(MethodAccess ma | ma.getMethod().hasName("addCookie") | - ma.getArgument(0) = this.asExpr() - ) - } -} - -/** - * Holds if there is a method access which converts `bytes` to the string `str`. - */ -private predicate covertsBytesToString(DataFlow::Node bytes, DataFlow::Node str) { - bytes.getType().(Array).getElementType().(PrimitiveType).hasName("byte") and - str.getType() instanceof TypeString and - exists(MethodAccess ma | ma = str.asExpr() | bytes.asExpr() = ma.getAnArgument()) -} - -/** - * A taint-tracking configuration for weak randomness. - */ -module WeakRandomnessConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node src) { src instanceof WeakRandomnessSource } - - predicate isSink(DataFlow::Node sink) { sink instanceof WeakRandomnessSink } - - predicate isBarrier(DataFlow::Node n) { n.getTypeBound() instanceof SafeRandomImplementation } - - predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess ma, Method m | - n1.asExpr() = ma.getQualifier() and - ma.getMethod() = m and - m.getDeclaringType() instanceof TypeRandom and - ( - m.hasName(["nextInt", "nextLong", "nextFloat", "nextDouble", "nextBoolean", "nextGaussian"]) and - n2.asExpr() = ma - or - m.hasName("nextBytes") and - n2.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = ma.getArgument(0) - ) - ) - or - covertsBytesToString(n1, n2) - } -} - -module WeakRandomnessFlow = TaintTracking::Global; - from WeakRandomnessFlow::PathNode source, WeakRandomnessFlow::PathNode sink where WeakRandomnessFlow::flowPath(source, sink) select sink.getNode(), source, sink, "Potential weak randomness due to a $@.", source.getNode(), From 1daa83bf4651538ea6237e771900efe4fd25b6f8 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 28 Jun 2023 12:56:37 -0400 Subject: [PATCH 03/29] Add test cases --- .../security/CWE-330/WeakRandomCookies.java | 40 +++++++++++++++++++ .../security/CWE-330/WeakRandomTest.expected | 2 + .../security/CWE-330/WeakRandomTest.ql | 18 +++++++++ .../test/query-tests/security/CWE-330/options | 1 + 4 files changed, 61 insertions(+) create mode 100644 java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java create mode 100644 java/ql/test/query-tests/security/CWE-330/WeakRandomTest.expected create mode 100644 java/ql/test/query-tests/security/CWE-330/WeakRandomTest.ql create mode 100644 java/ql/test/query-tests/security/CWE-330/options 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..e1862ca1431c --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java @@ -0,0 +1,40 @@ +import java.io.IOException; +import java.util.Random; +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; + +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)); + response.addCookie(cookie); // $hasWeakRandomFlow + + int c2 = r.nextInt(); + // BAD: The cookie value may be predictable. + Cookie cookie2 = new Cookie("name" + c2, "value"); + response.addCookie(cookie2); // $hasWeakRandomFlow + + byte[] bytes = new byte[16]; + r.nextBytes(bytes); + // BAD: The cookie value may be predictable. + Cookie cookie3 = new Cookie("name", new String(bytes)); + response.addCookie(cookie3); // $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)); + response.addCookie(cookie4); + } +} diff --git a/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.expected b/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.expected new file mode 100644 index 000000000000..48de9172b362 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.expected @@ -0,0 +1,2 @@ +failures +testFailures diff --git a/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.ql b/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.ql new file mode 100644 index 000000000000..49d1ff607c3a --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.ql @@ -0,0 +1,18 @@ +import java +import semmle.code.java.security.WeakRandomnessQuery +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 | WeakRandomnessFlow::flowTo(sink) | + sink.getLocation() = location and + element = sink.toString() and + value = "" + ) + } +} + +import MakeTest 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..4b3b1c7644dd --- /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 \ No newline at end of file From bf0123d6aef98208c02e1990784bb56d5008fc1b Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Tue, 11 Jul 2023 17:18:31 -0400 Subject: [PATCH 04/29] Add org.apache.commons.lang.RandomStringUtils as a source --- .../code/java/security/WeakRandomnessQuery.qll | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index abb226453c0f..894b9480c52e 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -29,6 +29,21 @@ private class JavaRandomSource extends WeakRandomnessSource { } } +private class ApacheRandomStringUtilsMethodAccessSource extends WeakRandomnessSource { + ApacheRandomStringUtilsMethodAccessSource() { + exists(MethodAccess ma | this.asExpr() = ma | + ma.getMethod() + .hasName([ + "random", "randomAlphabetic", "randomAlphanumeric", "randomAscii", "randomGraph", + "randomNumeric", "randomPrint" + ]) and + ma.getMethod() + .getDeclaringType() + .hasQualifiedName("org.apache.commons.lang", "RandomStringUtils") + ) + } +} + /** * The `random` method of `java.lang.Math`. */ From b713efb7115ee6500e02fec305cf87ece7ef6804 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Tue, 11 Jul 2023 17:42:34 -0400 Subject: [PATCH 05/29] Add ThreadLocalRandom.current as another source --- .../code/java/security/WeakRandomnessQuery.qll | 16 +++++++++++++++- .../security/CWE-330/WeakRandomCookies.java | 6 ++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 894b9480c52e..20e8e66a52ce 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -29,6 +29,9 @@ private class JavaRandomSource extends WeakRandomnessSource { } } +/** + * A node representing a call to one of the methods of `org.apache.commons.lang.RandomStringUtils`. + */ private class ApacheRandomStringUtilsMethodAccessSource extends WeakRandomnessSource { ApacheRandomStringUtilsMethodAccessSource() { exists(MethodAccess ma | this.asExpr() = ma | @@ -44,6 +47,17 @@ private class ApacheRandomStringUtilsMethodAccessSource extends WeakRandomnessSo } } +private class ThreadLocalRandomSource extends WeakRandomnessSource { + ThreadLocalRandomSource() { + exists(MethodAccess ma | this.asExpr() = ma | + ma.getMethod().hasName("current") and + ma.getMethod() + .getDeclaringType() + .hasQualifiedName("java.util.concurrent", "ThreadLocalRandom") + ) + } +} + /** * The `random` method of `java.lang.Math`. */ @@ -123,7 +137,7 @@ module WeakRandomnessConfig implements DataFlow::ConfigSig { exists(MethodAccess ma, Method m | n1.asExpr() = ma.getQualifier() and ma.getMethod() = m and - m.getDeclaringType() instanceof TypeRandom and + m.getDeclaringType().getAnAncestor() instanceof TypeRandom and ( m.hasName(["nextInt", "nextLong", "nextFloat", "nextDouble", "nextBoolean", "nextGaussian"]) and n2.asExpr() = ma diff --git a/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java b/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java index e1862ca1431c..92b240c11c9e 100644 --- a/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java +++ b/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java @@ -1,5 +1,6 @@ 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; @@ -36,5 +37,10 @@ public void doGet() { // GOOD: The cookie value is unpredictable. Cookie cookie4 = new Cookie("name", new String(bytes2)); response.addCookie(cookie4); + + ThreadLocalRandom tlr = ThreadLocalRandom.current(); + + Cookie cookie5 = new Cookie("name", Integer.toString(tlr.nextInt())); + response.addCookie(cookie5); // $hasWeakRandomFlow } } From 0313f3922941718c21c4d33b8d15a2e814e2f4bc Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Fri, 28 Jul 2023 15:39:00 -0400 Subject: [PATCH 06/29] Cryptographic sinks --- java/ql/lib/ext/java.security.spec.model.yml | 11 +++++++++++ java/ql/lib/ext/javax.crypto.spec.model.yml | 16 ++++++++++++++++ .../code/java/security/WeakRandomnessQuery.qll | 5 +++++ 3 files changed, 32 insertions(+) diff --git a/java/ql/lib/ext/java.security.spec.model.yml b/java/ql/lib/ext/java.security.spec.model.yml index 2318fa11f917..fafd5bdabf42 100644 --- a/java/ql/lib/ext/java.security.spec.model.yml +++ b/java/ql/lib/ext/java.security.spec.model.yml @@ -6,3 +6,14 @@ extensions: - ["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", "X509EncodedKeySpec", False, "X509EncodedKeySpec", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"] + - ["java.security.spec", "DSAParameterSpec", False, "DSAParameterSpec", "", "", "Argument[0..2]", "crypto-parameter", "manual"] + - ["java.security.spec", "DSAPrivateKeySpec", False, "DSAPrivateKeySpec", "", "", "Argument[0..3]", "crypto-parameter", "manual"] + - ["java.security.spec", "DSAPublicKeySpec", False, "DSAPublicKeySpec", "", "", "Argument[0..3]", "crypto-parameter", "manual"] + - ["java.security.spec", "ECPrivateKeySpec", False, "ECPrivateKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] + - ["java.security.spec", "EncodedKeySpec", False, "EncodedKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] + - ["java.security.spec", "PKCS8EncodedKeySpec", False, "PKCS8EncodedKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] + - ["java.security.spec", "RSAMultiPrimePrivateCrtKeySpec", False, "RSAMultiPrimePrivateCrtKeySpec", "", "", "Argument[0..8]", "crypto-parameter", "manual"] + - ["java.security.spec", "RSAPrivateCrtKeySpec", False, "RSAPrivateCrtKeySpec", "", "", "Argument[0..7]", "crypto-parameter", "manual"] + - ["java.security.spec", "RSAPrivateKeySpec", False, "RSAPrivateKeySpec", "", "", "Argument[0..1]", "crypto-parameter", "manual"] + - ["java.security.spec", "RSAPublicKeySpec", False, "RSAPublicKeySpec", "", "", "Argument[0..1]", "crypto-parameter", "manual"] + - ["java.security.spec", "X509EncodedKeySpec", False, "X509EncodedKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] diff --git a/java/ql/lib/ext/javax.crypto.spec.model.yml b/java/ql/lib/ext/javax.crypto.spec.model.yml index 9bc4f3cc1746..738b4d2260b3 100644 --- a/java/ql/lib/ext/javax.crypto.spec.model.yml +++ b/java/ql/lib/ext/javax.crypto.spec.model.yml @@ -23,3 +23,19 @@ extensions: - ["javax.crypto.spec", "DESedeKeySpec", False, "isParityAdjusted", "(byte[],int)", "", "Argument[0]", "credentials-key", "hq-generated"] - ["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"] + - ["javax.crypto.spec", "IvParameterSpec", False, "IvParameterSpec", "(byte[])", "", "Argument[0]", "encryption-iv", "manual"] + - ["javax.crypto.spec", "IvParameterSpec", False, "IvParameterSpec", "(byte[],int,int)", "", "Argument[0]", "encryption-iv", "manual"] + - ["javax.crypto.spec", "DESedeKeySpec", False, "DESedeKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "DESKeySpec", False, "DESKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "DHPrivateKeySpec", False, "DHPrivateKeySpec", "", "", "Argument[1..3]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "DHPublicKeySpec", False, "DHPublicKeySpec", "", "", "Argument[1..3]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "DHPublicKeySpec", False, "DHPublicKeySpec", "", "", "Argument[1..3]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "DSAParameterSpec", False, "DSAParameterSpec", "", "", "Argument[1..3]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "GCMParameterSpec", False, "GCMParameterSpec", "", "", "Argument[1]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "IvParameterSpec", False, "IvParameterSpec", "", "", "Argument[0]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "", "", "Argument[0..1]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "", "", "Argument[0..1]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "PBEParameterSpec", False, "PBEParameterSpec", "", "", "Argument[0]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "RC2ParameterSpec", False, "RC2ParameterSpec", "", "", "Argument[1]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "RC5ParameterSpec", False, "RC25arameterSpec", "", "", "Argument[3]", "crypto-parameter", "manual"] + - ["javax.crypto.spec", "SecretKeySpec", False, "SecretKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] \ No newline at end of file diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 20e8e66a52ce..2b798edbc4bd 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -4,6 +4,7 @@ import java import semmle.code.java.frameworks.Servlets import semmle.code.java.security.SensitiveActions import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.ExternalFlow import semmle.code.java.security.RandomQuery /** @@ -108,6 +109,10 @@ private class SensitiveActionSink extends WeakRandomnessSink { SensitiveActionSink() { this.asExpr() instanceof SensitiveExpr } } +private class CryptographicSink extends WeakRandomnessSink { + CryptographicSink() { sinkNode(this, "crypto-parameter") } +} + /** * Holds if there is a method access which converts `bytes` to the string `str`. */ From 14fdfa442882935d9ab9878d0b249ce6c003c036 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Tue, 1 Aug 2023 14:43:37 -0400 Subject: [PATCH 07/29] Add new sink kind and change note --- .../src/change-notes/2023-08-01-weak-randomness-query.md | 5 +++++ shared/mad/codeql/mad/ModelValidation.qll | 9 +++++---- 2 files changed, 10 insertions(+), 4 deletions(-) create mode 100644 java/ql/src/change-notes/2023-08-01-weak-randomness-query.md diff --git a/java/ql/src/change-notes/2023-08-01-weak-randomness-query.md b/java/ql/src/change-notes/2023-08-01-weak-randomness-query.md new file mode 100644 index 000000000000..6581f8751a50 --- /dev/null +++ b/java/ql/src/change-notes/2023-08-01-weak-randomness-query.md @@ -0,0 +1,5 @@ +--- +category: newQuery +--- +* Added the `java/weak-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/shared/mad/codeql/mad/ModelValidation.qll b/shared/mad/codeql/mad/ModelValidation.qll index 59fc2a19df38..cdd233889f11 100644 --- a/shared/mad/codeql/mad/ModelValidation.qll +++ b/shared/mad/codeql/mad/ModelValidation.qll @@ -30,10 +30,11 @@ module KindValidation { "js-injection", "ldap-injection", "log-injection", "path-injection", "request-forgery", "sql-injection", "url-redirection", // Java-only currently, but may be shared in the future - "bean-validation", "fragment-injection", "groovy-injection", "hostname-verification", - "information-leak", "intent-redirection", "jexl-injection", "jndi-injection", - "mvel-injection", "ognl-injection", "pending-intents", "response-splitting", - "trust-boundary-violation", "template-injection", "xpath-injection", "xslt-injection", + "bean-validation", "crypto-parameter", "fragment-injection", "groovy-injection", + "hostname-verification", "information-leak", "intent-redirection", "jexl-injection", + "jndi-injection", "mvel-injection", "ognl-injection", "pending-intents", + "response-splitting", "trust-boundary-violation", "template-injection", "xpath-injection", + "xslt-injection", // JavaScript-only currently, but may be shared in the future "mongodb.sink", "nosql-injection", "unsafe-deserialization", // Swift-only currently, but may be shared in the future From bc0655573ff6f53bafdd6c516177fd1b4219f133 Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Wed, 2 Aug 2023 09:25:58 -0400 Subject: [PATCH 08/29] Simplifications Co-authored-by: Tony Torralba --- .../java/security/WeakRandomnessQuery.qll | 54 ++++++++----------- 1 file changed, 22 insertions(+), 32 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 2b798edbc4bd..0f26b1257b94 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -25,9 +25,7 @@ abstract class WeakRandomnessSource extends DataFlow::Node { } * A node representing a call to a constructor of `java.util.Random`. */ private class JavaRandomSource extends WeakRandomnessSource { - JavaRandomSource() { - this.asExpr().getType() instanceof TypeRandom and this.asExpr() instanceof ConstructorCall - } + JavaRandomSource() { this.asExpr().(ClassInstanceExpr).getType() instanceof TypeRandom } } /** @@ -35,27 +33,23 @@ private class JavaRandomSource extends WeakRandomnessSource { */ private class ApacheRandomStringUtilsMethodAccessSource extends WeakRandomnessSource { ApacheRandomStringUtilsMethodAccessSource() { - exists(MethodAccess ma | this.asExpr() = ma | - ma.getMethod() - .hasName([ - "random", "randomAlphabetic", "randomAlphanumeric", "randomAscii", "randomGraph", - "randomNumeric", "randomPrint" - ]) and - ma.getMethod() - .getDeclaringType() - .hasQualifiedName("org.apache.commons.lang", "RandomStringUtils") - ) + this.asExpr() + .(MethodAccess) + .getMethod() + .hasQualifiedName("org.apache.commons.lang", "RandomStringUtils", + [ + "random", "randomAlphabetic", "randomAlphanumeric", "randomAscii", "randomGraph", + "randomNumeric", "randomPrint" + ]) } } private class ThreadLocalRandomSource extends WeakRandomnessSource { ThreadLocalRandomSource() { - exists(MethodAccess ma | this.asExpr() = ma | - ma.getMethod().hasName("current") and - ma.getMethod() - .getDeclaringType() - .hasQualifiedName("java.util.concurrent", "ThreadLocalRandom") - ) + this.asExpr() + .(MethodAccess) + .getMethod() + .hasQualifiedName("java.util.concurrent", "ThreadLocalRandom", "current") } } @@ -64,10 +58,7 @@ private class ThreadLocalRandomSource extends WeakRandomnessSource { */ private class MathRandomMethodAccess extends WeakRandomnessSource { MathRandomMethodAccess() { - exists(MethodAccess ma | this.asExpr() = ma | - ma.getMethod().hasName("random") and - ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "Math") - ) + this.asExpr().(MethodAccess).getMethod().hasQualifiedName("java.lang", "Math", "random") } } @@ -98,7 +89,7 @@ abstract class WeakRandomnessSink extends DataFlow::Node { } */ private class CookieSink extends WeakRandomnessSink { CookieSink() { - this.asExpr().getType() instanceof TypeCookie and + this.getType() instanceof TypeCookie and exists(MethodAccess ma | ma.getMethod().hasName("addCookie") | ma.getArgument(0) = this.asExpr() ) @@ -142,14 +133,13 @@ module WeakRandomnessConfig implements DataFlow::ConfigSig { exists(MethodAccess ma, Method m | n1.asExpr() = ma.getQualifier() and ma.getMethod() = m and - m.getDeclaringType().getAnAncestor() instanceof TypeRandom and - ( - m.hasName(["nextInt", "nextLong", "nextFloat", "nextDouble", "nextBoolean", "nextGaussian"]) and - n2.asExpr() = ma - or - m.hasName("nextBytes") and - n2.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = ma.getArgument(0) - ) + m.getDeclaringType().getAnAncestor() instanceof TypeRandom + | + m.hasName(["nextInt", "nextLong", "nextFloat", "nextDouble", "nextBoolean", "nextGaussian"]) and + n2.asExpr() = ma + or + m.hasName("nextBytes") and + n2.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = ma.getArgument(0) ) or covertsBytesToString(n1, n2) From ce7690b53f3700e3f09278c4a6665a071355fac9 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 2 Aug 2023 09:29:48 -0400 Subject: [PATCH 09/29] Make imports private --- .../semmle/code/java/security/WeakRandomnessQuery.qll | 10 +++++----- .../query-tests/security/CWE-330/WeakRandomTest.ql | 1 + 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 0f26b1257b94..3c00b4f85512 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -1,11 +1,11 @@ /** Provides classes and predicates for reasoning about weak randomness. */ import java -import semmle.code.java.frameworks.Servlets -import semmle.code.java.security.SensitiveActions -import semmle.code.java.dataflow.TaintTracking -import semmle.code.java.dataflow.ExternalFlow -import semmle.code.java.security.RandomQuery +private import semmle.code.java.frameworks.Servlets +private import semmle.code.java.security.SensitiveActions +private import semmle.code.java.dataflow.TaintTracking +private import semmle.code.java.dataflow.ExternalFlow +private import semmle.code.java.security.RandomQuery /** * The `java.util.Random` class. diff --git a/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.ql b/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.ql index 49d1ff607c3a..c2025172a0f5 100644 --- a/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.ql +++ b/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.ql @@ -1,4 +1,5 @@ import java +import semmle.code.java.dataflow.DataFlow import semmle.code.java.security.WeakRandomnessQuery import TestUtilities.InlineExpectationsTest From dc3e4cd92875a90d6a7371a11cb99bfc39611cdb Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Mon, 7 Aug 2023 00:01:28 -0400 Subject: [PATCH 10/29] Refactored method accesses to the RandomDataSource library --- .../code/java/security/RandomDataSource.qll | 23 ++++++++ .../java/security/WeakRandomnessQuery.qll | 57 ++----------------- 2 files changed, 28 insertions(+), 52 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/RandomDataSource.qll b/java/ql/lib/semmle/code/java/security/RandomDataSource.qll index 7d67032e3925..b483218ebcbc 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,17 @@ 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", "RandomStringUtils") + ) + } + + override Expr getOutput() { result = this } +} diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 3c00b4f85512..752709a0b1cc 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -21,44 +21,11 @@ class TypeRandom extends RefType { */ abstract class WeakRandomnessSource extends DataFlow::Node { } -/** - * A node representing a call to a constructor of `java.util.Random`. - */ -private class JavaRandomSource extends WeakRandomnessSource { - JavaRandomSource() { this.asExpr().(ClassInstanceExpr).getType() instanceof TypeRandom } -} - -/** - * A node representing a call to one of the methods of `org.apache.commons.lang.RandomStringUtils`. - */ -private class ApacheRandomStringUtilsMethodAccessSource extends WeakRandomnessSource { - ApacheRandomStringUtilsMethodAccessSource() { - this.asExpr() - .(MethodAccess) - .getMethod() - .hasQualifiedName("org.apache.commons.lang", "RandomStringUtils", - [ - "random", "randomAlphabetic", "randomAlphanumeric", "randomAscii", "randomGraph", - "randomNumeric", "randomPrint" - ]) - } -} - -private class ThreadLocalRandomSource extends WeakRandomnessSource { - ThreadLocalRandomSource() { - this.asExpr() - .(MethodAccess) - .getMethod() - .hasQualifiedName("java.util.concurrent", "ThreadLocalRandom", "current") - } -} - -/** - * The `random` method of `java.lang.Math`. - */ -private class MathRandomMethodAccess extends WeakRandomnessSource { - MathRandomMethodAccess() { - this.asExpr().(MethodAccess).getMethod().hasQualifiedName("java.lang", "Math", "random") +private class RandomMethodSource extends WeakRandomnessSource { + RandomMethodSource() { + exists(RandomDataSource s | this.asExpr() = s.getOutput() | + not s.getQualifier().getType() instanceof SafeRandomImplementation + ) } } @@ -121,8 +88,6 @@ module WeakRandomnessConfig implements DataFlow::ConfigSig { predicate isSink(DataFlow::Node sink) { sink instanceof WeakRandomnessSink } - predicate isBarrier(DataFlow::Node n) { n.getTypeBound() instanceof SafeRandomImplementation } - predicate isBarrierIn(DataFlow::Node n) { isSource(n) } predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { @@ -130,18 +95,6 @@ module WeakRandomnessConfig implements DataFlow::ConfigSig { or n1.asExpr() = n2.asExpr().(UnaryExpr).getExpr() or - exists(MethodAccess ma, Method m | - n1.asExpr() = ma.getQualifier() and - ma.getMethod() = m and - m.getDeclaringType().getAnAncestor() instanceof TypeRandom - | - m.hasName(["nextInt", "nextLong", "nextFloat", "nextDouble", "nextBoolean", "nextGaussian"]) and - n2.asExpr() = ma - or - m.hasName("nextBytes") and - n2.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = ma.getArgument(0) - ) - or covertsBytesToString(n1, n2) } } From ba3c38c2265bb99e59f8847542463fa2bd5035f5 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Mon, 7 Aug 2023 00:04:02 -0400 Subject: [PATCH 11/29] Restrict `addCookie` to specific interface --- java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 752709a0b1cc..66ed90aa5f97 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -57,7 +57,9 @@ abstract class WeakRandomnessSink extends DataFlow::Node { } private class CookieSink extends WeakRandomnessSink { CookieSink() { this.getType() instanceof TypeCookie and - exists(MethodAccess ma | ma.getMethod().hasName("addCookie") | + exists(MethodAccess ma | + ma.getMethod().hasQualifiedName("javax.servlet.http", "HttpServletResponse", "addCookie") + | ma.getArgument(0) = this.asExpr() ) } From fb875f5095a1406b904b62965245ecf1e06fad2c Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Mon, 7 Aug 2023 00:04:31 -0400 Subject: [PATCH 12/29] More variety of test cases --- .../security/CWE-330/WeakRandomCookies.java | 22 +++++++++++++++++++ .../test/query-tests/security/CWE-330/options | 2 +- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java b/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java index 92b240c11c9e..c9f28df8dfb6 100644 --- a/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java +++ b/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java @@ -7,6 +7,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.Cookie; +import org.apache.commons.lang3.RandomStringUtils; public class WeakRandomCookies extends HttpServlet { HttpServletResponse response; @@ -42,5 +43,26 @@ public void doGet() { Cookie cookie5 = new Cookie("name", Integer.toString(tlr.nextInt())); response.addCookie(cookie5); // $hasWeakRandomFlow + + Cookie cookie6 = new Cookie("name", RandomStringUtils.random(10)); + response.addCookie(cookie6); // $hasWeakRandomFlow + + Cookie cookie7 = new Cookie("name", RandomStringUtils.randomAscii(10)); + response.addCookie(cookie7); // $hasWeakRandomFlow + + long c3 = r.nextLong(); + // BAD: The cookie value may be predictable. + Cookie cookie8 = new Cookie("name", Long.toString(c3 * 5)); + response.addCookie(cookie8); // $hasWeakRandomFlow + + double c4 = Math.random(); + // BAD: The cookie value may be predictable. + Cookie cookie9 = new Cookie("name", Double.toString(c4)); + response.addCookie(cookie9); // $hasWeakRandomFlow + + double c5 = Math.random(); + // BAD: The cookie value may be predictable. + Cookie cookie10 = new Cookie("name", Double.toString(++c5)); + response.addCookie(cookie10); // $hasWeakRandomFlow } } diff --git a/java/ql/test/query-tests/security/CWE-330/options b/java/ql/test/query-tests/security/CWE-330/options index 4b3b1c7644dd..e2910580186f 100644 --- a/java/ql/test/query-tests/security/CWE-330/options +++ b/java/ql/test/query-tests/security/CWE-330/options @@ -1 +1 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/servlet-api-2.4 \ No newline at end of file +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/servlet-api-2.4:${testdir}/../../../stubs/apache-commons-lang3-3.7 \ No newline at end of file From 057a74d914dec5df9431df8c032a45f7ddf0e98e Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Mon, 7 Aug 2023 00:05:31 -0400 Subject: [PATCH 13/29] Remove unnused class --- .../lib/semmle/code/java/security/WeakRandomnessQuery.qll | 7 ------- 1 file changed, 7 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 66ed90aa5f97..182e6f832b62 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -7,13 +7,6 @@ private import semmle.code.java.dataflow.TaintTracking private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.security.RandomQuery -/** - * The `java.util.Random` class. - */ -class TypeRandom extends RefType { - TypeRandom() { this.hasQualifiedName("java.util", "Random") } -} - /** * A node representing a source of weak randomness. * From 646254c9b2fe3574342a1d076f31db2d64e40756 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Tue, 12 Sep 2023 23:13:33 -0400 Subject: [PATCH 14/29] Add credentials sinks from SensitiveApi --- java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll | 3 +++ 1 file changed, 3 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 182e6f832b62..79ebfb6802e4 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -3,6 +3,7 @@ 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 @@ -66,6 +67,8 @@ private class CryptographicSink extends WeakRandomnessSink { CryptographicSink() { sinkNode(this, "crypto-parameter") } } +private class CredentialsSink extends WeakRandomnessSink instanceof CredentialsSinkNode { } + /** * Holds if there is a method access which converts `bytes` to the string `str`. */ From b8b2de2f3c182c8342d88437b5c21b0f33c7f28a Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Fri, 29 Sep 2023 11:11:01 -0400 Subject: [PATCH 15/29] Remove use of `crypto-parameter` sink kind --- java/ql/lib/ext/java.security.spec.model.yml | 19 +++++------ java/ql/lib/ext/javax.crypto.spec.model.yml | 32 ++++++++----------- .../java/security/WeakRandomnessQuery.qll | 4 --- shared/mad/codeql/mad/ModelValidation.qll | 9 +++--- 4 files changed, 25 insertions(+), 39 deletions(-) diff --git a/java/ql/lib/ext/java.security.spec.model.yml b/java/ql/lib/ext/java.security.spec.model.yml index fafd5bdabf42..f2834224b30a 100644 --- a/java/ql/lib/ext/java.security.spec.model.yml +++ b/java/ql/lib/ext/java.security.spec.model.yml @@ -3,17 +3,14 @@ extensions: pack: codeql/java-all extensible: sinkModel data: + - ["java.security.spec", "DSAParameterSpec", False, "DSAParameterSpec", "", "", "Argument[0..2]", "credentials-key", "manual"] + - ["java.security.spec", "DSAPrivateKeySpec", False, "DSAPrivateKeySpec", "", "", "Argument[0..3]", "credentials-key", "manual"] + - ["java.security.spec", "DSAPublicKeySpec", False, "DSAPublicKeySpec", "", "", "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[0..8]", "credentials-key", "manual"] + - ["java.security.spec", "RSAPrivateCrtKeySpec", False, "RSAPrivateCrtKeySpec", "", "", "Argument[0..7]", "credentials-key", "manual"] + - ["java.security.spec", "RSAPrivateKeySpec", False, "RSAPrivateKeySpec", "", "", "Argument[0..1]", "credentials-key", "manual"] + - ["java.security.spec", "RSAPublicKeySpec", False, "RSAPublicKeySpec", "", "", "Argument[0..1]", "credentials-key", "manual"] - ["java.security.spec", "X509EncodedKeySpec", False, "X509EncodedKeySpec", "(byte[])", "", "Argument[0]", "credentials-key", "hq-generated"] - - ["java.security.spec", "DSAParameterSpec", False, "DSAParameterSpec", "", "", "Argument[0..2]", "crypto-parameter", "manual"] - - ["java.security.spec", "DSAPrivateKeySpec", False, "DSAPrivateKeySpec", "", "", "Argument[0..3]", "crypto-parameter", "manual"] - - ["java.security.spec", "DSAPublicKeySpec", False, "DSAPublicKeySpec", "", "", "Argument[0..3]", "crypto-parameter", "manual"] - - ["java.security.spec", "ECPrivateKeySpec", False, "ECPrivateKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] - - ["java.security.spec", "EncodedKeySpec", False, "EncodedKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] - - ["java.security.spec", "PKCS8EncodedKeySpec", False, "PKCS8EncodedKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] - - ["java.security.spec", "RSAMultiPrimePrivateCrtKeySpec", False, "RSAMultiPrimePrivateCrtKeySpec", "", "", "Argument[0..8]", "crypto-parameter", "manual"] - - ["java.security.spec", "RSAPrivateCrtKeySpec", False, "RSAPrivateCrtKeySpec", "", "", "Argument[0..7]", "crypto-parameter", "manual"] - - ["java.security.spec", "RSAPrivateKeySpec", False, "RSAPrivateKeySpec", "", "", "Argument[0..1]", "crypto-parameter", "manual"] - - ["java.security.spec", "RSAPublicKeySpec", False, "RSAPublicKeySpec", "", "", "Argument[0..1]", "crypto-parameter", "manual"] - - ["java.security.spec", "X509EncodedKeySpec", False, "X509EncodedKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] diff --git a/java/ql/lib/ext/javax.crypto.spec.model.yml b/java/ql/lib/ext/javax.crypto.spec.model.yml index 738b4d2260b3..0a81be628e20 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,21 +18,18 @@ 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", "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"] + - ["javax.crypto.spec", "DHPrivateKeySpec", False, "DHPrivateKeySpec", "", "", "Argument[1..3]", "credentials-key", "manual"] + - ["javax.crypto.spec", "DHPublicKeySpec", False, "DHPublicKeySpec", "", "", "Argument[1..3]", "credentials-key", "manual"] + - ["javax.crypto.spec", "DSAParameterSpec", False, "DSAParameterSpec", "", "", "Argument[1..3]", "credentials-key", "manual"] + - ["javax.crypto.spec", "GCMParameterSpec", False, "GCMParameterSpec", "", "", "Argument[1]", "encryption-iv", "manual"] - ["javax.crypto.spec", "IvParameterSpec", False, "IvParameterSpec", "(byte[])", "", "Argument[0]", "encryption-iv", "manual"] - ["javax.crypto.spec", "IvParameterSpec", False, "IvParameterSpec", "(byte[],int,int)", "", "Argument[0]", "encryption-iv", "manual"] - - ["javax.crypto.spec", "DESedeKeySpec", False, "DESedeKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "DESKeySpec", False, "DESKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "DHPrivateKeySpec", False, "DHPrivateKeySpec", "", "", "Argument[1..3]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "DHPublicKeySpec", False, "DHPublicKeySpec", "", "", "Argument[1..3]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "DHPublicKeySpec", False, "DHPublicKeySpec", "", "", "Argument[1..3]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "DSAParameterSpec", False, "DSAParameterSpec", "", "", "Argument[1..3]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "GCMParameterSpec", False, "GCMParameterSpec", "", "", "Argument[1]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "IvParameterSpec", False, "IvParameterSpec", "", "", "Argument[0]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "", "", "Argument[0..1]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "PBEKeySpec", False, "PBEKeySpec", "", "", "Argument[0..1]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "PBEParameterSpec", False, "PBEParameterSpec", "", "", "Argument[0]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "RC2ParameterSpec", False, "RC2ParameterSpec", "", "", "Argument[1]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "RC5ParameterSpec", False, "RC25arameterSpec", "", "", "Argument[3]", "crypto-parameter", "manual"] - - ["javax.crypto.spec", "SecretKeySpec", False, "SecretKeySpec", "", "", "Argument[0]", "crypto-parameter", "manual"] \ No newline at end of file + - ["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", "RC2ParameterSpec", False, "RC2ParameterSpec", "", "", "Argument[1]", "encryption-iv", "manual"] + - ["javax.crypto.spec", "RC5ParameterSpec", False, "RC5ParameterSpec", "", "", "Argument[3]", "encryption-iv", "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/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 79ebfb6802e4..7d7ea7efacd4 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -63,10 +63,6 @@ private class SensitiveActionSink extends WeakRandomnessSink { SensitiveActionSink() { this.asExpr() instanceof SensitiveExpr } } -private class CryptographicSink extends WeakRandomnessSink { - CryptographicSink() { sinkNode(this, "crypto-parameter") } -} - private class CredentialsSink extends WeakRandomnessSink instanceof CredentialsSinkNode { } /** diff --git a/shared/mad/codeql/mad/ModelValidation.qll b/shared/mad/codeql/mad/ModelValidation.qll index cdd233889f11..59fc2a19df38 100644 --- a/shared/mad/codeql/mad/ModelValidation.qll +++ b/shared/mad/codeql/mad/ModelValidation.qll @@ -30,11 +30,10 @@ module KindValidation { "js-injection", "ldap-injection", "log-injection", "path-injection", "request-forgery", "sql-injection", "url-redirection", // Java-only currently, but may be shared in the future - "bean-validation", "crypto-parameter", "fragment-injection", "groovy-injection", - "hostname-verification", "information-leak", "intent-redirection", "jexl-injection", - "jndi-injection", "mvel-injection", "ognl-injection", "pending-intents", - "response-splitting", "trust-boundary-violation", "template-injection", "xpath-injection", - "xslt-injection", + "bean-validation", "fragment-injection", "groovy-injection", "hostname-verification", + "information-leak", "intent-redirection", "jexl-injection", "jndi-injection", + "mvel-injection", "ognl-injection", "pending-intents", "response-splitting", + "trust-boundary-violation", "template-injection", "xpath-injection", "xslt-injection", // JavaScript-only currently, but may be shared in the future "mongodb.sink", "nosql-injection", "unsafe-deserialization", // Swift-only currently, but may be shared in the future From a1e9564cc578d39de3cf94bf14cf91f988be9c61 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 11 Oct 2023 16:55:51 -0400 Subject: [PATCH 16/29] Add more sources --- java/ql/lib/semmle/code/java/security/RandomDataSource.qll | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/RandomDataSource.qll b/java/ql/lib/semmle/code/java/security/RandomDataSource.qll index b483218ebcbc..05ba28ba450c 100644 --- a/java/ql/lib/semmle/code/java/security/RandomDataSource.qll +++ b/java/ql/lib/semmle/code/java/security/RandomDataSource.qll @@ -160,7 +160,9 @@ class ApacheCommonsRandomStringSource extends RandomDataSource { ApacheCommonsRandomStringSource() { exists(Method m | m = this.getMethod() | m.getName().matches("random%") and - m.getDeclaringType().hasQualifiedName("org.apache.commons.lang3", "RandomStringUtils") + m.getDeclaringType() + .hasQualifiedName(["org.apache.commons.lang3", "org.apache.commons.lang"], + "RandomStringUtils") ) } From e9ca4a25d4ad3e5c4dac4a3173e388fc7c43c061 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 25 Oct 2023 23:13:15 -0400 Subject: [PATCH 17/29] Update to new `MethodCall` name --- .../lib/semmle/code/java/security/WeakRandomnessQuery.qll | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 7d7ea7efacd4..306ded934565 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -51,10 +51,10 @@ abstract class WeakRandomnessSink extends DataFlow::Node { } private class CookieSink extends WeakRandomnessSink { CookieSink() { this.getType() instanceof TypeCookie and - exists(MethodAccess ma | - ma.getMethod().hasQualifiedName("javax.servlet.http", "HttpServletResponse", "addCookie") + exists(MethodCall mc | + mc.getMethod().hasQualifiedName("javax.servlet.http", "HttpServletResponse", "addCookie") | - ma.getArgument(0) = this.asExpr() + mc.getArgument(0) = this.asExpr() ) } } @@ -71,7 +71,7 @@ private class CredentialsSink extends WeakRandomnessSink instanceof CredentialsS private predicate covertsBytesToString(DataFlow::Node bytes, DataFlow::Node str) { bytes.getType().(Array).getElementType().(PrimitiveType).hasName("byte") and str.getType() instanceof TypeString and - exists(MethodAccess ma | ma = str.asExpr() | bytes.asExpr() = ma.getAnArgument()) + exists(MethodCall mc | mc = str.asExpr() | bytes.asExpr() = mc.getAnArgument()) } /** From 7241e0920c0cdd713350b2134b921a9340130423 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 25 Oct 2023 23:14:45 -0400 Subject: [PATCH 18/29] Replace `convertBytesToString` with models --- java/ql/lib/ext/org.owasp.esapi.model.yml | 3 ++- .../semmle/code/java/security/WeakRandomnessQuery.qll | 11 ----------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/java/ql/lib/ext/org.owasp.esapi.model.yml b/java/ql/lib/ext/org.owasp.esapi.model.yml index 30578debe580..a8e59b385130 100644 --- a/java/ql/lib/ext/org.owasp.esapi.model.yml +++ b/java/ql/lib/ext/org.owasp.esapi.model.yml @@ -3,4 +3,5 @@ extensions: pack: codeql/java-all extensible: summaryModel data: - - ["org.owasp.esapi", "Encoder", true, "encodeForHTML", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"] \ No newline at end of file + - ["org.owasp.esapi", "Encoder", true, "encodeForHTML", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"] + - ["org.owasp.esapi", "Encoder", true, "encodeForBase64", "(byte[],boolean)", "", "Argument[0]", "ReturnValue", "taint", "manual"] diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 306ded934565..022215518f1e 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -65,15 +65,6 @@ private class SensitiveActionSink extends WeakRandomnessSink { private class CredentialsSink extends WeakRandomnessSink instanceof CredentialsSinkNode { } -/** - * Holds if there is a method access which converts `bytes` to the string `str`. - */ -private predicate covertsBytesToString(DataFlow::Node bytes, DataFlow::Node str) { - bytes.getType().(Array).getElementType().(PrimitiveType).hasName("byte") and - str.getType() instanceof TypeString and - exists(MethodCall mc | mc = str.asExpr() | bytes.asExpr() = mc.getAnArgument()) -} - /** * A taint-tracking configuration for weak randomness. */ @@ -88,8 +79,6 @@ module WeakRandomnessConfig implements DataFlow::ConfigSig { n1.asExpr() = n2.asExpr().(BinaryExpr).getAnOperand() or n1.asExpr() = n2.asExpr().(UnaryExpr).getExpr() - or - covertsBytesToString(n1, n2) } } From 7f3995f5244246390c5d10dc1a8861d0ed4f6836 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Thu, 26 Oct 2023 00:36:33 -0400 Subject: [PATCH 19/29] Remove extra `encryption-iv` models --- java/ql/lib/ext/javax.crypto.spec.model.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/java/ql/lib/ext/javax.crypto.spec.model.yml b/java/ql/lib/ext/javax.crypto.spec.model.yml index 0a81be628e20..a81811930147 100644 --- a/java/ql/lib/ext/javax.crypto.spec.model.yml +++ b/java/ql/lib/ext/javax.crypto.spec.model.yml @@ -21,15 +21,10 @@ extensions: - ["javax.crypto.spec", "DHPrivateKeySpec", False, "DHPrivateKeySpec", "", "", "Argument[1..3]", "credentials-key", "manual"] - ["javax.crypto.spec", "DHPublicKeySpec", False, "DHPublicKeySpec", "", "", "Argument[1..3]", "credentials-key", "manual"] - ["javax.crypto.spec", "DSAParameterSpec", False, "DSAParameterSpec", "", "", "Argument[1..3]", "credentials-key", "manual"] - - ["javax.crypto.spec", "GCMParameterSpec", False, "GCMParameterSpec", "", "", "Argument[1]", "encryption-iv", "manual"] - - ["javax.crypto.spec", "IvParameterSpec", False, "IvParameterSpec", "(byte[])", "", "Argument[0]", "encryption-iv", "manual"] - - ["javax.crypto.spec", "IvParameterSpec", False, "IvParameterSpec", "(byte[],int,int)", "", "Argument[0]", "encryption-iv", "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", "RC2ParameterSpec", False, "RC2ParameterSpec", "", "", "Argument[1]", "encryption-iv", "manual"] - - ["javax.crypto.spec", "RC5ParameterSpec", False, "RC5ParameterSpec", "", "", "Argument[3]", "encryption-iv", "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"] From b9d2a26e6e511c9f6b5cd2a0049edc9d40dcf44e Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Tue, 7 Nov 2023 16:49:13 -0500 Subject: [PATCH 20/29] Move ESAPI models into the Weak Randomness query These models don't need to apply to all queries. So instead they are better suited to be within the weak randomness query itself. --- java/ql/lib/ext/org.owasp.esapi.model.yml | 3 +-- .../lib/semmle/code/java/security/WeakRandomnessQuery.qll | 8 ++++++++ .../query-tests/security/CWE-330/WeakRandomCookies.java | 5 ++++- java/ql/test/query-tests/security/CWE-330/options | 2 +- .../test/stubs/esapi-2.0.1/org/owasp/esapi/Encoder.java | 2 ++ .../org/owasp/esapi/reference/DefaultEncoder.java | 1 + 6 files changed, 17 insertions(+), 4 deletions(-) diff --git a/java/ql/lib/ext/org.owasp.esapi.model.yml b/java/ql/lib/ext/org.owasp.esapi.model.yml index a8e59b385130..30578debe580 100644 --- a/java/ql/lib/ext/org.owasp.esapi.model.yml +++ b/java/ql/lib/ext/org.owasp.esapi.model.yml @@ -3,5 +3,4 @@ extensions: pack: codeql/java-all extensible: summaryModel data: - - ["org.owasp.esapi", "Encoder", true, "encodeForHTML", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"] - - ["org.owasp.esapi", "Encoder", true, "encodeForBase64", "(byte[],boolean)", "", "Argument[0]", "ReturnValue", "taint", "manual"] + - ["org.owasp.esapi", "Encoder", true, "encodeForHTML", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"] \ No newline at end of file diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 022215518f1e..c44048674e88 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -79,6 +79,14 @@ module WeakRandomnessConfig implements DataFlow::ConfigSig { 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 + ) } } diff --git a/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java b/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java index c9f28df8dfb6..b065081e460c 100644 --- a/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java +++ b/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java @@ -8,6 +8,7 @@ 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; @@ -20,9 +21,11 @@ public void doGet() { Cookie cookie = new Cookie("name", Integer.toString(c)); response.addCookie(cookie); // $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" + c2, "value"); + Cookie cookie2 = new Cookie("name", value); response.addCookie(cookie2); // $hasWeakRandomFlow byte[] bytes = new byte[16]; diff --git a/java/ql/test/query-tests/security/CWE-330/options b/java/ql/test/query-tests/security/CWE-330/options index e2910580186f..5977a9ee7105 100644 --- a/java/ql/test/query-tests/security/CWE-330/options +++ b/java/ql/test/query-tests/security/CWE-330/options @@ -1 +1 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/servlet-api-2.4:${testdir}/../../../stubs/apache-commons-lang3-3.7 \ No newline at end of file +//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; } } From 4bdf2b5e182df23a73cc442001ae5d02fb2d00a7 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Tue, 7 Nov 2023 16:51:52 -0500 Subject: [PATCH 21/29] Bump change note date --- ...ak-randomness-query.md => 2023-11-08-weak-randomness-query.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename java/ql/src/change-notes/{2023-08-01-weak-randomness-query.md => 2023-11-08-weak-randomness-query.md} (100%) diff --git a/java/ql/src/change-notes/2023-08-01-weak-randomness-query.md b/java/ql/src/change-notes/2023-11-08-weak-randomness-query.md similarity index 100% rename from java/ql/src/change-notes/2023-08-01-weak-randomness-query.md rename to java/ql/src/change-notes/2023-11-08-weak-randomness-query.md From bbf99375c78a6b6aa815ebca0f64943b7692674e Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Tue, 7 Nov 2023 17:07:58 -0500 Subject: [PATCH 22/29] Alter cookie sinks to instead focus on creation of a cookie --- .../java/security/WeakRandomnessQuery.qll | 14 ++++---- .../security/CWE-330/WeakRandomCookies.java | 32 +++++++------------ 2 files changed, 19 insertions(+), 27 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index c44048674e88..60d9aabdcdf3 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -46,15 +46,17 @@ private class TypeHadoopOsSecureRandom extends SafeRandomImplementation { abstract class WeakRandomnessSink extends DataFlow::Node { } /** - * A node which creates a cookie. + * A node which sets the value of a cookie. */ private class CookieSink extends WeakRandomnessSink { CookieSink() { - this.getType() instanceof TypeCookie and - exists(MethodCall mc | - mc.getMethod().hasQualifiedName("javax.servlet.http", "HttpServletResponse", "addCookie") - | - mc.getArgument(0) = this.asExpr() + 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) ) } } diff --git a/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java b/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java index b065081e460c..514783c95fb8 100644 --- a/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java +++ b/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java @@ -18,54 +18,44 @@ public void doGet() { int c = r.nextInt(); // BAD: The cookie value may be predictable. - Cookie cookie = new Cookie("name", Integer.toString(c)); - response.addCookie(cookie); // $hasWeakRandomFlow + 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); - response.addCookie(cookie2); // $hasWeakRandomFlow + 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)); - response.addCookie(cookie3); // $hasWeakRandomFlow + 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)); - response.addCookie(cookie4); - + Cookie cookie4 = new Cookie("name", new String(bytes2)); + ThreadLocalRandom tlr = ThreadLocalRandom.current(); - Cookie cookie5 = new Cookie("name", Integer.toString(tlr.nextInt())); - response.addCookie(cookie5); // $hasWeakRandomFlow + Cookie cookie5 = new Cookie("name", Integer.toString(tlr.nextInt())); // $hasWeakRandomFlow - Cookie cookie6 = new Cookie("name", RandomStringUtils.random(10)); - response.addCookie(cookie6); // $hasWeakRandomFlow + Cookie cookie6 = new Cookie("name", RandomStringUtils.random(10)); // $hasWeakRandomFlow - Cookie cookie7 = new Cookie("name", RandomStringUtils.randomAscii(10)); - response.addCookie(cookie7); // $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)); - response.addCookie(cookie8); // $hasWeakRandomFlow + 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)); - response.addCookie(cookie9); // $hasWeakRandomFlow + 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)); - response.addCookie(cookie10); // $hasWeakRandomFlow + Cookie cookie10 = new Cookie("name", Double.toString(++c5)); // $hasWeakRandomFlow } } From 4678302edbefb4db160d233423a4c371bbb95280 Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Tue, 14 Nov 2023 13:19:48 -0500 Subject: [PATCH 23/29] Update query metadata Co-authored-by: Tony Torralba --- java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql | 12 +++++++----- .../change-notes/2023-11-08-weak-randomness-query.md | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql b/java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql index 6b3dfa99712b..da3d0cd15404 100644 --- a/java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql +++ b/java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql @@ -1,11 +1,13 @@ /** - * @name Weak Randomness - * @description Using a weak source of randomness may allow an attacker to predict the generated values. + * @name Insecure randomness + * @description Using a cryptographically weak 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 error - * @security-severity 8.6 + * @problem.severity warning + * @security-severity 7.8 * @precision high - * @id java/weak-randomness + * @id java/insecure-randomness * @tags security * external/cwe/cwe-330 * external/cwe/cwe-338 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 index 6581f8751a50..9022f825af6e 100644 --- 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 @@ -1,5 +1,5 @@ --- category: newQuery --- -* Added the `java/weak-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. +* 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. From 6e70e6c85a9c34dae8cec30368de5bd6f2c65fd5 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Thu, 16 Nov 2023 11:08:10 -0500 Subject: [PATCH 24/29] Use pre-exisiting type for SecureRandom --- .../ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll index 60d9aabdcdf3..4fc8847ee9ec 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll @@ -30,9 +30,8 @@ private class RandomMethodSource extends WeakRandomnessSource { */ abstract private class SafeRandomImplementation extends RefType { } -private class TypeSecureRandom extends SafeRandomImplementation { - TypeSecureRandom() { this.hasQualifiedName("java.security", "SecureRandom") } -} +private class TypeSecureRandom extends SafeRandomImplementation instanceof SecureRandomNumberGenerator +{ } private class TypeHadoopOsSecureRandom extends SafeRandomImplementation { TypeHadoopOsSecureRandom() { From 3ca039bc8ff2aaceac2dfc733a7de72581ef7690 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Thu, 16 Nov 2023 11:11:30 -0500 Subject: [PATCH 25/29] Rename to InsecureRandomness --- ...sQuery.qll => InsecureRandomnessQuery.qll} | 30 +++++++++---------- ...domness.qhelp => InsecureRandomness.qhelp} | 0 .../CWE/CWE-330/InsecureRandomness.ql | 23 ++++++++++++++ .../Security/CWE/CWE-330/WeakRandomness.ql | 23 -------------- 4 files changed, 38 insertions(+), 38 deletions(-) rename java/ql/lib/semmle/code/java/security/{WeakRandomnessQuery.qll => InsecureRandomnessQuery.qll} (65%) rename java/ql/src/Security/CWE/CWE-330/{WeakRandomness.qhelp => InsecureRandomness.qhelp} (100%) create mode 100644 java/ql/src/Security/CWE/CWE-330/InsecureRandomness.ql delete mode 100644 java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql diff --git a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll similarity index 65% rename from java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll rename to java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll index 4fc8847ee9ec..d24d44e3805d 100644 --- a/java/ql/lib/semmle/code/java/security/WeakRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll @@ -1,4 +1,4 @@ -/** Provides classes and predicates for reasoning about weak randomness. */ +/** Provides classes and predicates for reasoning about insecure randomness. */ import java private import semmle.code.java.frameworks.Servlets @@ -9,13 +9,13 @@ private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.security.RandomQuery /** - * A node representing a source of weak randomness. + * A node representing a source of insecure randomness. * * For example, use of `java.util.Random` or `java.lang.Math.random`. */ -abstract class WeakRandomnessSource extends DataFlow::Node { } +abstract class InsecureRandomnessSource extends DataFlow::Node { } -private class RandomMethodSource extends WeakRandomnessSource { +private class RandomMethodSource extends InsecureRandomnessSource { RandomMethodSource() { exists(RandomDataSource s | this.asExpr() = s.getOutput() | not s.getQualifier().getType() instanceof SafeRandomImplementation @@ -40,14 +40,14 @@ private class TypeHadoopOsSecureRandom extends SafeRandomImplementation { } /** - * A node representing an operation which should not use a weakly random value. + * A node representing an operation which should not use a Insecurely random value. */ -abstract class WeakRandomnessSink extends DataFlow::Node { } +abstract class InsecureRandomnessSink extends DataFlow::Node { } /** * A node which sets the value of a cookie. */ -private class CookieSink extends WeakRandomnessSink { +private class CookieSink extends InsecureRandomnessSink { CookieSink() { exists(Call c | c.(ClassInstanceExpr).getConstructedType() instanceof TypeCookie and @@ -60,19 +60,19 @@ private class CookieSink extends WeakRandomnessSink { } } -private class SensitiveActionSink extends WeakRandomnessSink { +private class SensitiveActionSink extends InsecureRandomnessSink { SensitiveActionSink() { this.asExpr() instanceof SensitiveExpr } } -private class CredentialsSink extends WeakRandomnessSink instanceof CredentialsSinkNode { } +private class CredentialsSink extends InsecureRandomnessSink instanceof CredentialsSinkNode { } /** - * A taint-tracking configuration for weak randomness. + * A taint-tracking configuration for Insecure randomness. */ -module WeakRandomnessConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node src) { src instanceof WeakRandomnessSource } +module InsecureRandomnessConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src instanceof InsecureRandomnessSource } - predicate isSink(DataFlow::Node sink) { sink instanceof WeakRandomnessSink } + predicate isSink(DataFlow::Node sink) { sink instanceof InsecureRandomnessSink } predicate isBarrierIn(DataFlow::Node n) { isSource(n) } @@ -92,6 +92,6 @@ module WeakRandomnessConfig implements DataFlow::ConfigSig { } /** - * Taint-tracking flow of a weakly random value into a sensitive sink. + * Taint-tracking flow of a Insecurely random value into a sensitive sink. */ -module WeakRandomnessFlow = TaintTracking::Global; +module InsecureRandomnessFlow = TaintTracking::Global; diff --git a/java/ql/src/Security/CWE/CWE-330/WeakRandomness.qhelp b/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp similarity index 100% rename from java/ql/src/Security/CWE/CWE-330/WeakRandomness.qhelp rename to java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp 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/WeakRandomness.ql b/java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql deleted file mode 100644 index da3d0cd15404..000000000000 --- a/java/ql/src/Security/CWE/CWE-330/WeakRandomness.ql +++ /dev/null @@ -1,23 +0,0 @@ -/** - * @name Insecure randomness - * @description Using a cryptographically weak 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.WeakRandomnessQuery -import WeakRandomnessFlow::PathGraph - -from WeakRandomnessFlow::PathNode source, WeakRandomnessFlow::PathNode sink -where WeakRandomnessFlow::flowPath(source, sink) -select sink.getNode(), source, sink, "Potential weak randomness due to a $@.", source.getNode(), - "weak randomness source." From 1271cd3348c4a959a825c424686d1efcc8b0c205 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 29 Nov 2023 11:31:51 -0500 Subject: [PATCH 26/29] Remove unnecessary crypto sinks --- java/ql/lib/ext/java.security.spec.model.yml | 9 +++------ java/ql/lib/ext/javax.crypto.spec.model.yml | 4 +--- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/java/ql/lib/ext/java.security.spec.model.yml b/java/ql/lib/ext/java.security.spec.model.yml index f2834224b30a..adb62372a824 100644 --- a/java/ql/lib/ext/java.security.spec.model.yml +++ b/java/ql/lib/ext/java.security.spec.model.yml @@ -3,14 +3,11 @@ extensions: pack: codeql/java-all extensible: sinkModel data: - - ["java.security.spec", "DSAParameterSpec", False, "DSAParameterSpec", "", "", "Argument[0..2]", "credentials-key", "manual"] - ["java.security.spec", "DSAPrivateKeySpec", False, "DSAPrivateKeySpec", "", "", "Argument[0..3]", "credentials-key", "manual"] - - ["java.security.spec", "DSAPublicKeySpec", False, "DSAPublicKeySpec", "", "", "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[0..8]", "credentials-key", "manual"] - - ["java.security.spec", "RSAPrivateCrtKeySpec", False, "RSAPrivateCrtKeySpec", "", "", "Argument[0..7]", "credentials-key", "manual"] - - ["java.security.spec", "RSAPrivateKeySpec", False, "RSAPrivateKeySpec", "", "", "Argument[0..1]", "credentials-key", "manual"] - - ["java.security.spec", "RSAPublicKeySpec", False, "RSAPublicKeySpec", "", "", "Argument[0..1]", "credentials-key", "manual"] + - ["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 a81811930147..0f879c1f900c 100644 --- a/java/ql/lib/ext/javax.crypto.spec.model.yml +++ b/java/ql/lib/ext/javax.crypto.spec.model.yml @@ -18,9 +18,7 @@ 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[1..3]", "credentials-key", "manual"] - - ["javax.crypto.spec", "DHPublicKeySpec", False, "DHPublicKeySpec", "", "", "Argument[1..3]", "credentials-key", "manual"] - - ["javax.crypto.spec", "DSAParameterSpec", False, "DSAParameterSpec", "", "", "Argument[1..3]", "credentials-key", "manual"] + - ["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"] From 736215822926ea47f03ec22690a69da0e2a8c0b0 Mon Sep 17 00:00:00 2001 From: Ed Minnix Date: Wed, 29 Nov 2023 11:35:44 -0500 Subject: [PATCH 27/29] Fix test case --- ...eakRandomTest.expected => InsecureRandomnessTest.expected} | 0 .../CWE-330/{WeakRandomTest.ql => InsecureRandomnessTest.ql} | 4 ++-- 2 files changed, 2 insertions(+), 2 deletions(-) rename java/ql/test/query-tests/security/CWE-330/{WeakRandomTest.expected => InsecureRandomnessTest.expected} (100%) rename java/ql/test/query-tests/security/CWE-330/{WeakRandomTest.ql => InsecureRandomnessTest.ql} (78%) diff --git a/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.expected b/java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.expected similarity index 100% rename from java/ql/test/query-tests/security/CWE-330/WeakRandomTest.expected rename to java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.expected diff --git a/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.ql b/java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.ql similarity index 78% rename from java/ql/test/query-tests/security/CWE-330/WeakRandomTest.ql rename to java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.ql index c2025172a0f5..a2b6f329ae80 100644 --- a/java/ql/test/query-tests/security/CWE-330/WeakRandomTest.ql +++ b/java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.ql @@ -1,6 +1,6 @@ import java import semmle.code.java.dataflow.DataFlow -import semmle.code.java.security.WeakRandomnessQuery +import semmle.code.java.security.InsecureRandomnessQuery import TestUtilities.InlineExpectationsTest module WeakRandomTest implements TestSig { @@ -8,7 +8,7 @@ module WeakRandomTest implements TestSig { predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasWeakRandomFlow" and - exists(DataFlow::Node sink | WeakRandomnessFlow::flowTo(sink) | + exists(DataFlow::Node sink | InsecureRandomnessFlow::flowTo(sink) | sink.getLocation() = location and element = sink.toString() and value = "" From ce20c4ae032db5b3c462fd3afd188b08a037afff Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Fri, 8 Dec 2023 10:37:29 -0500 Subject: [PATCH 28/29] Docs review suggestions Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com> --- .../src/Security/CWE/CWE-330/InsecureRandomness.qhelp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp b/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp index 2de585b753c7..912c423bd659 100644 --- a/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp +++ b/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp @@ -4,13 +4,13 @@

    - Using a cryptographically weak pseudo-random number generator to generate a security-sensitive value, - such as a password, makes it easier for an attacker to predict the value. + 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 + 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.

    @@ -18,7 +18,7 @@

    Use a cryptographically secure pseudo-random number generator if the output is to be used in a - security-sensitive context. As a rule of thumb, a value should be considered "security-sensitive" + 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. @@ -36,7 +36,7 @@

    - In the first case, we generate a fresh cookie by appending a random integer to the end of a static + 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.

    From 06eef93f89b18daf2d21db1916f9c4deee349814 Mon Sep 17 00:00:00 2001 From: Edward Minnix III Date: Fri, 8 Dec 2023 10:40:48 -0500 Subject: [PATCH 29/29] Docs review suggestions --- .../CWE/CWE-330/InsecureRandomness.qhelp | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp b/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp index 912c423bd659..414e32520577 100644 --- a/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp +++ b/java/ql/src/Security/CWE/CWE-330/InsecureRandomness.qhelp @@ -16,17 +16,16 @@     -

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

    -

    - For Java, java.util.Random is not cryptographically secure. Use java.security.SecureRandom instead. + 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. +

    @@ -44,7 +43,7 @@

    - In the second case, we generate a fresh cookie by appending a random integer to the end of a static + 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.