mirror of
https://github.com/github/codeql.git
synced 2025-12-21 11:16:30 +01:00
Merge pull request #5911 from atorralba/atorralba/promote-missing-jwt-signature-check
Java: Promote Missing JWT signature check query from experimental
This commit is contained in:
Tony Torralba
@@ -25,9 +25,9 @@ by overriding the <code>onPlaintextJws</code> or <code>onClaimsJws</code> of <co
|
||||
<example>
|
||||
|
||||
<p>The following example shows four cases where a signing key is set for a parser.
|
||||
In the first bad case the <code>parse</code> method is used which will not validate the signature.
|
||||
The second bad case uses a <code>JwtHandlerAdapter</code> where the <code>onPlaintextJwt</code> method is overriden so it will not validate the signature.
|
||||
The third and fourth good cases use <code>parseClaimsJws</code> method or override the <code>onPlaintextJws</code> method.
|
||||
In the first 'BAD' case the <code>parse</code> method is used, which will not validate the signature.
|
||||
The second 'BAD' case uses a <code>JwtHandlerAdapter</code> where the <code>onPlaintextJwt</code> method is overriden, so it will not validate the signature.
|
||||
The third and fourth 'GOOD' cases use <code>parseClaimsJws</code> method or override the <code>onPlaintextJws</code> method.
|
||||
</p>
|
||||
|
||||
<sample src="MissingJWTSignatureCheck.java" />
|
||||
20
java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql
Normal file
20
java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql
Normal file
@@ -0,0 +1,20 @@
|
||||
/**
|
||||
* @name Missing JWT signature check
|
||||
* @description Failing to check the Json Web Token (JWT) signature may allow an attacker to forge their own tokens.
|
||||
* @kind path-problem
|
||||
* @problem.severity error
|
||||
* @security-severity 7.8
|
||||
* @precision high
|
||||
* @id java/missing-jwt-signature-check
|
||||
* @tags security
|
||||
* external/cwe/cwe-347
|
||||
*/
|
||||
|
||||
import java
|
||||
import semmle.code.java.security.MissingJWTSignatureCheckQuery
|
||||
import DataFlow::PathGraph
|
||||
|
||||
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"
|
||||
@@ -1,200 +0,0 @@
|
||||
/**
|
||||
* @name Missing JWT signature check
|
||||
* @description Not checking the JWT signature allows an attacker to forge their own tokens.
|
||||
* @kind problem
|
||||
* @problem.severity error
|
||||
* @precision high
|
||||
* @id java/missing-jwt-signature-check
|
||||
* @tags security
|
||||
* external/cwe/cwe-347
|
||||
*/
|
||||
|
||||
import java
|
||||
import semmle.code.java.dataflow.DataFlow
|
||||
|
||||
/** The interface `io.jsonwebtoken.JwtParser`. */
|
||||
class TypeJwtParser extends Interface {
|
||||
TypeJwtParser() { this.hasQualifiedName("io.jsonwebtoken", "JwtParser") }
|
||||
}
|
||||
|
||||
/** The interface `io.jsonwebtoken.JwtParser` or a type derived from it. */
|
||||
class TypeDerivedJwtParser extends RefType {
|
||||
TypeDerivedJwtParser() { this.getASourceSupertype*() instanceof TypeJwtParser }
|
||||
}
|
||||
|
||||
/** The interface `io.jsonwebtoken.JwtParserBuilder`. */
|
||||
class TypeJwtParserBuilder extends Interface {
|
||||
TypeJwtParserBuilder() { this.hasQualifiedName("io.jsonwebtoken", "JwtParserBuilder") }
|
||||
}
|
||||
|
||||
/** The interface `io.jsonwebtoken.JwtHandler`. */
|
||||
class TypeJwtHandler extends Interface {
|
||||
TypeJwtHandler() { this.hasQualifiedName("io.jsonwebtoken", "JwtHandler") }
|
||||
}
|
||||
|
||||
/** The class `io.jsonwebtoken.JwtHandlerAdapter`. */
|
||||
class TypeJwtHandlerAdapter extends Class {
|
||||
TypeJwtHandlerAdapter() { this.hasQualifiedName("io.jsonwebtoken", "JwtHandlerAdapter") }
|
||||
}
|
||||
|
||||
/** The `parse(token, handler)` method defined in `JwtParser`. */
|
||||
private class JwtParserParseHandlerMethod extends Method {
|
||||
JwtParserParseHandlerMethod() {
|
||||
this.hasName("parse") and
|
||||
this.getDeclaringType() instanceof TypeJwtParser and
|
||||
this.getNumberOfParameters() = 2
|
||||
}
|
||||
}
|
||||
|
||||
/** The `parse(token)`, `parseClaimsJwt(token)` and `parsePlaintextJwt(token)` methods defined in `JwtParser`. */
|
||||
private class JwtParserInsecureParseMethod extends Method {
|
||||
JwtParserInsecureParseMethod() {
|
||||
this.hasName(["parse", "parseClaimsJwt", "parsePlaintextJwt"]) and
|
||||
this.getNumberOfParameters() = 1 and
|
||||
this.getDeclaringType() instanceof TypeJwtParser
|
||||
}
|
||||
}
|
||||
|
||||
/** The `on(Claims|Plaintext)Jwt` methods defined in `JwtHandler`. */
|
||||
private class JwtHandlerOnJwtMethod extends Method {
|
||||
JwtHandlerOnJwtMethod() {
|
||||
this.hasName(["onClaimsJwt", "onPlaintextJwt"]) and
|
||||
this.getNumberOfParameters() = 1 and
|
||||
this.getDeclaringType() instanceof TypeJwtHandler
|
||||
}
|
||||
}
|
||||
|
||||
/** The `on(Claims|Plaintext)Jwt` methods defined in `JwtHandlerAdapter`. */
|
||||
private class JwtHandlerAdapterOnJwtMethod extends Method {
|
||||
JwtHandlerAdapterOnJwtMethod() {
|
||||
this.hasName(["onClaimsJwt", "onPlaintextJwt"]) and
|
||||
this.getNumberOfParameters() = 1 and
|
||||
this.getDeclaringType() instanceof TypeJwtHandlerAdapter
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `parseHandlerExpr` is an insecure `JwtHandler`.
|
||||
* 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) {
|
||||
exists(RefType t |
|
||||
parseHandlerExpr.getType() = t and
|
||||
t.getASourceSupertype*() instanceof TypeJwtHandler and
|
||||
exists(Method m |
|
||||
m = t.getAMethod() and
|
||||
m.getASourceOverriddenMethod+() instanceof JwtHandlerOnJwtMethod and
|
||||
not m.getSourceDeclaration() instanceof JwtHandlerAdapterOnJwtMethod
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* An access to an insecure parsing method.
|
||||
* That is, either a call to a `parse(token)`, `parseClaimsJwt(token)` or `parsePlaintextJwt(token)` method or
|
||||
* a call to a `parse(token, handler)` method where the `handler` is considered insecure.
|
||||
*/
|
||||
private class JwtParserInsecureParseMethodAccess extends MethodAccess {
|
||||
JwtParserInsecureParseMethodAccess() {
|
||||
this.getMethod().getASourceOverriddenMethod*() instanceof JwtParserInsecureParseMethod
|
||||
or
|
||||
this.getMethod().getASourceOverriddenMethod*() instanceof JwtParserParseHandlerMethod and
|
||||
isInsecureJwtHandler(this.getArgument(1))
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* 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:
|
||||
* ```java
|
||||
* Jwts.parser().setSigningKey(key).parse(token);
|
||||
* ```
|
||||
* Here the signing key is set directly on a `JwtParser`.
|
||||
* Indirectly means code like this:
|
||||
* ```java
|
||||
* Jwts.parserBuilder().setSigningKey(key).build().parse(token);
|
||||
* ```
|
||||
* In this case, the signing key is set on a `JwtParserBuilder` indirectly setting the key of `JwtParser` that is created by the call to `build`.
|
||||
*/
|
||||
private predicate isSigningKeySetter(Expr expr, MethodAccess signingMa) {
|
||||
any(SigningToInsecureMethodAccessDataFlow s)
|
||||
.hasFlow(DataFlow::exprNode(signingMa), DataFlow::exprNode(expr))
|
||||
}
|
||||
|
||||
/**
|
||||
* An expr that is a (sub-type of) `JwtParser` for which a signing key has been set and which is used as
|
||||
* the qualifier to a `JwtParserInsecureParseMethodAccess`.
|
||||
*/
|
||||
private class JwtParserWithSigningKeyExpr extends Expr {
|
||||
MethodAccess signingMa;
|
||||
|
||||
JwtParserWithSigningKeyExpr() {
|
||||
this.getType() instanceof TypeDerivedJwtParser and
|
||||
isSigningKeySetter(this, signingMa)
|
||||
}
|
||||
|
||||
/** Gets the method access that sets the signing key for this parser. */
|
||||
MethodAccess getSigningMethodAccess() { result = signingMa }
|
||||
}
|
||||
|
||||
/**
|
||||
* Models flow from `SigningKeyMethodAccess`es to qualifiers of `JwtParserInsecureParseMethodAccess`es.
|
||||
* 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.asExpr() instanceof SigningKeyMethodAccess
|
||||
}
|
||||
|
||||
override predicate isSink(DataFlow::Node sink) {
|
||||
any(JwtParserInsecureParseMethodAccess ma).getQualifier() = sink.asExpr()
|
||||
}
|
||||
|
||||
/** Models the builder style of `JwtParser` and `JwtParserBuilder`. */
|
||||
override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
|
||||
(
|
||||
pred.asExpr().getType() instanceof TypeDerivedJwtParser or
|
||||
pred.asExpr().getType().(RefType).getASourceSupertype*() instanceof TypeJwtParserBuilder
|
||||
) and
|
||||
succ.asExpr().(MethodAccess).getQualifier() = pred.asExpr()
|
||||
}
|
||||
}
|
||||
|
||||
/** An access to the `setSigningKey` or `setSigningKeyResolver` method (or an overridden method) defined in `JwtParser` and `JwtParserBuilder`. */
|
||||
private class SigningKeyMethodAccess extends MethodAccess {
|
||||
SigningKeyMethodAccess() {
|
||||
exists(Method m |
|
||||
m.hasName(["setSigningKey", "setSigningKeyResolver"]) and
|
||||
m.getNumberOfParameters() = 1 and
|
||||
(
|
||||
m.getDeclaringType() instanceof TypeJwtParser or
|
||||
m.getDeclaringType() instanceof TypeJwtParserBuilder
|
||||
)
|
||||
|
|
||||
m = this.getMethod().getASourceOverriddenMethod*()
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if the `MethodAccess` `ma` occurs in a test file. A test file is any file that
|
||||
* is a direct or indirect child of a directory named `test`, ignoring case.
|
||||
*/
|
||||
private predicate isInTestFile(MethodAccess ma) {
|
||||
exists(string lowerCasedAbsolutePath |
|
||||
lowerCasedAbsolutePath = ma.getLocation().getFile().getAbsolutePath().toLowerCase()
|
||||
|
|
||||
lowerCasedAbsolutePath.matches("%/test/%") and
|
||||
not lowerCasedAbsolutePath
|
||||
.matches("%/ql/test/experimental/query-tests/security/CWE-347/%".toLowerCase())
|
||||
)
|
||||
}
|
||||
|
||||
from JwtParserInsecureParseMethodAccess ma, JwtParserWithSigningKeyExpr parserExpr
|
||||
where ma.getQualifier() = parserExpr and not isInTestFile(ma)
|
||||
select ma, "A signing key is set $@, but the signature is not verified.",
|
||||
parserExpr.getSigningMethodAccess(), "here"
|
||||
127
java/ql/src/semmle/code/java/security/JWT.qll
Normal file
127
java/ql/src/semmle/code/java/security/JWT.qll
Normal file
@@ -0,0 +1,127 @@
|
||||
/** Provides classes for working with JSON Web Token (JWT) libraries. */
|
||||
|
||||
import java
|
||||
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 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
|
||||
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 }
|
||||
}
|
||||
|
||||
/**
|
||||
* A unit class for adding additional flow steps.
|
||||
*
|
||||
* Extend this class to add additional flow steps that should apply to the `MissingJwtSignatureCheckConf`.
|
||||
*/
|
||||
class JwtParserWithInsecureParseAdditionalFlowStep extends Unit {
|
||||
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
|
||||
}
|
||||
|
||||
/** A set of additional flow steps to consider when working with JWT parsing related data flows. */
|
||||
private class DefaultJwtParserWithInsecureParseAdditionalFlowStep extends JwtParserWithInsecureParseAdditionalFlowStep {
|
||||
override 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`.
|
||||
* `JwtHandlerAdapter`'s overrides are safe since they always throw an exception.
|
||||
*/
|
||||
private predicate isInsecureJwtHandler(Expr parseHandlerExpr) {
|
||||
exists(RefType t |
|
||||
parseHandlerExpr.getType() = t and
|
||||
t.getASourceSupertype*() instanceof TypeJwtHandler and
|
||||
exists(Method m |
|
||||
m = t.getAMethod() and
|
||||
m.getASourceOverriddenMethod+() instanceof JwtHandlerOnJwtMethod and
|
||||
not m.getSourceDeclaration() instanceof JwtHandlerAdapterOnJwtMethod
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
/** The interface `io.jsonwebtoken.JwtParser`. */
|
||||
private class TypeJwtParser extends Interface {
|
||||
TypeJwtParser() { this.hasQualifiedName("io.jsonwebtoken", "JwtParser") }
|
||||
}
|
||||
|
||||
/** The interface `io.jsonwebtoken.JwtParserBuilder`. */
|
||||
private class TypeJwtParserBuilder extends Interface {
|
||||
TypeJwtParserBuilder() { this.hasQualifiedName("io.jsonwebtoken", "JwtParserBuilder") }
|
||||
}
|
||||
|
||||
/** The interface `io.jsonwebtoken.JwtHandler`. */
|
||||
private class TypeJwtHandler extends Interface {
|
||||
TypeJwtHandler() { this.hasQualifiedName("io.jsonwebtoken", "JwtHandler") }
|
||||
}
|
||||
|
||||
/** The class `io.jsonwebtoken.JwtHandlerAdapter`. */
|
||||
private class TypeJwtHandlerAdapter extends Class {
|
||||
TypeJwtHandlerAdapter() { this.hasQualifiedName("io.jsonwebtoken", "JwtHandlerAdapter") }
|
||||
}
|
||||
|
||||
/** The `on(Claims|Plaintext)Jwt` methods defined in `JwtHandler`. */
|
||||
private class JwtHandlerOnJwtMethod extends Method {
|
||||
JwtHandlerOnJwtMethod() {
|
||||
this.hasName(["onClaimsJwt", "onPlaintextJwt"]) and
|
||||
this.getNumberOfParameters() = 1 and
|
||||
this.getDeclaringType() instanceof TypeJwtHandler
|
||||
}
|
||||
}
|
||||
|
||||
/** The `on(Claims|Plaintext)Jwt` methods defined in `JwtHandlerAdapter`. */
|
||||
private class JwtHandlerAdapterOnJwtMethod extends Method {
|
||||
JwtHandlerAdapterOnJwtMethod() {
|
||||
this.hasName(["onClaimsJwt", "onPlaintextJwt"]) and
|
||||
this.getNumberOfParameters() = 1 and
|
||||
this.getDeclaringType() instanceof TypeJwtHandlerAdapter
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,23 @@
|
||||
/** Provides classes to be used in queries related to JSON Web Token (JWT) signature vulnerabilities. */
|
||||
|
||||
import java
|
||||
import semmle.code.java.dataflow.DataFlow
|
||||
import semmle.code.java.security.JWT
|
||||
|
||||
/**
|
||||
* Models flow from signing keys assignments to qualifiers of JWT insecure parsers.
|
||||
* This is used to determine whether a `JwtParser` performing unsafe parsing has a signing key set.
|
||||
*/
|
||||
class MissingJwtSignatureCheckConf extends DataFlow::Configuration {
|
||||
MissingJwtSignatureCheckConf() { 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(JwtParserWithInsecureParseAdditionalFlowStep c).step(node1, node2)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user