From 3e4ccaf9a875ff7e1aed338389dd7ec629ce105b Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 17 May 2021 10:41:54 +0200 Subject: [PATCH 01/19] Move from experimental to standard --- .../Security/CWE/CWE-347/MissingJWTSignatureCheck.java | 0 .../Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp | 0 .../Security/CWE/CWE-347/MissingJWTSignatureCheck.ql | 2 +- .../query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref | 1 - .../security/CWE-347/MissingJWTSignatureCheck.expected | 0 .../query-tests/security/CWE-347/MissingJWTSignatureCheck.java | 0 .../query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref | 1 + java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/LICENSE | 0 .../jwtk-jjwt-0.11.2/io/jsonwebtoken/ClaimJwtException.java | 0 .../stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Claims.java | 0 .../stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ClaimsMutator.java | 0 .../jwtk-jjwt-0.11.2/io/jsonwebtoken/ExpiredJwtException.java | 0 .../stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Header.java | 0 .../stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jws.java | 0 .../stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwsHeader.java | 0 .../stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jwt.java | 0 .../stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtException.java | 0 .../stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtHandler.java | 0 .../jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtHandlerAdapter.java | 0 .../stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtParser.java | 0 .../jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtParserBuilder.java | 0 .../stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jwts.java | 0 .../jwtk-jjwt-0.11.2/io/jsonwebtoken/MalformedJwtException.java | 0 .../jwtk-jjwt-0.11.2/io/jsonwebtoken/SignatureException.java | 0 .../jwtk-jjwt-0.11.2/io/jsonwebtoken/SigningKeyResolver.java | 0 .../io/jsonwebtoken/UnsupportedJwtException.java | 0 .../jwtk-jjwt-0.11.2/io/jsonwebtoken/impl/DefaultJwtParser.java | 0 .../io/jsonwebtoken/security/SecurityException.java | 0 28 files changed, 2 insertions(+), 2 deletions(-) rename java/ql/src/{experimental => }/Security/CWE/CWE-347/MissingJWTSignatureCheck.java (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql (98%) delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref rename java/ql/test/{experimental => }/query-tests/security/CWE-347/MissingJWTSignatureCheck.expected (100%) rename java/ql/test/{experimental => }/query-tests/security/CWE-347/MissingJWTSignatureCheck.java (100%) create mode 100644 java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/LICENSE (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ClaimJwtException.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Claims.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ClaimsMutator.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/ExpiredJwtException.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Header.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jws.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwsHeader.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jwt.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtException.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtHandler.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtHandlerAdapter.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtParser.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/JwtParserBuilder.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/Jwts.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/MalformedJwtException.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/SignatureException.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/SigningKeyResolver.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/UnsupportedJwtException.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/impl/DefaultJwtParser.java (100%) rename java/ql/test/{experimental => }/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/security/SecurityException.java (100%) 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 100% rename from java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp rename to java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp diff --git a/java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql similarity index 98% rename from java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql rename to java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql index 6d7462f1338e..873701f0eb31 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql +++ b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql @@ -190,7 +190,7 @@ private predicate isInTestFile(MethodAccess ma) { | lowerCasedAbsolutePath.matches("%/test/%") and not lowerCasedAbsolutePath - .matches("%/ql/test/experimental/query-tests/security/CWE-347/%".toLowerCase()) + .matches("%/ql/test/query-tests/security/CWE-347/%".toLowerCase()) ) } 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/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.expected b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.expected similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.expected rename to java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.expected diff --git a/java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.java b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.java similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-347/MissingJWTSignatureCheck.java rename to java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.java diff --git a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref new file mode 100644 index 000000000000..aeb030ef1455 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-347/MissingJWTSignatureCheck.ql \ 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/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 From 897cd5384f5dafa9426cb73e2b9f88871ca481ba Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 17 May 2021 14:44:33 +0200 Subject: [PATCH 02/19] 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 873701f0eb31..d2cb81978b33 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 5d1598406113..c40d15394a12 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 000000000000..203e03f19945 --- /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 + } +} From cfb38c43b34a901c698356304da19226f60acf1e Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 17 May 2021 15:04:50 +0200 Subject: [PATCH 03/19] QLDocs --- java/ql/src/semmle/code/java/security/JWT.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/ql/src/semmle/code/java/security/JWT.qll b/java/ql/src/semmle/code/java/security/JWT.qll index 203e03f19945..ac92c9c3a95f 100644 --- a/java/ql/src/semmle/code/java/security/JWT.qll +++ b/java/ql/src/semmle/code/java/security/JWT.qll @@ -34,6 +34,7 @@ class JwtParserWithInsecureParseSink extends DataFlow::Node { ) } + /** Gets the method access that does the insecure parsing. */ MethodAccess getParseMethodAccess() { result = insecureParseMa } } @@ -86,7 +87,7 @@ private class SigningKeySourceModel extends SourceModelCsv { } } -/** CSV sink models representing qualifiers of methods that insecurely parse a JWT */ +/** CSV sink models representing qualifiers of methods that parse a JWT insecurely. */ private class InsecureJwtParseSinkModel extends SinkModelCsv { override predicate row(string row) { row = From bc2370ae1dd3c7ce86252dc2e30551c4e1b39020 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 17 May 2021 15:58:33 +0200 Subject: [PATCH 04/19] Use InlineExpectationsTest for tests --- .../CWE-347/MissingJWTSignatureCheck.expected | 8 -- .../CWE-347/MissingJWTSignatureCheck.qlref | 1 - .../MissingJWTSignatureCheckTest.expected | 0 ...java => MissingJWTSignatureCheckTest.java} | 95 +++++++------------ .../CWE-347/MissingJWTSignatureCheckTest.ql | 19 ++++ .../test/query-tests/security/CWE-347/options | 1 + 6 files changed, 54 insertions(+), 70 deletions(-) delete mode 100644 java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.expected delete mode 100644 java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref create mode 100644 java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.expected rename java/ql/test/query-tests/security/CWE-347/{MissingJWTSignatureCheck.java => MissingJWTSignatureCheckTest.java} (59%) create mode 100644 java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql create mode 100644 java/ql/test/query-tests/security/CWE-347/options diff --git a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.expected b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.expected deleted file mode 100644 index e93720fcea83..000000000000 --- a/java/ql/test/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/query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref deleted file mode 100644 index aeb030ef1455..000000000000 --- a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.qlref +++ /dev/null @@ -1 +0,0 @@ -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/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/query-tests/security/CWE-347/MissingJWTSignatureCheck.java rename to java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.java index 624dbb1d48da..5dd3494388ab 100644 --- a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheck.java +++ b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.java @@ -1,5 +1,3 @@ -//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; @@ -9,10 +7,7 @@ import io.jsonwebtoken.JwtHandlerAdapter; import io.jsonwebtoken.impl.DefaultJwtParser; -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,63 @@ 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 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..9f5f8619bc2c --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql @@ -0,0 +1,19 @@ +import java +import semmle.code.java.security.JWT +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(JwtParserWithInsecureParseSink sink, JwtParserWithSigningKeyExpr parserExpr | + sink.asExpr() = parserExpr and + 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 From 347bd2ebc2eec49cdb7034cd88c1a406fd5dd74a Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 17 May 2021 17:51:07 +0200 Subject: [PATCH 05/19] Added change note --- .../2021-05-17-missing-jwt-signature-check-query.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 java/change-notes/2021-05-17-missing-jwt-signature-check-query.md 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..1e992e111963 --- /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 From 34a55e77ef1a95c224b6ad9962c490b183cf30ef Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 18 May 2021 09:38:35 +0200 Subject: [PATCH 06/19] Add missing subtype test --- .../CWE-347/MissingJWTSignatureCheckTest.java | 14 ++++-- .../impl/DefaultJwtParserBuilder.java | 49 +++++++++++++++++++ 2 files changed, 58 insertions(+), 5 deletions(-) create mode 100644 java/ql/test/stubs/jwtk-jjwt-0.11.2/io/jsonwebtoken/impl/DefaultJwtParserBuilder.java 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 5dd3494388ab..93b54154ffc1 100644 --- a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.java +++ b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.java @@ -1,11 +1,11 @@ -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 MissingJWTSignatureCheckTest { @@ -110,6 +110,10 @@ private void badJwtOnParserBuilder(String token) { 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, // $hasMissingJwtSignatureCheck new JwtHandlerAdapter>() { 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; + } +} From 2dd862661bfa62080e68f4ac8281da5d901b6986 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 16 Jun 2021 16:23:50 +0200 Subject: [PATCH 07/19] Generic type parameters no longer needed in CSV sink models --- java/ql/src/semmle/code/java/security/JWT.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/semmle/code/java/security/JWT.qll b/java/ql/src/semmle/code/java/security/JWT.qll index ac92c9c3a95f..c7d7fd1909a0 100644 --- a/java/ql/src/semmle/code/java/security/JWT.qll +++ b/java/ql/src/semmle/code/java/security/JWT.qll @@ -104,7 +104,7 @@ private class InsecureJwtParseHandlerSinkModel extends SinkModelCsv { override predicate row(string row) { row = [ - "io.jsonwebtoken;JwtParser;true;parse;(String,JwtHandler);;Argument[-1];jwt-insecure-parse-handler" + "io.jsonwebtoken;JwtParser;true;parse;(String,JwtHandler);;Argument[-1];jwt-insecure-parse-handler" ] } } From 22c9baa462f90da9421b154ea5346cf83cfe519a Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 20 Jul 2021 17:14:34 +0200 Subject: [PATCH 08/19] Refactor JWT.qll --- .../CWE/CWE-347/MissingJWTSignatureCheck.ql | 2 +- java/ql/src/semmle/code/java/security/JWT.qll | 142 ++++++------------ .../semmle/code/java/security/JWTQuery.qll | 57 +++++++ .../CWE-347/MissingJWTSignatureCheckTest.ql | 2 +- 4 files changed, 103 insertions(+), 100 deletions(-) create mode 100644 java/ql/src/semmle/code/java/security/JWTQuery.qll diff --git a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql index d2cb81978b33..01468662dea9 100644 --- a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql +++ b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql @@ -10,7 +10,7 @@ */ import java -import semmle.code.java.security.JWT +import semmle.code.java.security.JWTQuery from JwtParserWithInsecureParseSink sink, JwtParserWithSigningKeyExpr parserExpr where sink.asExpr() = parserExpr diff --git a/java/ql/src/semmle/code/java/security/JWT.qll b/java/ql/src/semmle/code/java/security/JWT.qll index c7d7fd1909a0..16c88f131576 100644 --- a/java/ql/src/semmle/code/java/security/JWT.qll +++ b/java/ql/src/semmle/code/java/security/JWT.qll @@ -1,36 +1,44 @@ /** 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 } +private import semmle.code.java.dataflow.ExternalFlow +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 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. + * 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 - ( - sinkNode(this, "jwt-insecure-parse") - or - sinkNode(this, "jwt-insecure-parse-handler") and - isInsecureJwtHandler(insecureParseMa.getArgument(1)) + 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)) + ) ) } @@ -38,28 +46,26 @@ class JwtParserWithInsecureParseSink extends DataFlow::Node { 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)) +/** A set of additional taint steps to consider when taint tracking JWT related data flows. */ +class JwtParserWithInsecureParseAdditionalTaintStep extends Unit { + 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`. + * 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) { @@ -74,66 +80,6 @@ private predicate isInsecureJwtHandler(Expr parseHandlerExpr) { ) } -/** 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 parse a JWT insecurely. */ -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") } diff --git a/java/ql/src/semmle/code/java/security/JWTQuery.qll b/java/ql/src/semmle/code/java/security/JWTQuery.qll new file mode 100644 index 000000000000..e991e0bb9f5a --- /dev/null +++ b/java/ql/src/semmle/code/java/security/JWTQuery.qll @@ -0,0 +1,57 @@ +/** Provides classes to be used in queries related to JWT vulnerabilities. */ + +import java +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.ExternalFlow +import semmle.code.java.security.JWT + +/** + * 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 } +} + +/** + * 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)) +} + +/** + * 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) { + source instanceof JwtParserWithInsecureParseSource + } + + override predicate isSink(DataFlow::Node sink) { sink instanceof JwtParserWithInsecureParseSink } + + override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + any(JwtParserWithInsecureParseAdditionalTaintStep c).step(node1, node2) + } +} + + diff --git a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql index 9f5f8619bc2c..35afd69d339d 100644 --- a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql +++ b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql @@ -1,5 +1,5 @@ import java -import semmle.code.java.security.JWT +import semmle.code.java.security.JWTQuery import TestUtilities.InlineExpectationsTest class HasMissingJwtSignatureCheckTest extends InlineExpectationsTest { From ed0db7c7b44182595f89585bbafa458c38e477ff Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 20 Jul 2021 17:24:24 +0200 Subject: [PATCH 09/19] Fix release note --- .../2021-05-17-missing-jwt-signature-check-query.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 1e992e111963..46c0ca7f8b0e 100644 --- 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 @@ -1,2 +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 +* 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 From 76905c47b4762ca25770d2a93b5234d286860276 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 21 Jul 2021 09:47:45 +0200 Subject: [PATCH 10/19] Formatting --- java/ql/src/semmle/code/java/security/JWTQuery.qll | 2 -- 1 file changed, 2 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/JWTQuery.qll b/java/ql/src/semmle/code/java/security/JWTQuery.qll index e991e0bb9f5a..a55f0058b37a 100644 --- a/java/ql/src/semmle/code/java/security/JWTQuery.qll +++ b/java/ql/src/semmle/code/java/security/JWTQuery.qll @@ -53,5 +53,3 @@ private class SigningToInsecureMethodAccessDataFlow extends DataFlow::Configurat any(JwtParserWithInsecureParseAdditionalTaintStep c).step(node1, node2) } } - - From ebf004a4df79e2153a08740fea199894ea681d7d Mon Sep 17 00:00:00 2001 From: mc <42146119+mchammer01@users.noreply.github.com> Date: Thu, 29 Jul 2021 09:13:00 +0100 Subject: [PATCH 11/19] Update MissingJWTSignatureCheck.qhelp Using same syntax as on other queries for 'BAD' and 'GOOD'. --- .../src/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.qhelp index 3b93936c21b8..f682db85618c 100644 --- a/java/ql/src/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.

From 4ea6729c536e43d4da50842f40b40909b4e86564 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 29 Jul 2021 16:10:49 +0200 Subject: [PATCH 12/19] Update java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com> --- java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql index 01468662dea9..00d0503b6f78 100644 --- a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql +++ b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql @@ -1,6 +1,6 @@ /** * @name Missing JWT signature check - * @description Not checking the JWT signature allows an attacker to forge their own tokens. + * @description Failing to check the JWT signature may allow an attacker to forge their own tokens. * @kind problem * @problem.severity error * @precision high From f4bc4df8c172b592aa9cbad29fce241061963a4a Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 4 Aug 2021 12:08:08 +0200 Subject: [PATCH 13/19] Renamed JWTQuery so that it's named after the actual query name --- java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql | 2 +- .../{JWTQuery.qll => MissingJWTSignatureCheckQuery.qll} | 2 +- .../security/CWE-347/MissingJWTSignatureCheckTest.ql | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) rename java/ql/src/semmle/code/java/security/{JWTQuery.qll => MissingJWTSignatureCheckQuery.qll} (95%) diff --git a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql index 00d0503b6f78..ad6e41320ee6 100644 --- a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql +++ b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql @@ -10,7 +10,7 @@ */ import java -import semmle.code.java.security.JWTQuery +import semmle.code.java.security.MissingJWTSignatureCheckQuery from JwtParserWithInsecureParseSink sink, JwtParserWithSigningKeyExpr parserExpr where sink.asExpr() = parserExpr diff --git a/java/ql/src/semmle/code/java/security/JWTQuery.qll b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll similarity index 95% rename from java/ql/src/semmle/code/java/security/JWTQuery.qll rename to java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll index a55f0058b37a..7432eaac85d8 100644 --- a/java/ql/src/semmle/code/java/security/JWTQuery.qll +++ b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll @@ -1,4 +1,4 @@ -/** Provides classes to be used in queries related to JWT vulnerabilities. */ +/** Provides classes to be used in queries related to JWT signature vulnerabilities. */ import java import semmle.code.java.dataflow.DataFlow diff --git a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql index 35afd69d339d..d9d4be1e4d5b 100644 --- a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql +++ b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql @@ -1,5 +1,5 @@ import java -import semmle.code.java.security.JWTQuery +import semmle.code.java.security.MissingJWTSignatureCheckQuery import TestUtilities.InlineExpectationsTest class HasMissingJwtSignatureCheckTest extends InlineExpectationsTest { From b586f3ec9c6cd0c6255166d6b9aac23f2b9db106 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 4 Aug 2021 12:11:17 +0200 Subject: [PATCH 14/19] Make the additional flow step abstract --- java/ql/src/semmle/code/java/security/JWT.qll | 15 ++++++++++++--- .../security/MissingJWTSignatureCheckQuery.qll | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/JWT.qll b/java/ql/src/semmle/code/java/security/JWT.qll index 16c88f131576..d7fec0ea9588 100644 --- a/java/ql/src/semmle/code/java/security/JWT.qll +++ b/java/ql/src/semmle/code/java/security/JWT.qll @@ -46,9 +46,18 @@ class JwtParserWithInsecureParseSink extends DataFlow::Node { MethodAccess getParseMethodAccess() { result = insecureParseMa } } -/** A set of additional taint steps to consider when taint tracking JWT related data flows. */ -class JwtParserWithInsecureParseAdditionalTaintStep extends Unit { - predicate step(DataFlow::Node node1, DataFlow::Node node2) { +/** + * A unit class for adding additional flow steps. + * + * Extend this class to add additional flow steps that should apply to the `SigningToInsecureMethodAccessDataFlow`. + */ +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()) } } diff --git a/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll index 7432eaac85d8..634fac69c61a 100644 --- a/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll +++ b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll @@ -50,6 +50,6 @@ private class SigningToInsecureMethodAccessDataFlow extends DataFlow::Configurat override predicate isSink(DataFlow::Node sink) { sink instanceof JwtParserWithInsecureParseSink } override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - any(JwtParserWithInsecureParseAdditionalTaintStep c).step(node1, node2) + any(JwtParserWithInsecureParseAdditionalFlowStep c).step(node1, node2) } } From 452fd9a8e37f76de3a009361910189dd96689bcf Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 4 Aug 2021 13:05:18 +0200 Subject: [PATCH 15/19] Refactor to path query --- .../CWE/CWE-347/MissingJWTSignatureCheck.ql | 12 +++--- .../MissingJWTSignatureCheckQuery.qll | 37 ++----------------- .../CWE-347/MissingJWTSignatureCheckTest.ql | 5 ++- 3 files changed, 13 insertions(+), 41 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql index ad6e41320ee6..b17915a8fd55 100644 --- a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql +++ b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql @@ -1,8 +1,9 @@ /** * @name Missing JWT signature check * @description Failing to check the JWT signature may allow an attacker to forge their own tokens. - * @kind problem + * @kind path-problem * @problem.severity error + * @security-severity 7.8 * @precision high * @id java/missing-jwt-signature-check * @tags security @@ -11,8 +12,9 @@ import java import semmle.code.java.security.MissingJWTSignatureCheckQuery +import DataFlow::PathGraph -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" +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/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll index 634fac69c61a..f70b9ab64472 100644 --- a/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll +++ b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll @@ -5,43 +5,12 @@ import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.ExternalFlow import semmle.code.java.security.JWT -/** - * 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 } -} - -/** - * 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)) -} - /** * 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. + * This is used to determine whether a `JwtParser` performing unsafe parsing has a signing key set. */ -private class SigningToInsecureMethodAccessDataFlow extends DataFlow::Configuration { - SigningToInsecureMethodAccessDataFlow() { this = "SigningToExprDataFlow" } +class MissingJwtSignatureCheckConf extends DataFlow::Configuration { + MissingJwtSignatureCheckConf() { this = "SigningToExprDataFlow" } override predicate isSource(DataFlow::Node source) { source instanceof JwtParserWithInsecureParseSource diff --git a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql index d9d4be1e4d5b..557fc28bf017 100644 --- a/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql +++ b/java/ql/test/query-tests/security/CWE-347/MissingJWTSignatureCheckTest.ql @@ -9,8 +9,9 @@ class HasMissingJwtSignatureCheckTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasMissingJwtSignatureCheck" and - exists(JwtParserWithInsecureParseSink sink, JwtParserWithSigningKeyExpr parserExpr | - sink.asExpr() = parserExpr and + exists(DataFlow::Node source, DataFlow::Node sink, MissingJwtSignatureCheckConf conf | + conf.hasFlow(source, sink) + | sink.getLocation() = location and element = sink.toString() and value = "" From a046d75ea68cfb371d9496b680910161d8f6f2c7 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 4 Aug 2021 13:15:49 +0200 Subject: [PATCH 16/19] Apply suggestions from code review --- java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql | 2 +- java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll | 1 - java/ql/src/semmle/code/java/security/JWT.qll | 2 +- .../code/java/security/MissingJWTSignatureCheckQuery.qll | 3 +-- 4 files changed, 3 insertions(+), 5 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql index b17915a8fd55..30caee117c88 100644 --- a/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql +++ b/java/ql/src/Security/CWE/CWE-347/MissingJWTSignatureCheck.ql @@ -1,6 +1,6 @@ /** * @name Missing JWT signature check - * @description Failing to check the JWT signature may allow an attacker to forge their own tokens. + * @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 diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index fa1461768c3f..e3d9e7e35cf0 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -97,7 +97,6 @@ private module Frameworks { private import semmle.code.java.security.ResponseSplitting private import semmle.code.java.security.InformationLeak private import semmle.code.java.security.JexlInjectionSinkModels - private import semmle.code.java.security.JWT private import semmle.code.java.security.LdapInjection private import semmle.code.java.security.XPath private import semmle.code.java.frameworks.android.SQLite diff --git a/java/ql/src/semmle/code/java/security/JWT.qll b/java/ql/src/semmle/code/java/security/JWT.qll index d7fec0ea9588..3f4453c62dd9 100644 --- a/java/ql/src/semmle/code/java/security/JWT.qll +++ b/java/ql/src/semmle/code/java/security/JWT.qll @@ -1,4 +1,4 @@ -/** Provides classes for working with JWT libraries. */ +/** Provides classes for working with JSON Web Token (JWT) libraries. */ import java private import semmle.code.java.dataflow.ExternalFlow diff --git a/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll index f70b9ab64472..82c036408756 100644 --- a/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll +++ b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll @@ -1,8 +1,7 @@ -/** Provides classes to be used in queries related to JWT signature vulnerabilities. */ +/** 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.dataflow.ExternalFlow import semmle.code.java.security.JWT /** From bc9563c07350ed0346eb72751f14d3fb0285eb54 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 4 Aug 2021 14:40:32 +0200 Subject: [PATCH 17/19] Apply suggestions from code review Co-authored-by: Anders Schack-Mulligen --- java/ql/src/semmle/code/java/security/JWT.qll | 1 - .../semmle/code/java/security/MissingJWTSignatureCheckQuery.qll | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/JWT.qll b/java/ql/src/semmle/code/java/security/JWT.qll index 3f4453c62dd9..298ee305c0b4 100644 --- a/java/ql/src/semmle/code/java/security/JWT.qll +++ b/java/ql/src/semmle/code/java/security/JWT.qll @@ -1,7 +1,6 @@ /** Provides classes for working with JSON Web Token (JWT) libraries. */ import java -private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.DataFlow /** A method access that assigns signing keys to a JWT parser. */ diff --git a/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll index 82c036408756..818a4c664fe6 100644 --- a/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll +++ b/java/ql/src/semmle/code/java/security/MissingJWTSignatureCheckQuery.qll @@ -5,7 +5,7 @@ import semmle.code.java.dataflow.DataFlow import semmle.code.java.security.JWT /** - * Models flow from signing keys assignements to qualifiers of JWT insecure parsers. + * 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 { From 78998d0ca1e4572342497fe10493d33c0bb838af Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 4 Aug 2021 16:22:56 +0200 Subject: [PATCH 18/19] Update java/ql/src/semmle/code/java/security/JWT.qll --- java/ql/src/semmle/code/java/security/JWT.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/semmle/code/java/security/JWT.qll b/java/ql/src/semmle/code/java/security/JWT.qll index 298ee305c0b4..e902e9cd1c66 100644 --- a/java/ql/src/semmle/code/java/security/JWT.qll +++ b/java/ql/src/semmle/code/java/security/JWT.qll @@ -48,7 +48,7 @@ class JwtParserWithInsecureParseSink extends DataFlow::Node { /** * A unit class for adding additional flow steps. * - * Extend this class to add additional flow steps that should apply to the `SigningToInsecureMethodAccessDataFlow`. + * 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); From 5f9f857c349f04af0bd5907f64101ce3b184a6c4 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 4 Aug 2021 16:23:21 +0200 Subject: [PATCH 19/19] Update java/ql/src/semmle/code/java/security/JWT.qll --- java/ql/src/semmle/code/java/security/JWT.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/semmle/code/java/security/JWT.qll b/java/ql/src/semmle/code/java/security/JWT.qll index e902e9cd1c66..a7ed68ffddc0 100644 --- a/java/ql/src/semmle/code/java/security/JWT.qll +++ b/java/ql/src/semmle/code/java/security/JWT.qll @@ -48,7 +48,7 @@ class JwtParserWithInsecureParseSink extends DataFlow::Node { /** * A unit class for adding additional flow steps. * - * Extend this class to add additional flow steps that should apply to the `MissingJwtSignatureCheckConf `. + * 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);