Refactor JWT.qll

Author: atorralba

java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql

@@ -10,7 +10,7 @@
  */
 
 import java
-import semmle.code.java.security.JWT
+import semmle.code.java.security.JWTQuery
 
 from JwtParserWithInsecureParseSink sink, JwtParserWithSigningKeyExpr parserExpr
 where sink.asExpr() = parserExpr

java/ql/src/semmle/code/java/security/JWT.qll

@@ -1,65 +1,71 @@
 /** Provides classes for working with JWT libraries. */
 
 import java
-import semmle.code.java.dataflow.DataFlow
-import semmle.code.java.dataflow.ExternalFlow
-
-/**
- * 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 }
+private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.dataflow.DataFlow
+
+/** A method access that assigns signing keys to a JWT parser. */
+class JwtParserWithInsecureParseSource extends DataFlow::Node {
+  JwtParserWithInsecureParseSource() {
+    exists(MethodAccess ma, Method m |
+      m.getDeclaringType().getASupertype*() instanceof TypeJwtParser or
+      m.getDeclaringType().getASupertype*() instanceof TypeJwtParserBuilder
+    |
+      this.asExpr() = ma and
+      ma.getMethod() = m and
+      m.hasName(["setSigningKey", "setSigningKeyResolver"])
+    )
+  }
 }
 
 /**
  * The qualifier of an insecure parsing method.
- * That is, either the qualifier of a call to a `parse(token)`, `parseClaimsJwt(token)` or `parsePlaintextJwt(token)` method or
- * the qualifier of a call to a `parse(token, handler)` method where the `handler` is considered insecure.
+ * That is, either the qualifier of a call to the `parse(token)`,
+ * `parseClaimsJwt(token)` or `parsePlaintextJwt(token)` methods or
+ * the qualifier of a call to a `parse(token, handler)` method
+ * where the `handler` is considered insecure.
  */
 class JwtParserWithInsecureParseSink extends DataFlow::Node {
   MethodAccess insecureParseMa;
 
   JwtParserWithInsecureParseSink() {
     insecureParseMa.getQualifier() = this.asExpr() and
-    (
-      sinkNode(this, "jwt-insecure-parse")
-      or
-      sinkNode(this, "jwt-insecure-parse-handler") and
-      isInsecureJwtHandler(insecureParseMa.getArgument(1))
+    exists(Method m |
+      insecureParseMa.getMethod() = m and
+      m.getDeclaringType().getASupertype*() instanceof TypeJwtParser and
+      m.hasName(["parse", "parseClaimsJwt", "parsePlaintextJwt"]) and
+      (
+        m.getNumberOfParameters() = 1
+        or
+        isInsecureJwtHandler(insecureParseMa.getArgument(1))
+      )
     )
   }
 
   /** Gets the method access that does the insecure parsing. */
   MethodAccess getParseMethodAccess() { result = insecureParseMa }
 }
 
-/**
- * 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))
+/** A set of additional taint steps to consider when taint tracking JWT related data flows. */
+class JwtParserWithInsecureParseAdditionalTaintStep extends Unit {
+  predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+    jwtParserStep(node1.asExpr(), node2.asExpr())
+  }
+}
+
+/** Models the builder style of `JwtParser` and `JwtParserBuilder`. */
+private predicate jwtParserStep(Expr parser, MethodAccess ma) {
+  (
+    parser.getType().(RefType).getASourceSupertype*() instanceof TypeJwtParser or
+    parser.getType().(RefType).getASourceSupertype*() instanceof TypeJwtParserBuilder
+  ) and
+  ma.getQualifier() = parser
 }
 
 /**
  * Holds if `parseHandlerExpr` is an insecure `JwtHandler`.
- * That is, it overrides a method from `JwtHandlerOnJwtMethod` and the override is not defined on `JwtHandlerAdapter`.
+ * That is, it overrides a method from `JwtHandlerOnJwtMethod` and
+ * the override is not defined on `JwtHandlerAdapter`.
  * `JwtHandlerAdapter`'s overrides are safe since they always throw an exception.
  */
 private predicate isInsecureJwtHandler(Expr parseHandlerExpr) {
@@ -74,66 +80,6 @@ private predicate isInsecureJwtHandler(Expr parseHandlerExpr) {
   )
 }
 
