From 435d1f97a311517d9ad5d7b26831a83401dc90b1 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 2 Nov 2023 14:33:42 +0100 Subject: [PATCH 1/6] Add sink for OpenSAML's RequestType.setID --- .../semmle/code/java/frameworks/OpenSaml.qll | 29 +++++++++++++++++++ .../java/security/InsecureRandomnessQuery.qll | 3 +- 2 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 java/ql/lib/semmle/code/java/frameworks/OpenSaml.qll diff --git a/java/ql/lib/semmle/code/java/frameworks/OpenSaml.qll b/java/ql/lib/semmle/code/java/frameworks/OpenSaml.qll new file mode 100644 index 000000000000..c8b9a320ec1b --- /dev/null +++ b/java/ql/lib/semmle/code/java/frameworks/OpenSaml.qll @@ -0,0 +1,29 @@ +/** + * Provides classes and predicates for working with the OpenSAML libraries. + */ + +import java +private import semmle.code.java.security.InsecureRandomnessQuery + +/** The interface `org.opensaml.saml.saml2.core.RequestAbstractType`. */ +class SamlRequestAbstractType extends Interface { + SamlRequestAbstractType() { + this.hasQualifiedName("org.opensaml.saml.saml2.core", "RequestAbstractType") + } +} + +/** The method `setID` of the interface `RequestAbstractType`. */ +class SamlRequestSetIdMethod extends Method { + SamlRequestSetIdMethod() { + this.getDeclaringType() instanceof SamlRequestAbstractType and + this.hasName("setID") + } +} + +private class SamlRequestSetIdSink extends InsecureRandomnessSink { + SamlRequestSetIdSink() { + exists(MethodCall c | c.getMethod() instanceof SamlRequestSetIdMethod | + c.getArgument(0) = this.asExpr() + ) + } +} diff --git a/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll index d24d44e3805d..59e264504031 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll @@ -1,6 +1,7 @@ /** Provides classes and predicates for reasoning about insecure randomness. */ import java +private import semmle.code.java.frameworks.OpenSaml private import semmle.code.java.frameworks.Servlets private import semmle.code.java.security.SensitiveActions private import semmle.code.java.security.SensitiveApi @@ -40,7 +41,7 @@ private class TypeHadoopOsSecureRandom extends SafeRandomImplementation { } /** - * A node representing an operation which should not use a Insecurely random value. + * A node representing an operation which should not use an insecurely random value. */ abstract class InsecureRandomnessSink extends DataFlow::Node { } From 3a5d711711c8aae29739fe9f9c9e196c2a7980ec Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 3 Nov 2023 12:57:56 +0100 Subject: [PATCH 2/6] Add cookie sinks --- .../lib/semmle/code/java/frameworks/Netty.qll | 23 +++++++++++++ .../semmle/code/java/frameworks/Servlets.qll | 12 ++++++- .../lib/semmle/code/java/security/Cookies.qll | 32 +++++++++++++++++++ .../java/security/InsecureRandomnessQuery.qll | 29 +++++++++-------- 4 files changed, 82 insertions(+), 14 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/frameworks/Netty.qll create mode 100644 java/ql/lib/semmle/code/java/security/Cookies.qll diff --git a/java/ql/lib/semmle/code/java/frameworks/Netty.qll b/java/ql/lib/semmle/code/java/frameworks/Netty.qll new file mode 100644 index 000000000000..9a72c7f64043 --- /dev/null +++ b/java/ql/lib/semmle/code/java/frameworks/Netty.qll @@ -0,0 +1,23 @@ +/** Provides definitions related to the Netty framework. */ + +import java + +/** The interface `Cookie` in the packages `io.netty.handler.codec.http` and `io.netty.handler.codec.http.cookie`. */ +class NettyCookie extends Interface { + NettyCookie() { this.hasQualifiedName("io.netty.handler.codec.http" + [".cookie", ""], "Cookie") } +} + +/** The class `DefaultCookie` in the packages `io.netty.handler.codec.http` and `io.netty.handler.codec.http.cookie`. */ +class NettyDefaultCookie extends Class { + NettyDefaultCookie() { + this.hasQualifiedName("io.netty.handler.codec.http" + [".cookie", ""], "DefaultCookie") + } +} + +/** The method `setValue` of the interface `Cookie` or a class implementing it. */ +class NettySetCookieValueMethod extends Method { + NettySetCookieValueMethod() { + this.getDeclaringType*() instanceof NettyCookie and + this.hasName("setValue") + } +} diff --git a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll index 88b8c5c5d976..cb4e5c1bba7f 100644 --- a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll +++ b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll @@ -244,7 +244,7 @@ class TypeCookie extends Class { } /** - * The method `getValue(String)` declared in `javax.servlet.http.Cookie`. + * The method `getValue()` declared in `javax.servlet.http.Cookie`. */ class CookieGetValueMethod extends Method { CookieGetValueMethod() { @@ -254,6 +254,16 @@ class CookieGetValueMethod extends Method { } } +/** + * The method `setValue(String)` declared in `javax.servlet.http.Cookie`. + */ +class CookieSetValueMethod extends Method { + CookieSetValueMethod() { + this.getDeclaringType() instanceof TypeCookie and + this.hasName("setValue") + } +} + /** * The method `getName()` declared in `javax.servlet.http.Cookie`. */ diff --git a/java/ql/lib/semmle/code/java/security/Cookies.qll b/java/ql/lib/semmle/code/java/security/Cookies.qll new file mode 100644 index 000000000000..202f18921ca6 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/Cookies.qll @@ -0,0 +1,32 @@ +/** Provides definitions to reason about HTTP cookies. */ + +import java +private import semmle.code.java.frameworks.Netty +private import semmle.code.java.frameworks.Servlets + +/** An expression setting the value of a cookie. */ +abstract class SetCookieValue extends Expr { } + +private class ServletSetCookieValue extends SetCookieValue { + ServletSetCookieValue() { + exists(Call c | + c.(ClassInstanceExpr).getConstructedType() instanceof TypeCookie and + this = c.getArgument(1) + or + c.(MethodCall).getMethod() instanceof CookieSetValueMethod and + this = c.getArgument(0) + ) + } +} + +private class NettySetCookieValue extends SetCookieValue { + NettySetCookieValue() { + exists(Call c | + c.(ClassInstanceExpr).getConstructedType() instanceof NettyDefaultCookie and + this = c.getArgument(1) + or + c.(MethodCall).getMethod() instanceof NettySetCookieValueMethod and + this = c.getArgument(0) + ) + } +} diff --git a/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll index 59e264504031..efab64ab6115 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll @@ -3,11 +3,12 @@ import java private import semmle.code.java.frameworks.OpenSaml 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.dataflow.TaintTracking +private import semmle.code.java.security.Cookies private import semmle.code.java.security.RandomQuery +private import semmle.code.java.security.SensitiveActions +private import semmle.code.java.security.SensitiveApi /** * A node representing a source of insecure randomness. @@ -49,16 +50,7 @@ abstract class InsecureRandomnessSink extends DataFlow::Node { } * A node which sets the value of a cookie. */ private class CookieSink extends InsecureRandomnessSink { - CookieSink() { - exists(Call c | - c.(ClassInstanceExpr).getConstructedType() instanceof TypeCookie and - this.asExpr() = c.getArgument(1) - or - c.(MethodCall).getMethod().getDeclaringType() instanceof TypeCookie and - c.(MethodCall).getMethod().hasName("setValue") and - this.asExpr() = c.getArgument(0) - ) - } + CookieSink() { this.asExpr() instanceof SetCookieValue } } private class SensitiveActionSink extends InsecureRandomnessSink { @@ -89,6 +81,17 @@ module InsecureRandomnessConfig implements DataFlow::ConfigSig { n1.asExpr() = mc.getArgument(0) and n2.asExpr() = mc ) + or + // TODO: Once we have a default sanitizer for UUIDs, we can convert these to global summaries. + exists(Call c | + c.(ClassInstanceExpr).getConstructedType().hasQualifiedName("java.util", "UUID") and + n1.asExpr() = c.getAnArgument() and + n2.asExpr() = c + or + c.(MethodCall).getMethod().hasQualifiedName("java.util", "UUID", "toString") and + n1.asExpr() = c.getQualifier() and + n2.asExpr() = c + ) } } From 7bc907840cb7e09dd63b9676bf08a3f66374ce7a Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 6 Nov 2023 13:19:52 +0100 Subject: [PATCH 3/6] Fix tests --- .../query-tests/security/CWE-330/WeakRandomCookies.java | 8 ++++++++ java/ql/test/query-tests/security/CWE-330/options | 2 +- .../netty-4.1.x/io/netty/handler/codec/http/Cookie.java | 6 ++++++ .../io/netty/handler/codec/http/DefaultCookie.java | 9 +++++++++ 4 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 java/ql/test/stubs/netty-4.1.x/io/netty/handler/codec/http/Cookie.java create mode 100644 java/ql/test/stubs/netty-4.1.x/io/netty/handler/codec/http/DefaultCookie.java 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 514783c95fb8..f7e3c27fa2c1 100644 --- a/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java +++ b/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java @@ -19,6 +19,14 @@ public void doGet() { int c = r.nextInt(); // BAD: The cookie value may be predictable. Cookie cookie = new Cookie("name", Integer.toString(c)); // $hasWeakRandomFlow + cookie.setValue(Integer.toString(c)); // $hasWeakRandomFlow + + io.netty.handler.codec.http.Cookie nettyCookie = + new io.netty.handler.codec.http.DefaultCookie("name", Integer.toString(c)); // $hasWeakRandomFlow + nettyCookie.setValue(Integer.toString(c)); // $hasWeakRandomFlow + io.netty.handler.codec.http.cookie.Cookie nettyCookie2 = + new io.netty.handler.codec.http.cookie.DefaultCookie("name", Integer.toString(c)); // $hasWeakRandomFlow + nettyCookie2.setValue(Integer.toString(c)); // $hasWeakRandomFlow Encoder enc = null; int c2 = r.nextInt(); diff --git a/java/ql/test/query-tests/security/CWE-330/options b/java/ql/test/query-tests/security/CWE-330/options index 5977a9ee7105..671a6c7bd4ff 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:${testdir}/../../../stubs/esapi-2.0.1 \ 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:${testdir}/../../../stubs/netty-4.1.x diff --git a/java/ql/test/stubs/netty-4.1.x/io/netty/handler/codec/http/Cookie.java b/java/ql/test/stubs/netty-4.1.x/io/netty/handler/codec/http/Cookie.java new file mode 100644 index 000000000000..b9bc9ac50ee8 --- /dev/null +++ b/java/ql/test/stubs/netty-4.1.x/io/netty/handler/codec/http/Cookie.java @@ -0,0 +1,6 @@ +// Generated automatically from io.netty.handler.codec.http.cookie.Cookie for testing purposes + +package io.netty.handler.codec.http; + +public interface Cookie extends io.netty.handler.codec.http.cookie.Cookie { +} diff --git a/java/ql/test/stubs/netty-4.1.x/io/netty/handler/codec/http/DefaultCookie.java b/java/ql/test/stubs/netty-4.1.x/io/netty/handler/codec/http/DefaultCookie.java new file mode 100644 index 000000000000..ce5c6834a03e --- /dev/null +++ b/java/ql/test/stubs/netty-4.1.x/io/netty/handler/codec/http/DefaultCookie.java @@ -0,0 +1,9 @@ +// Generated automatically from io.netty.handler.codec.http.cookie.DefaultCookie for testing +// purposes + +package io.netty.handler.codec.http; + +public class DefaultCookie extends io.netty.handler.codec.http.cookie.DefaultCookie + implements Cookie { + public DefaultCookie(String p0, String p1) {} +} From fc45621ab1aadc78256641835f574147445ea9d2 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 8 Nov 2023 10:04:11 +0100 Subject: [PATCH 4/6] Add pac4j JWT cryptographic key sinks --- .../ql/lib/ext/org.pac4j.jwt.config.encryption.model.yml | 9 +++++++++ java/ql/lib/ext/org.pac4j.jwt.config.signature.model.yml | 9 +++++++++ 2 files changed, 18 insertions(+) create mode 100644 java/ql/lib/ext/org.pac4j.jwt.config.encryption.model.yml create mode 100644 java/ql/lib/ext/org.pac4j.jwt.config.signature.model.yml diff --git a/java/ql/lib/ext/org.pac4j.jwt.config.encryption.model.yml b/java/ql/lib/ext/org.pac4j.jwt.config.encryption.model.yml new file mode 100644 index 000000000000..353c3c02be5d --- /dev/null +++ b/java/ql/lib/ext/org.pac4j.jwt.config.encryption.model.yml @@ -0,0 +1,9 @@ +extensions: + - addsTo: + pack: codeql/java-all + extensible: sinkModel + data: + - ["org.pac4j.jwt.config.encryption", "SecretEncryptionConfiguration", True, "SecretEncryptionConfiguration", "", "", "Argument[0]", "credentials-key", "manual"] + - ["org.pac4j.jwt.config.encryption", "SecretEncryptionConfiguration", True, "setSecret", "", "", "Argument[0]", "credentials-key", "manual"] + - ["org.pac4j.jwt.config.encryption", "SecretEncryptionConfiguration", True, "setSecretBase64", "", "", "Argument[0]", "credentials-key", "manual"] + - ["org.pac4j.jwt.config.encryption", "SecretEncryptionConfiguration", True, "setSecretBytes", "", "", "Argument[0]", "credentials-key", "manual"] diff --git a/java/ql/lib/ext/org.pac4j.jwt.config.signature.model.yml b/java/ql/lib/ext/org.pac4j.jwt.config.signature.model.yml new file mode 100644 index 000000000000..89fc9f337166 --- /dev/null +++ b/java/ql/lib/ext/org.pac4j.jwt.config.signature.model.yml @@ -0,0 +1,9 @@ +extensions: + - addsTo: + pack: codeql/java-all + extensible: sinkModel + data: + - ["org.pac4j.jwt.config.signature", "SecretSignatureConfiguration", True, "SecretEncryptionConfiguration", "", "", "Argument[0]", "credentials-key", "manual"] + - ["org.pac4j.jwt.config.signature", "SecretSignatureConfiguration", True, "setSecret", "", "", "Argument[0]", "credentials-key", "manual"] + - ["org.pac4j.jwt.config.signature", "SecretSignatureConfiguration", True, "setSecretBase64", "", "", "Argument[0]", "credentials-key", "manual"] + - ["org.pac4j.jwt.config.signature", "SecretSignatureConfiguration", True, "setSecretBytes", "", "", "Argument[0]", "credentials-key", "manual"] From d955dce72a8271ae1693485d0879ebe8f0672a7f Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 8 Nov 2023 12:14:24 +0100 Subject: [PATCH 5/6] Improve source of randomness detection Also sanitize flow out of sinks to avoid overlapping paths --- .../java/security/InsecureRandomnessQuery.qll | 4 +++- .../code/java/security/RandomDataSource.qll | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll index efab64ab6115..f983876d7b38 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll @@ -20,7 +20,7 @@ abstract class InsecureRandomnessSource extends DataFlow::Node { } private class RandomMethodSource extends InsecureRandomnessSource { RandomMethodSource() { exists(RandomDataSource s | this.asExpr() = s.getOutput() | - not s.getQualifier().getType() instanceof SafeRandomImplementation + not s.getSourceOfRandomness() instanceof SafeRandomImplementation ) } } @@ -69,6 +69,8 @@ module InsecureRandomnessConfig implements DataFlow::ConfigSig { predicate isBarrierIn(DataFlow::Node n) { isSource(n) } + predicate isBarrierOut(DataFlow::Node n) { isSink(n) } + predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { n1.asExpr() = n2.asExpr().(BinaryExpr).getAnOperand() or diff --git a/java/ql/lib/semmle/code/java/security/RandomDataSource.qll b/java/ql/lib/semmle/code/java/security/RandomDataSource.qll index 05ba28ba450c..b44bcc07efe2 100644 --- a/java/ql/lib/semmle/code/java/security/RandomDataSource.qll +++ b/java/ql/lib/semmle/code/java/security/RandomDataSource.qll @@ -3,6 +3,7 @@ */ import java +private import semmle.code.java.dataflow.TypeFlow /** * A method access that returns random data or writes random data to an argument. @@ -43,6 +44,9 @@ abstract class RandomDataSource extends MethodCall { * in the case where it writes random data to that argument. */ abstract Expr getOutput(); + + /** Gets the type of the source of randomness used by this call. */ + RefType getSourceOfRandomness() { boundOrStaticType(this.getQualifier(), result) } } /** @@ -167,4 +171,18 @@ class ApacheCommonsRandomStringSource extends RandomDataSource { } override Expr getOutput() { result = this } + + override RefType getSourceOfRandomness() { + if + this.getMethod().hasStringSignature("random(int, int, int, boolean, boolean, char[], Random)") + then boundOrStaticType(this.getArgument(6), result) + else result.hasQualifiedName("java.util", "Random") + } +} + +/** Holds if `t` is the static type of `e`, or an upper bound of the runtime type of `e`. */ +private predicate boundOrStaticType(Expr e, RefType t) { + exprTypeFlow(e, t, false) + or + t = e.getType() } From 66b54f03b75480440ea7d7d0c165c15fa2b43a7b Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 13 Dec 2023 11:14:48 +0100 Subject: [PATCH 6/6] Rename test --- .../{WeakRandomCookies.java => InsecureRandomCookies.java} | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename java/ql/test/query-tests/security/CWE-330/{WeakRandomCookies.java => InsecureRandomCookies.java} (97%) diff --git a/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java b/java/ql/test/query-tests/security/CWE-330/InsecureRandomCookies.java similarity index 97% rename from java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java rename to java/ql/test/query-tests/security/CWE-330/InsecureRandomCookies.java index f7e3c27fa2c1..d34a0283313e 100644 --- a/java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java +++ b/java/ql/test/query-tests/security/CWE-330/InsecureRandomCookies.java @@ -10,7 +10,7 @@ import org.apache.commons.lang3.RandomStringUtils; import org.owasp.esapi.Encoder; -public class WeakRandomCookies extends HttpServlet { +public class InsecureRandomCookies extends HttpServlet { HttpServletResponse response; public void doGet() { @@ -44,8 +44,8 @@ public void doGet() { byte[] bytes2 = new byte[16]; sr.nextBytes(bytes2); // GOOD: The cookie value is unpredictable. - Cookie cookie4 = new Cookie("name", new String(bytes2)); - + Cookie cookie4 = new Cookie("name", new String(bytes2)); + ThreadLocalRandom tlr = ThreadLocalRandom.current(); Cookie cookie5 = new Cookie("name", Integer.toString(tlr.nextInt())); // $hasWeakRandomFlow