Skip to content

Fix/jwt source #16141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix/jwt source #16141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14aaf9d
Add test for jwt non-builder usage.
intrigus-lgtm Apr 5, 2024
d3ac94d
fix: use qualifier as source instead of access
intrigus-lgtm Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions java/ql/lib/semmle/code/java/security/JWT.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@
import java
private import semmle.code.java.dataflow.DataFlow

/** A method access that assigns signing keys to a JWT parser. */
/** The qualifier of a method access that assigns signing keys to a JWT parser. */
class JwtParserWithInsecureParseSource extends DataFlow::Node {
JwtParserWithInsecureParseSource() {
exists(MethodCall ma, Method m |
m.getDeclaringType().getAnAncestor() instanceof TypeJwtParser or
m.getDeclaringType().getAnAncestor() instanceof TypeJwtParserBuilder
|
this.asExpr() = ma and
this.asExpr() = ma.getQualifier() and
Copy link
Contributor

@atorralba atorralba Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep the return value as well, since the call to setSigningKey returns the parser/builder:

Suggested change
this.asExpr() = ma.getQualifier() and
this.asExpr() = [ma, ma.getQualifier()] and

ma.getMethod() = m and
m.hasName(["setSigningKey", "setSigningKeyResolver"])
)
Expand Down
243 changes: 199 additions & 44 deletions java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import io.jsonwebtoken.Jwt;
import io.jsonwebtoken.JwtHandlerAdapter;
import io.jsonwebtoken.JwtParser;
import io.jsonwebtoken.JwtParserBuilder;
import io.jsonwebtoken.Jwts;
import io.jsonwebtoken.impl.DefaultJwtParser;
import io.jsonwebtoken.impl.DefaultJwtParserBuilder;
Expand All @@ -13,99 +14,253 @@ private JwtParser getASignedParser() {
return Jwts.parser().setSigningKey("someBase64EncodedKey");
}

private JwtParser getASignedParserNonBuilderStyle() {
JwtParser parser = Jwts.parser();
parser.setSigningKey("someBase64EncodedKey");
return parser;
}

private JwtParser getASignedParserFromParserBuilder() {
return Jwts.parserBuilder().setSigningKey("someBase64EncodedKey").build();
}

private JwtParser getASignedParserFromParserBuilderNonBuilderStyle() {
JwtParserBuilder parserBuilder = Jwts.parserBuilder();
parserBuilder.setSigningKey("someBase64EncodedKey");
JwtParser parser = parserBuilder.build();
return parser;
}

private JwtParser getASignedNewParser() {
return new DefaultJwtParser().setSigningKey("someBase64EncodedKey");
}

private JwtParser getASignedNewParserNonBuilderStyle() {
JwtParser parser = new DefaultJwtParser();
parser.setSigningKey("someBase64EncodedKey");
return parser;
}

private void callSignedParsers() {
JwtParser parser1 = getASignedParser();
badJwtOnParserBuilder(parser1, "");
badJwtHandlerOnParserBuilder(parser1, "");
goodJwtOnParserBuilder(parser1, "");
goodJwtHandler(parser1, "");
parser1.parse(""); // $hasMissingJwtSignatureCheck
parser1.parse("", new JwtHandlerAdapter<Jwt<Header, String>>() { // $hasMissingJwtSignatureCheck
@Override
public Jwt<Header, String> onPlaintextJwt(Jwt<Header, String> jwt) {
return jwt;
}
});
parser1.parseClaimsJws("") // Safe
.getBody();
parser1.parse("", new JwtHandlerAdapter<Jws<String>>() { // Safe
@Override
public Jws<String> onPlaintextJws(Jws<String> jws) {
return jws;
}
});

JwtParser parser2 = getASignedParserFromParserBuilder();
badJwtOnParserBuilder(parser2, "");
badJwtHandlerOnParserBuilder(parser2, "");
goodJwtOnParserBuilder(parser2, "");
goodJwtHandler(parser2, "");
parser2.parse(""); // $hasMissingJwtSignatureCheck
parser2.parse("", new JwtHandlerAdapter<Jwt<Header, String>>() { // $hasMissingJwtSignatureCheck
@Override
public Jwt<Header, String> onPlaintextJwt(Jwt<Header, String> jwt) {
return jwt;
}
});
parser2.parseClaimsJws("") // Safe
.getBody();
parser2.parse("", new JwtHandlerAdapter<Jws<String>>() { // Safe
@Override
public Jws<String> onPlaintextJws(Jws<String> jws) {
return jws;
}
});

JwtParser parser3 = getASignedNewParser();
badJwtOnParserBuilder(parser3, "");
badJwtHandlerOnParserBuilder(parser3, "");
goodJwtOnParserBuilder(parser3, "");
goodJwtHandler(parser3, "");
parser3.parse(""); // $hasMissingJwtSignatureCheck
parser3.parse("", new JwtHandlerAdapter<Jwt<Header, String>>() { // $hasMissingJwtSignatureCheck
@Override
public Jwt<Header, String> onPlaintextJwt(Jwt<Header, String> jwt) {
return jwt;
}
});
parser3.parseClaimsJws("") // Safe
.getBody();
parser3.parse("", new JwtHandlerAdapter<Jws<String>>() { // Safe
@Override
public Jws<String> onPlaintextJws(Jws<String> jws) {
return jws;
}
});

JwtParser parser4 = getASignedParserNonBuilderStyle();
parser4.parse(""); // $hasMissingJwtSignatureCheck
parser4.parse("", new JwtHandlerAdapter<Jwt<Header, String>>() { // $hasMissingJwtSignatureCheck
@Override
public Jwt<Header, String> onPlaintextJwt(Jwt<Header, String> jwt) {
return jwt;
}
});
parser4.parseClaimsJws("") // Safe
.getBody();
parser4.parse("", new JwtHandlerAdapter<Jws<String>>() { // Safe
@Override
public Jws<String> onPlaintextJws(Jws<String> jws) {
return jws;
}
});

JwtParser parser5 = getASignedParserFromParserBuilderNonBuilderStyle();
parser5.parse(""); // $hasMissingJwtSignatureCheck
parser5.parse("", new JwtHandlerAdapter<Jwt<Header, String>>() { // $hasMissingJwtSignatureCheck
@Override
public Jwt<Header, String> onPlaintextJwt(Jwt<Header, String> jwt) {
return jwt;
}
});
parser5.parseClaimsJws("") // Safe
.getBody();
parser5.parse("", new JwtHandlerAdapter<Jws<String>>() { // Safe
@Override
public Jws<String> onPlaintextJws(Jws<String> jws) {
return jws;
}
});

JwtParser parser6 = getASignedNewParserNonBuilderStyle();
parser6.parse(""); // $hasMissingJwtSignatureCheck
parser6.parse("", new JwtHandlerAdapter<Jwt<Header, String>>() { // $hasMissingJwtSignatureCheck
@Override
public Jwt<Header, String> onPlaintextJwt(Jwt<Header, String> jwt) {
return jwt;
}
});
parser6.parseClaimsJws("") // Safe
.getBody();
parser6.parse("", new JwtHandlerAdapter<Jws<String>>() { // Safe
@Override
public Jws<String> onPlaintextJws(Jws<String> jws) {
return jws;
}
});
}

private JwtParser getAnUnsignedParser() {
return Jwts.parser();
}

private JwtParser getAnUnsignedParserNonBuilderStyle() {
JwtParser parser = Jwts.parser();
return parser;
}

private JwtParser getAnUnsignedParserFromParserBuilder() {
return Jwts.parserBuilder().build();
}

private JwtParser getAnUnsignedParserFromParserBuilderNonBuilderStyle() {
JwtParserBuilder parserBuilder = Jwts.parserBuilder();
JwtParser parser = parserBuilder.build();
return parser;
}

private JwtParser getAnUnsignedNewParser() {
return new DefaultJwtParser();
}

private void callUnsignedParsers() {
JwtParser parser1 = getAnUnsignedParser();
badJwtOnParserBuilder(parser1, "");
badJwtHandlerOnParserBuilder(parser1, "");
goodJwtOnParserBuilder(parser1, "");
goodJwtHandler(parser1, "");
parser1.parse(""); // Ignored, parser has no signing key
parser1.parse("", new JwtHandlerAdapter<Jwt<Header, String>>() { // Ignored, parser has no signing key
@Override
public Jwt<Header, String> onPlaintextJwt(Jwt<Header, String> jwt) {
return jwt;
}
});
parser1.parseClaimsJws("") // Safe
.getBody();
parser1.parse("", new JwtHandlerAdapter<Jws<String>>() { // Safe
@Override
public Jws<String> onPlaintextJws(Jws<String> jws) {
return jws;
}
});

JwtParser parser2 = getAnUnsignedParserFromParserBuilder();
badJwtOnParserBuilder(parser2, "");
badJwtHandlerOnParserBuilder(parser2, "");
goodJwtOnParserBuilder(parser2, "");
goodJwtHandler(parser2, "");
parser2.parse(""); // Ignored, parser has no signing key
parser2.parse("", new JwtHandlerAdapter<Jwt<Header, String>>() { // Ignored, parser has no signing key
@Override
public Jwt<Header, String> onPlaintextJwt(Jwt<Header, String> jwt) {
return jwt;
}
});
parser2.parseClaimsJws("") // Safe
.getBody();
parser2.parse("", new JwtHandlerAdapter<Jws<String>>() { // Safe
@Override
public Jws<String> onPlaintextJws(Jws<String> jws) {
return jws;
}
});

JwtParser parser3 = getAnUnsignedNewParser();
badJwtOnParserBuilder(parser3, "");
badJwtHandlerOnParserBuilder(parser3, "");
goodJwtOnParserBuilder(parser3, "");
goodJwtHandler(parser3, "");
}

private void signParserAfterParseCall() {
JwtParser parser = getAnUnsignedParser();
parser.parse(""); // Safe
parser.setSigningKey("someBase64EncodedKey");
}

private void badJwtOnParserBuilder(JwtParser parser, String token) {
parser.parse(token); // $hasMissingJwtSignatureCheck
}

private void badJwtHandlerOnParserBuilder(JwtParser parser, String token) {
parser.parse(token, new JwtHandlerAdapter<Jwt<Header, String>>() { // $hasMissingJwtSignatureCheck
parser3.parse(""); // Ignored, parser has no signing key
parser3.parse("", new JwtHandlerAdapter<Jwt<Header, String>>() { // Ignored, parser has no signing key
@Override
public Jwt<Header, String> onPlaintextJwt(Jwt<Header, String> jwt) {
return jwt;
}
});
}
parser3.parseClaimsJws("") // Safe
.getBody();
parser3.parse("", new JwtHandlerAdapter<Jws<String>>() { // Safe
@Override
public Jws<String> onPlaintextJws(Jws<String> jws) {
return jws;
}
});

private void goodJwtOnParserBuilder(JwtParser parser, String token) {
parser.parseClaimsJws(token) // Safe
JwtParser parser4 = getAnUnsignedParserNonBuilderStyle();
parser4.parse(""); // Ignored, parser has no signing key
parser4.parse("", new JwtHandlerAdapter<Jwt<Header, String>>() { // Ignored, parser has no signing key
@Override
public Jwt<Header, String> onPlaintextJwt(Jwt<Header, String> jwt) {
return jwt;
}
});
parser4.parseClaimsJws("") // Safe
.getBody();
}
parser4.parse("", new JwtHandlerAdapter<Jws<String>>() { // Safe
@Override
public Jws<String> onPlaintextJws(Jws<String> jws) {
return jws;
}
});

private void goodJwtHandler(JwtParser parser, String token) {
parser.parse(token, new JwtHandlerAdapter<Jws<String>>() { // Safe
JwtParser parser5 = getAnUnsignedParserFromParserBuilderNonBuilderStyle();
parser5.parse(""); // Ignored, parser has no signing key
parser5.parse("", new JwtHandlerAdapter<Jwt<Header, String>>() { // Ignored, parser has no signing key
@Override
public Jwt<Header, String> onPlaintextJwt(Jwt<Header, String> jwt) {
return jwt;
}
});
parser5.parseClaimsJws("") // Safe
.getBody();
parser5.parse("", new JwtHandlerAdapter<Jws<String>>() { // Safe
@Override
public Jws<String> onPlaintextJws(Jws<String> jws) {
return jws;
}
});
}

private void signParserAfterParseCall() {
JwtParser parser = getAnUnsignedParser();
parser.parse(""); // Safe
parser.setSigningKey("someBase64EncodedKey");
}

private void badJwtOnParserBuilder(String token) {
Jwts.parserBuilder().setSigningKey("someBase64EncodedKey").build().parse(token); // $hasMissingJwtSignatureCheck
}
Expand Down