-/** CSV source models representing methods that assign signing keys to a JWT parser. */
-private class SigningKeySourceModel extends SourceModelCsv {
-  override predicate row(string row) {
-    row =
-      [
-        "io.jsonwebtoken;JwtParser;true;setSigningKey;;;ReturnValue;jwt-signing-key",
-        "io.jsonwebtoken;JwtParser;true;setSigningKeyResolver;;;ReturnValue;jwt-signing-key",
-        "io.jsonwebtoken;JwtParserBuilder;true;setSigningKey;;;ReturnValue;jwt-signing-key",
-        "io.jsonwebtoken;JwtParserBuilder;true;setSigningKeyResolver;;;ReturnValue;jwt-signing-key"
-      ]
-  }
-}
-
-/** CSV sink models representing qualifiers of methods that parse a JWT insecurely. */
-private class InsecureJwtParseSinkModel extends SinkModelCsv {
-  override predicate row(string row) {
-    row =
-      [
-        "io.jsonwebtoken;JwtParser;true;parse;(String);;Argument[-1];jwt-insecure-parse",
-        "io.jsonwebtoken;JwtParser;true;parseClaimsJwt;;;Argument[-1];jwt-insecure-parse",
-        "io.jsonwebtoken;JwtParser;true;parsePlaintextJwt;;;Argument[-1];jwt-insecure-parse"
-      ]
-  }
-}
-
-/** CSV sink models representing qualifiers of methods that insecurely parse a JWT with a handler */
-private class InsecureJwtParseHandlerSinkModel extends SinkModelCsv {
-  override predicate row(string row) {
-    row =
-      [
-        "io.jsonwebtoken;JwtParser;true;parse;(String,JwtHandler);;Argument[-1];jwt-insecure-parse-handler"
-      ]
-  }
-}
-
-/** Models the builder style of `JwtParser` and `JwtParserBuilder`. */
-private predicate jwtParserStep(Expr parser, MethodAccess ma) {
-  (
-    parser.getType().(RefType).getASourceSupertype*() instanceof TypeJwtParser or
-    parser.getType().(RefType).getASourceSupertype*() instanceof TypeJwtParserBuilder
-  ) and
-  ma.getQualifier() = parser
-}
-
-/**
- * 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.
- */
-private class SigningToInsecureMethodAccessDataFlow extends DataFlow::Configuration {
-  SigningToInsecureMethodAccessDataFlow() { this = "SigningToExprDataFlow" }
-
-  override predicate isSource(DataFlow::Node source) { sourceNode(source, "jwt-signing-key") }
-
-  override predicate isSink(DataFlow::Node sink) { sink instanceof JwtParserWithInsecureParseSink }
-
-  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    jwtParserStep(node1.asExpr(), node2.asExpr())
-  }
-}
-
 /** The interface `io.jsonwebtoken.JwtParser`. */
 private class TypeJwtParser extends Interface {
   TypeJwtParser() { this.hasQualifiedName("io.jsonwebtoken", "JwtParser") }

java/ql/src/semmle/code/java/security/JWTQuery.qll

@@ -0,0 +1,57 @@
+/** Provides classes to be used in queries related to JWT vulnerabilities. */
+
+import java
+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.
+ */
+private class SigningToInsecureMethodAccessDataFlow extends DataFlow::Configuration {
+  SigningToInsecureMethodAccessDataFlow() { this = "SigningToExprDataFlow" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof JwtParserWithInsecureParseSource
+  }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof JwtParserWithInsecureParseSink }
+
+  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    any(JwtParserWithInsecureParseAdditionalTaintStep c).step(node1, node2)
+  }
+}
+
+

java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql

@@ -1,5 +1,5 @@
 import java
-import semmle.code.java.security.JWT
+import semmle.code.java.security.JWTQuery
 import TestUtilities.InlineExpectationsTest
 
 class HasMissingJwtSignatureCheckTest extends InlineExpectationsTest {