From 14aaf9d392500be22446638272f68ff689a2e231 Mon Sep 17 00:00:00 2001 From: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> Date: Fri, 5 Apr 2024 19:59:22 +0000 Subject: [PATCH 1/2] Add test for jwt non-builder usage. --- .../CWE-347/MissingJWTSignatureCheckTest.java | 243 ++++++++++++++---- 1 file changed, 199 insertions(+), 44 deletions(-) diff --git a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.java b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.java index 93b54154ffc1..2241119ac301 100644 --- a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.java +++ b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.java @@ -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; @@ -13,92 +14,240 @@ 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>() { // $hasMissingJwtSignatureCheck + @Override + public Jwt onPlaintextJwt(Jwt jwt) { + return jwt; + } + }); + parser1.parseClaimsJws("") // Safe + .getBody(); + parser1.parse("", new JwtHandlerAdapter>() { // Safe + @Override + public Jws onPlaintextJws(Jws jws) { + return jws; + } + }); JwtParser parser2 = getASignedParserFromParserBuilder(); - badJwtOnParserBuilder(parser2, ""); - badJwtHandlerOnParserBuilder(parser2, ""); - goodJwtOnParserBuilder(parser2, ""); - goodJwtHandler(parser2, ""); + parser2.parse(""); // $hasMissingJwtSignatureCheck + parser2.parse("", new JwtHandlerAdapter>() { // $hasMissingJwtSignatureCheck + @Override + public Jwt onPlaintextJwt(Jwt jwt) { + return jwt; + } + }); + parser2.parseClaimsJws("") // Safe + .getBody(); + parser2.parse("", new JwtHandlerAdapter>() { // Safe + @Override + public Jws onPlaintextJws(Jws jws) { + return jws; + } + }); JwtParser parser3 = getASignedNewParser(); - badJwtOnParserBuilder(parser3, ""); - badJwtHandlerOnParserBuilder(parser3, ""); - goodJwtOnParserBuilder(parser3, ""); - goodJwtHandler(parser3, ""); + parser3.parse(""); // $hasMissingJwtSignatureCheck + parser3.parse("", new JwtHandlerAdapter>() { // $hasMissingJwtSignatureCheck + @Override + public Jwt onPlaintextJwt(Jwt jwt) { + return jwt; + } + }); + parser3.parseClaimsJws("") // Safe + .getBody(); + parser3.parse("", new JwtHandlerAdapter>() { // Safe + @Override + public Jws onPlaintextJws(Jws jws) { + return jws; + } + }); + + JwtParser parser4 = getASignedParserNonBuilderStyle(); + parser4.parse(""); // $hasMissingJwtSignatureCheck + parser4.parse("", new JwtHandlerAdapter>() { // $hasMissingJwtSignatureCheck + @Override + public Jwt onPlaintextJwt(Jwt jwt) { + return jwt; + } + }); + parser4.parseClaimsJws("") // Safe + .getBody(); + parser4.parse("", new JwtHandlerAdapter>() { // Safe + @Override + public Jws onPlaintextJws(Jws jws) { + return jws; + } + }); + + JwtParser parser5 = getASignedParserFromParserBuilderNonBuilderStyle(); + parser5.parse(""); // $hasMissingJwtSignatureCheck + parser5.parse("", new JwtHandlerAdapter>() { // $hasMissingJwtSignatureCheck + @Override + public Jwt onPlaintextJwt(Jwt jwt) { + return jwt; + } + }); + parser5.parseClaimsJws("") // Safe + .getBody(); + parser5.parse("", new JwtHandlerAdapter>() { // Safe + @Override + public Jws onPlaintextJws(Jws jws) { + return jws; + } + }); + + JwtParser parser6 = getASignedNewParserNonBuilderStyle(); + parser6.parse(""); // $hasMissingJwtSignatureCheck + parser6.parse("", new JwtHandlerAdapter>() { // $hasMissingJwtSignatureCheck + @Override + public Jwt onPlaintextJwt(Jwt jwt) { + return jwt; + } + }); + parser6.parseClaimsJws("") // Safe + .getBody(); + parser6.parse("", new JwtHandlerAdapter>() { // Safe + @Override + public Jws onPlaintextJws(Jws 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>() { // Ignored, parser has no signing key + @Override + public Jwt onPlaintextJwt(Jwt jwt) { + return jwt; + } + }); + parser1.parseClaimsJws("") // Safe + .getBody(); + parser1.parse("", new JwtHandlerAdapter>() { // Safe + @Override + public Jws onPlaintextJws(Jws 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>() { // Ignored, parser has no signing key + @Override + public Jwt onPlaintextJwt(Jwt jwt) { + return jwt; + } + }); + parser2.parseClaimsJws("") // Safe + .getBody(); + parser2.parse("", new JwtHandlerAdapter>() { // Safe + @Override + public Jws onPlaintextJws(Jws 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>() { // $hasMissingJwtSignatureCheck + parser3.parse(""); // Ignored, parser has no signing key + parser3.parse("", new JwtHandlerAdapter>() { // Ignored, parser has no signing key @Override public Jwt onPlaintextJwt(Jwt jwt) { return jwt; } }); - } + parser3.parseClaimsJws("") // Safe + .getBody(); + parser3.parse("", new JwtHandlerAdapter>() { // Safe + @Override + public Jws onPlaintextJws(Jws 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>() { // Ignored, parser has no signing key + @Override + public Jwt onPlaintextJwt(Jwt jwt) { + return jwt; + } + }); + parser4.parseClaimsJws("") // Safe .getBody(); - } + parser4.parse("", new JwtHandlerAdapter>() { // Safe + @Override + public Jws onPlaintextJws(Jws jws) { + return jws; + } + }); - private void goodJwtHandler(JwtParser parser, String token) { - parser.parse(token, new JwtHandlerAdapter>() { // Safe + JwtParser parser5 = getAnUnsignedParserFromParserBuilderNonBuilderStyle(); + parser5.parse(""); // Ignored, parser has no signing key + parser5.parse("", new JwtHandlerAdapter>() { // Ignored, parser has no signing key + @Override + public Jwt onPlaintextJwt(Jwt jwt) { + return jwt; + } + }); + parser5.parseClaimsJws("") // Safe + .getBody(); + parser5.parse("", new JwtHandlerAdapter>() { // Safe @Override public Jws onPlaintextJws(Jws jws) { return jws; @@ -106,6 +255,12 @@ public Jws onPlaintextJws(Jws 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 } From d3ac94d58562cdd98855cf3087fc02583ef9bf5f Mon Sep 17 00:00:00 2001 From: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com> Date: Fri, 5 Apr 2024 20:57:38 +0000 Subject: [PATCH 2/2] fix: use qualifier as source instead of access Consider this code: ```java JwtParserBuilder parserBuilder = Jwts.parserBuilder(); parserBuilder.setSigningKey("someBase64EncodedKey"); JwtParser parser = parserBuilder.build(); return parser; ``` Previously `setSigningKey` (the method access) was considered the source. However the return value of that call does not flow anywhere, the method changes the qualifier instead. So this commit changes the source to the qualifier instead. --- java/ql/lib/semmle/code/java/security/JWT.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/JWT.qll b/java/ql/lib/semmle/code/java/security/JWT.qll index 183495d85652..627869ada6cd 100644 --- a/java/ql/lib/semmle/code/java/security/JWT.qll +++ b/java/ql/lib/semmle/code/java/security/JWT.qll @@ -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 ma.getMethod() = m and m.hasName(["setSigningKey", "setSigningKeyResolver"]) )