diff --git a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql index ad6e41320ee..b17915a8fd5 100644 --- a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql +++ b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql @@ -1,8 +1,9 @@ /** * @name Missing JWT signature check * @description Failing to check the JWT signature may allow an attacker to forge their own tokens. - * @kind problem + * @kind path-problem * @problem.severity error + * @security-severity 7.8 * @precision high * @id java/missing-jwt-signature-check * @tags security @@ -11,8 +12,9 @@ import java import semmle.code.java.security.MissingJWTSignatureCheckQuery +import DataFlow::PathGraph -from JwtParserWithInsecureParseSink sink, JwtParserWithSigningKeyExpr parserExpr -where sink.asExpr() = parserExpr -select sink.getParseMethodAccess(), "A signing key is set $@, but the signature is not verified.", - parserExpr.getSigningMethodAccess(), "here" +from DataFlow::PathNode source, DataFlow::PathNode sink, MissingJwtSignatureCheckConf conf +where conf.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "A signing key is set $@, but the signature is not verified.", + source.getNode(), "here" diff --git a/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll index 634fac69c61..f70b9ab6447 100644 --- a/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll +++ b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll @@ -5,43 +5,12 @@ import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.ExternalFlow import semmle.code.java.security.JWT -/** - * An expr that is a (sub-type of) `JwtParser` for which a signing key has been set. - */ -class JwtParserWithSigningKeyExpr extends Expr { - MethodAccess signingMa; - - JwtParserWithSigningKeyExpr() { isSigningKeySetter(this, signingMa) } - - /** Gets the method access that sets the signing key for this parser. */ - MethodAccess getSigningMethodAccess() { result = signingMa } -} - -/** - * Holds if `signingMa` directly or indirectly sets a signing key for `expr`, which is a `JwtParser`. - * The `setSigningKey` and `setSigningKeyResolver` methods set a signing key for a `JwtParser`. - * - * Directly means code like this (the signing key is set directly on a `JwtParser`): - * ```java - * Jwts.parser().setSigningKey(key).parse(token); - * ``` - * - * Indirectly means code like this (the signing key is set on a `JwtParserBuilder` indirectly setting the key of `JwtParser` that is created by the call to `build`): - * ```java - * Jwts.parserBuilder().setSigningKey(key).build().parse(token); - * ``` - */ -private predicate isSigningKeySetter(Expr expr, MethodAccess signingMa) { - any(SigningToInsecureMethodAccessDataFlow s) - .hasFlow(DataFlow::exprNode(signingMa), DataFlow::exprNode(expr)) -} - /** * Models flow from signing keys assignements to qualifiers of JWT insecure parsers. - * This is used to determine whether a `JwtParser` has a signing key set. + * This is used to determine whether a `JwtParser` performing unsafe parsing has a signing key set. */ -private class SigningToInsecureMethodAccessDataFlow extends DataFlow::Configuration { - SigningToInsecureMethodAccessDataFlow() { this = "SigningToExprDataFlow" } +class MissingJwtSignatureCheckConf extends DataFlow::Configuration { + MissingJwtSignatureCheckConf() { this = "SigningToExprDataFlow" } override predicate isSource(DataFlow::Node source) { source instanceof JwtParserWithInsecureParseSource diff --git a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql index d9d4be1e4d5..557fc28bf01 100644 --- a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql +++ b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql @@ -9,8 +9,9 @@ class HasMissingJwtSignatureCheckTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasMissingJwtSignatureCheck" and - exists(JwtParserWithInsecureParseSink sink, JwtParserWithSigningKeyExpr parserExpr | - sink.asExpr() = parserExpr and + exists(DataFlow::Node source, DataFlow::Node sink, MissingJwtSignatureCheckConf conf | + conf.hasFlow(source, sink) + | sink.getLocation() = location and element = sink.toString() and value = ""