Refactor to path query

Author: atorralba

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"

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

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 = ""