diff --git a/java/change-notes/2021-05-17-missing-jwt-signature-check-query.md b/java/change-notes/2021-05-17-missing-jwt-signature-check-query.md new file mode 100644 index 000000000000..46c0ca7f8b0e --- /dev/null +++ b/java/change-notes/2021-05-17-missing-jwt-signature-check-query.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* 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). \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.java b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.java similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.java rename to java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp similarity index 81% rename from java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp rename to java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp index 3b93936c21b8..f682db85618c 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp +++ b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp @@ -25,9 +25,9 @@ by overriding the onPlaintextJws or onClaimsJws of

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

diff --git a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql new file mode 100644 index 000000000000..30caee117c88 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql @@ -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" diff --git a/java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql b/java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql deleted file mode 100644 index 6d7462f1338e..000000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql +++ /dev/null @@ -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" diff --git a/java/ql/src/semmle/code/java/security/JWT.qll b/java/ql/src/semmle/code/java/security/JWT.qll new file mode 100644 index 000000000000..a7ed68ffddc0 --- /dev/null +++ b/java/ql/src/semmle/code/java/security/JWT.qll @@ -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 + } +} diff --git a/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll new file mode 100644 index 000000000000..818a4c664fe6 --- /dev/null +++ b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll @@ -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) + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.expected b/java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.expected deleted file mode 100644 index e93720fcea83..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.expected +++ /dev/null @@ -1,8 +0,0 @@ -| MissingJWTSignatureCheck.java:96:9:96:27 | parse(...) | A signing key is set $@, but the signature is not verified. | MissingJWTSignatureCheck.java:18:16:18:66 | setSigningKey(...) | here | -| MissingJWTSignatureCheck.java:96:9:96:27 | parse(...) | A signing key is set $@, but the signature is not verified. | MissingJWTSignatureCheck.java:22:16:22:73 | setSigningKey(...) | here | -| MissingJWTSignatureCheck.java:96:9:96:27 | parse(...) | A signing key is set $@, but the signature is not verified. | MissingJWTSignatureCheck.java:26:16:26:75 | setSigningKey(...) | here | -| MissingJWTSignatureCheck.java:100:9:105:22 | parse(...) | A signing key is set $@, but the signature is not verified. | MissingJWTSignatureCheck.java:18:16:18:66 | setSigningKey(...) | here | -| MissingJWTSignatureCheck.java:100:9:105:22 | parse(...) | A signing key is set $@, but the signature is not verified. | MissingJWTSignatureCheck.java:22:16:22:73 | setSigningKey(...) | here | -| MissingJWTSignatureCheck.java:100:9:105:22 | parse(...) | A signing key is set $@, but the signature is not verified. | MissingJWTSignatureCheck.java:26:16:26:75 | setSigningKey(...) | here | -| MissingJWTSignatureCheck.java:127:9:129:33 | parse(...) | A signing key is set $@, but the signature is not verified. | MissingJWTSignatureCheck.java:127:9:128:58 | setSigningKey(...) | here | -| MissingJWTSignatureCheck.java:133:9:140:22 | parse(...) | A signing key is set $@, but the signature is not verified. | MissingJWTSignatureCheck.java:133:9:134:58 | setSigningKey(...) | here | diff --git a/java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref b/java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref deleted file mode 100644 index 67e7bf3e158c..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.expected b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.java b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.java similarity index 59% rename from java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.java rename to java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.java index 624dbb1d48da..93b54154ffc1 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.java +++ b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.java @@ -1,18 +1,13 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/jwtk-jjwt-0.11.2 - -import io.jsonwebtoken.Jwts; -import io.jsonwebtoken.JwtParser; -import io.jsonwebtoken.Jwt; -import io.jsonwebtoken.Jws; import io.jsonwebtoken.Header; -import io.jsonwebtoken.JwtParserBuilder; +import io.jsonwebtoken.Jws; +import io.jsonwebtoken.Jwt; import io.jsonwebtoken.JwtHandlerAdapter; +import io.jsonwebtoken.JwtParser; +import io.jsonwebtoken.Jwts; import io.jsonwebtoken.impl.DefaultJwtParser; +import io.jsonwebtoken.impl.DefaultJwtParserBuilder; -public class MissingJWTSignatureCheck { - - - // SIGNED +public class MissingJWTSignatureCheckTest { private JwtParser getASignedParser() { return Jwts.parser().setSigningKey("someBase64EncodedKey"); @@ -46,10 +41,6 @@ private void callSignedParsers() { goodJwtHandler(parser3, ""); } - // SIGNED END - - // UNSIGNED - private JwtParser getAnUnsignedParser() { return Jwts.parser(); } @@ -84,81 +75,67 @@ private void callUnsignedParsers() { private void signParserAfterParseCall() { JwtParser parser = getAnUnsignedParser(); - parser.parse(""); // Should not be detected + parser.parse(""); // Safe parser.setSigningKey("someBase64EncodedKey"); } - // UNSIGNED END - - // INDIRECT - private void badJwtOnParserBuilder(JwtParser parser, String token) { - parser.parse(token); // BAD: Does not verify the signature + parser.parse(token); // $hasMissingJwtSignatureCheck } private void badJwtHandlerOnParserBuilder(JwtParser parser, String token) { - parser.parse(token, new JwtHandlerAdapter>() { // BAD: The handler is called on an unverified JWT - @Override - public Jwt onPlaintextJwt(Jwt jwt) { - return jwt; - } - }); + parser.parse(token, new JwtHandlerAdapter>() { // $hasMissingJwtSignatureCheck + @Override + public Jwt onPlaintextJwt(Jwt jwt) { + return jwt; + } + }); } private void goodJwtOnParserBuilder(JwtParser parser, String token) { - parser.parseClaimsJws(token) // GOOD: Verify the signature - .getBody(); + parser.parseClaimsJws(token) // Safe + .getBody(); } private void goodJwtHandler(JwtParser parser, String token) { - parser.parse(token, new JwtHandlerAdapter>() { // GOOD: The handler is called on a verified JWS - @Override - public Jws onPlaintextJws(Jws jws) { - return jws; - } - }); + parser.parse(token, new JwtHandlerAdapter>() { // Safe + @Override + public Jws onPlaintextJws(Jws jws) { + return jws; + } + }); } - // INDIRECT END - - // DIRECT - private void badJwtOnParserBuilder(String token) { - Jwts.parserBuilder() - .setSigningKey("someBase64EncodedKey").build() - .parse(token); // BAD: Does not verify the signature + Jwts.parserBuilder().setSigningKey("someBase64EncodedKey").build().parse(token); // $hasMissingJwtSignatureCheck + } + + private void badJwtOnDefaultParserBuilder(String token) { + new DefaultJwtParserBuilder().setSigningKey("someBase64EncodedKey").build().parse(token); // $hasMissingJwtSignatureCheck } private void badJwtHandlerOnParser(String token) { - Jwts.parser() - .setSigningKey("someBase64EncodedKey") - .parse(token, new JwtHandlerAdapter>() { // BAD: The handler is called on an unverified JWT - @Override - public Jwt onPlaintextJwt(Jwt jwt) { - return jwt; - } - }); + Jwts.parser().setSigningKey("someBase64EncodedKey").parse(token, // $hasMissingJwtSignatureCheck + new JwtHandlerAdapter>() { + @Override + public Jwt onPlaintextJwt(Jwt jwt) { + return jwt; + } + }); } private void goodJwtOnParser(String token) { - Jwts.parser() - .setSigningKey("someBase64EncodedKey") - .parseClaimsJws(token) // GOOD: Verify the signature - .getBody(); + Jwts.parser().setSigningKey("someBase64EncodedKey").parseClaimsJws(token) // Safe + .getBody(); } private void goodJwtHandlerOnParserBuilder(String token) { - Jwts.parserBuilder() - .setSigningKey("someBase64EncodedKey").build() - .parse(token, new JwtHandlerAdapter>() { // GOOD: The handler is called on a verified JWS - @Override - public Jws onPlaintextJws(Jws jws) { - return jws; - } - }); + Jwts.parserBuilder().setSigningKey("someBase64EncodedKey").build().parse(token, // Safe + new JwtHandlerAdapter>() { + @Override + public Jws onPlaintextJws(Jws jws) { + return jws; + } + }); } - - // DIRECT END - - } diff --git a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql new file mode 100644 index 000000000000..557fc28bf017 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql @@ -0,0 +1,20 @@ +import java +import semmle.code.java.security.MissingJWTSignatureCheckQuery +import TestUtilities.InlineExpectationsTest + +class HasMissingJwtSignatureCheckTest extends InlineExpectationsTest { + HasMissingJwtSignatureCheckTest() { this = "HasMissingJwtSignatureCheckTest" } + + override string getARelevantTag() { result = "hasMissingJwtSignatureCheck" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasMissingJwtSignatureCheck" and + exists(DataFlow::Node source, DataFlow::Node sink, MissingJwtSignatureCheckConf conf | + conf.hasFlow(source, sink) + | + sink.getLocation() = location and + element = sink.toString() and + value = "" + ) + } +} diff --git a/java/ql/test/query-tests/security/CWE-347/options b/java/ql/test/query-tests/security/CWE-347/options new file mode 100644 index 000000000000..286cff918e45 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-347/options @@ -0,0 +1 @@ +semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/jwtk-jjwt-0.11.2 \ No newline at end of file diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/LICENSE b/java/ql/test/stubs/jwtk-jjwt-0.11.2/LICENSE similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/LICENSE rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/LICENSE diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ClaimJwtException.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ClaimJwtException.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ClaimJwtException.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ClaimJwtException.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Claims.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Claims.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Claims.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Claims.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ClaimsMutator.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ClaimsMutator.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ClaimsMutator.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ClaimsMutator.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ExpiredJwtException.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ExpiredJwtException.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ExpiredJwtException.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ExpiredJwtException.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Header.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Header.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Header.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Header.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jws.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jws.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jws.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jws.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwsHeader.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwsHeader.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwsHeader.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwsHeader.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jwt.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jwt.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jwt.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jwt.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtException.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtException.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtException.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtException.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtHandler.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtHandler.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtHandler.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtHandler.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtHandlerAdapter.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtHandlerAdapter.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtHandlerAdapter.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtHandlerAdapter.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtParser.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtParser.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtParser.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtParser.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtParserBuilder.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtParserBuilder.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtParserBuilder.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtParserBuilder.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jwts.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jwts.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jwts.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jwts.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/MalformedJwtException.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/MalformedJwtException.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/MalformedJwtException.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/MalformedJwtException.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/SignatureException.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/SignatureException.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/SignatureException.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/SignatureException.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/SigningKeyResolver.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/SigningKeyResolver.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/SigningKeyResolver.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/SigningKeyResolver.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/UnsupportedJwtException.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/UnsupportedJwtException.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/UnsupportedJwtException.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/UnsupportedJwtException.java diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/impl/DefaultJwtParser.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/impl/DefaultJwtParser.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/impl/DefaultJwtParser.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/impl/DefaultJwtParser.java diff --git a/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/impl/DefaultJwtParserBuilder.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/impl/DefaultJwtParserBuilder.java new file mode 100644 index 000000000000..161531c47439 --- /dev/null +++ b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/impl/DefaultJwtParserBuilder.java @@ -0,0 +1,49 @@ + +/* + * Copyright (C) 2019 jsonwebtoken.io + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package io.jsonwebtoken.impl; + +import java.security.Key; +import io.jsonwebtoken.JwtParser; +import io.jsonwebtoken.JwtParserBuilder; +import io.jsonwebtoken.SigningKeyResolver; + + +public class DefaultJwtParserBuilder implements JwtParserBuilder { + + @Override + public JwtParserBuilder setSigningKey(byte[] key) { + return this; + } + + @Override + public JwtParserBuilder setSigningKey(String base64EncodedSecretKey) { + return this; + } + + @Override + public JwtParserBuilder setSigningKey(Key key) { + return this; + } + + @Override + public JwtParserBuilder setSigningKeyResolver(SigningKeyResolver signingKeyResolver) { + return this; + } + + @Override + public JwtParser build() { + return null; + } +} diff --git a/java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/security/SecurityException.java b/java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/security/SecurityException.java similarity index 100% rename from java/ql/test/experimental/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/security/SecurityException.java rename to java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/security/SecurityException.java