From 897cd5384f5dafa9426cb73e2b9f88871ca481ba Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 17 May 2021 14:44:33 +0200 Subject: [PATCH] Created JWT.qll and refactored to use CSV models --- .../CWE/CWE-347/MissingJWTSignatureCheck.ql | 190 +----------------- .../code/java/dataflow/ExternalFlow.qll | 1 + java/ql/src/semmle/code/java/security/JWT.qll | 172 ++++++++++++++++ 3 files changed, 177 insertions(+), 186 deletions(-) create mode 100644 java/ql/src/semmle/code/java/security/JWT.qll diff --git a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql index 873701f0eb3..d2cb81978b3 100644 --- a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql +++ b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql @@ -10,191 +10,9 @@ */ import java -import semmle.code.java.dataflow.DataFlow +import semmle.code.java.security.JWT -/** 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/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.", +from JwtParserWithInsecureParseSink sink, JwtParserWithSigningKeyExpr parserExpr +where sink.asExpr() = parserExpr +select sink.getParseMethodAccess(), "A signing key is set $@, but the signature is not verified.", parserExpr.getSigningMethodAccess(), "here" diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 5d159840611..c40d15394a1 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -81,6 +81,7 @@ private module Frameworks { private import semmle.code.java.security.XSS private import semmle.code.java.security.LdapInjection private import semmle.code.java.security.XPath + private import semmle.code.java.security.JWT } private predicate sourceModelCsv(string row) { 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 00000000000..203e03f1994 --- /dev/null +++ b/java/ql/src/semmle/code/java/security/JWT.qll @@ -0,0 +1,172 @@ +/** Provides classes for working with JWT libraries. */ + +import java +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.ExternalFlow + +/** + * An expr that is a (sub-type of) `JwtParser` for which a signing key has been set + */ +class JwtParserWithSigningKeyExpr extends Expr { + MethodAccess signingMa; + + JwtParserWithSigningKeyExpr() { isSigningKeySetter(this, signingMa) } + + /** Gets the method access that sets the signing key for this parser. */ + MethodAccess getSigningMethodAccess() { result = signingMa } +} + +/** + * The qualifier of an insecure parsing method. + * That is, either the qualifier of a call to a `parse(token)`, `parseClaimsJwt(token)` or `parsePlaintextJwt(token)` method 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 + ( + sinkNode(this, "jwt-insecure-parse") + or + sinkNode(this, "jwt-insecure-parse-handler") and + isInsecureJwtHandler(insecureParseMa.getArgument(1)) + ) + } + + MethodAccess getParseMethodAccess() { result = insecureParseMa } +} + +/** + * 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 (the signing key is set directly on a `JwtParser`): + * ```java + * Jwts.parser().setSigningKey(key).parse(token); + * ``` + * + * Indirectly means code like this (the signing key is set on a `JwtParserBuilder` indirectly setting the key of `JwtParser` that is created by the call to `build`): + * ```java + * Jwts.parserBuilder().setSigningKey(key).build().parse(token); + * ``` + */ +private predicate isSigningKeySetter(Expr expr, MethodAccess signingMa) { + any(SigningToInsecureMethodAccessDataFlow s) + .hasFlow(DataFlow::exprNode(signingMa), DataFlow::exprNode(expr)) +} + +/** + * 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 + ) + ) +} + +/** CSV source models representing methods that assign signing keys to a JWT parser. */ +private class SigningKeySourceModel extends SourceModelCsv { + override predicate row(string row) { + row = + [ + "io.jsonwebtoken;JwtParser;true;setSigningKey;;;ReturnValue;jwt-signing-key", + "io.jsonwebtoken;JwtParser;true;setSigningKeyResolver;;;ReturnValue;jwt-signing-key", + "io.jsonwebtoken;JwtParserBuilder;true;setSigningKey;;;ReturnValue;jwt-signing-key", + "io.jsonwebtoken;JwtParserBuilder;true;setSigningKeyResolver;;;ReturnValue;jwt-signing-key" + ] + } +} + +/** CSV sink models representing qualifiers of methods that insecurely parse a JWT */ +private class InsecureJwtParseSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "io.jsonwebtoken;JwtParser;true;parse;(String);;Argument[-1];jwt-insecure-parse", + "io.jsonwebtoken;JwtParser;true;parseClaimsJwt;;;Argument[-1];jwt-insecure-parse", + "io.jsonwebtoken;JwtParser;true;parsePlaintextJwt;;;Argument[-1];jwt-insecure-parse" + ] + } +} + +/** CSV sink models representing qualifiers of methods that insecurely parse a JWT with a handler */ +private class InsecureJwtParseHandlerSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "io.jsonwebtoken;JwtParser;true;parse;(String,JwtHandler);;Argument[-1];jwt-insecure-parse-handler" + ] + } +} + +/** 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 +} + +/** + * Models flow from signing keys assignements to qualifiers of JWT insecure parsers. + * 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) { sourceNode(source, "jwt-signing-key") } + + override predicate isSink(DataFlow::Node sink) { sink instanceof JwtParserWithInsecureParseSink } + + override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + jwtParserStep(node1.asExpr(), node2.asExpr()) + } +} + +/** 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 + } +}