Skip to content

Commit 0356ed7

Browse files
authored
Merge pull request github#5911 from atorralba/atorralba/promote-missing-jwt-signature-check
Java: Promote Missing JWT signature check query from experimental
2 parents 1932f60 + 5f9f857 commit 0356ed7

35 files changed

+288
-278
lines changed
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The query "Missing JWT signature check" (`java/missing-jwt-signature-check`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @intrigus-lgtm](https://github.com/github/codeql/pull/5597).

java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp renamed to java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,9 @@ by overriding the <code>onPlaintextJws</code> or <code>onClaimsJws</code> of <co
2525
<example>
2626

2727
<p>The following example shows four cases where a signing key is set for a parser.
28-
In the first bad case the <code>parse</code> method is used which will not validate the signature.
29-
The second bad case uses a <code>JwtHandlerAdapter</code> where the <code>onPlaintextJwt</code> method is overriden so it will not validate the signature.
30-
The third and fourth good cases use <code>parseClaimsJws</code> method or override the <code>onPlaintextJws</code> method.
28+
In the first 'BAD' case the <code>parse</code> method is used, which will not validate the signature.
29+
The second 'BAD' case uses a <code>JwtHandlerAdapter</code> where the <code>onPlaintextJwt</code> method is overriden, so it will not validate the signature.
30+
The third and fourth 'GOOD' cases use <code>parseClaimsJws</code> method or override the <code>onPlaintextJws</code> method.
3131
</p>
3232

3333
<sample src="MissingJWTSignatureCheck.java" />
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* @name Missing JWT signature check
3+
* @description Failing to check the Json Web Token (JWT) signature may allow an attacker to forge their own tokens.
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @security-severity 7.8
7+
* @precision high
8+
* @id java/missing-jwt-signature-check
9+
* @tags security
10+
* external/cwe/cwe-347
11+
*/
12+
13+
import java
14+
import semmle.code.java.security.MissingJWTSignatureCheckQuery
15+
import DataFlow::PathGraph
16+
17+
from DataFlow::PathNode source, DataFlow::PathNode sink, MissingJwtSignatureCheckConf conf
18+
where conf.hasFlowPath(source, sink)
19+
select sink.getNode(), source, sink, "A signing key is set $@, but the signature is not verified.",
20+
source.getNode(), "here"

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

Lines changed: 0 additions & 200 deletions
This file was deleted.
Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
/** Provides classes for working with JSON Web Token (JWT) libraries. */
2+
3+
import java
4+
private import semmle.code.java.dataflow.DataFlow
5+
6+
/** A method access that assigns signing keys to a JWT parser. */
7+
class JwtParserWithInsecureParseSource extends DataFlow::Node {
8+
JwtParserWithInsecureParseSource() {
9+
exists(MethodAccess ma, Method m |
10+
m.getDeclaringType().getASupertype*() instanceof TypeJwtParser or
11+
m.getDeclaringType().getASupertype*() instanceof TypeJwtParserBuilder
12+
|
13+
this.asExpr() = ma and
14+
ma.getMethod() = m and
15+
m.hasName(["setSigningKey", "setSigningKeyResolver"])
16+
)
17+
}
18+
}
19+
20+
/**
21+
* The qualifier of an insecure parsing method.
22+
* That is, either the qualifier of a call to the `parse(token)`,
23+
* `parseClaimsJwt(token)` or `parsePlaintextJwt(token)` methods or
24+
* the qualifier of a call to a `parse(token, handler)` method
25+
* where the `handler` is considered insecure.
26+
*/
27+
class JwtParserWithInsecureParseSink extends DataFlow::Node {
28+
MethodAccess insecureParseMa;
29+
30+
JwtParserWithInsecureParseSink() {
31+
insecureParseMa.getQualifier() = this.asExpr() and
32+
exists(Method m |
33+
insecureParseMa.getMethod() = m and
34+
m.getDeclaringType().getASupertype*() instanceof TypeJwtParser and
35+
m.hasName(["parse", "parseClaimsJwt", "parsePlaintextJwt"]) and
36+
(
37+
m.getNumberOfParameters() = 1
38+
or
39+
isInsecureJwtHandler(insecureParseMa.getArgument(1))
40+
)
41+
)
42+
}
43+
44+
/** Gets the method access that does the insecure parsing. */
45+
MethodAccess getParseMethodAccess() { result = insecureParseMa }
46+
}
47+
48+
/**
49+
* A unit class for adding additional flow steps.
50+
*
51+
* Extend this class to add additional flow steps that should apply to the `MissingJwtSignatureCheckConf`.
52+
*/
53+
class JwtParserWithInsecureParseAdditionalFlowStep extends Unit {
54+
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
55+
}
56+
57+
/** A set of additional flow steps to consider when working with JWT parsing related data flows. */
58+
private class DefaultJwtParserWithInsecureParseAdditionalFlowStep extends JwtParserWithInsecureParseAdditionalFlowStep {
59+
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
60+
jwtParserStep(node1.asExpr(), node2.asExpr())
61+
}
62+
}
63+
64+
/** Models the builder style of `JwtParser` and `JwtParserBuilder`. */
65+
private predicate jwtParserStep(Expr parser, MethodAccess ma) {
66+
(
67+
parser.getType().(RefType).getASourceSupertype*() instanceof TypeJwtParser or
68+
parser.getType().(RefType).getASourceSupertype*() instanceof TypeJwtParserBuilder
69+
) and
70+
ma.getQualifier() = parser
71+
}
72+
73+
/**
74+
* Holds if `parseHandlerExpr` is an insecure `JwtHandler`.
75+
* That is, it overrides a method from `JwtHandlerOnJwtMethod` and
76+
* the override is not defined on `JwtHandlerAdapter`.
77+
* `JwtHandlerAdapter`'s overrides are safe since they always throw an exception.
78+
*/
79+
private predicate isInsecureJwtHandler(Expr parseHandlerExpr) {
80+
exists(RefType t |
81+
parseHandlerExpr.getType() = t and
82+
t.getASourceSupertype*() instanceof TypeJwtHandler and
83+
exists(Method m |
84+
m = t.getAMethod() and
85+
m.getASourceOverriddenMethod+() instanceof JwtHandlerOnJwtMethod and
86+
not m.getSourceDeclaration() instanceof JwtHandlerAdapterOnJwtMethod
87+
)
88+
)
89+
}
90+
91+
/** The interface `io.jsonwebtoken.JwtParser`. */
92+
private class TypeJwtParser extends Interface {
93+
TypeJwtParser() { this.hasQualifiedName("io.jsonwebtoken", "JwtParser") }
94+
}
95+
96+
/** The interface `io.jsonwebtoken.JwtParserBuilder`. */
97+
private class TypeJwtParserBuilder extends Interface {
98+
TypeJwtParserBuilder() { this.hasQualifiedName("io.jsonwebtoken", "JwtParserBuilder") }
99+
}
100+
101+
/** The interface `io.jsonwebtoken.JwtHandler`. */
102+
private class TypeJwtHandler extends Interface {
103+
TypeJwtHandler() { this.hasQualifiedName("io.jsonwebtoken", "JwtHandler") }
104+
}
105+
106+
/** The class `io.jsonwebtoken.JwtHandlerAdapter`. */
107+
private class TypeJwtHandlerAdapter extends Class {
108+
TypeJwtHandlerAdapter() { this.hasQualifiedName("io.jsonwebtoken", "JwtHandlerAdapter") }
109+
}
110+
111+
/** The `on(Claims|Plaintext)Jwt` methods defined in `JwtHandler`. */
112+
private class JwtHandlerOnJwtMethod extends Method {
113+
JwtHandlerOnJwtMethod() {
114+
this.hasName(["onClaimsJwt", "onPlaintextJwt"]) and
115+
this.getNumberOfParameters() = 1 and
116+
this.getDeclaringType() instanceof TypeJwtHandler
117+
}
118+
}
119+
120+
/** The `on(Claims|Plaintext)Jwt` methods defined in `JwtHandlerAdapter`. */
121+
private class JwtHandlerAdapterOnJwtMethod extends Method {
122+
JwtHandlerAdapterOnJwtMethod() {
123+
this.hasName(["onClaimsJwt", "onPlaintextJwt"]) and
124+
this.getNumberOfParameters() = 1 and
125+
this.getDeclaringType() instanceof TypeJwtHandlerAdapter
126+
}
127+
}
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/** Provides classes to be used in queries related to JSON Web Token (JWT) signature vulnerabilities. */
2+
3+
import java
4+
import semmle.code.java.dataflow.DataFlow
5+
import semmle.code.java.security.JWT
6+
7+
/**
8+
* Models flow from signing keys assignments to qualifiers of JWT insecure parsers.
9+
* This is used to determine whether a `JwtParser` performing unsafe parsing has a signing key set.
10+
*/
11+
class MissingJwtSignatureCheckConf extends DataFlow::Configuration {
12+
MissingJwtSignatureCheckConf() { this = "SigningToExprDataFlow" }
13+
14+
override predicate isSource(DataFlow::Node source) {
15+
source instanceof JwtParserWithInsecureParseSource
16+
}
17+
18+
override predicate isSink(DataFlow::Node sink) { sink instanceof JwtParserWithInsecureParseSink }
19+
20+
override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
21+
any(JwtParserWithInsecureParseAdditionalFlowStep c).step(node1, node2)
22+
}
23+
}

java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.expected

Lines changed: 0 additions & 8 deletions
This file was deleted.

0 commit comments

Comments
 (0)