Merge pull request #15585 from atorralba/atorralba/go/promote-jwt-unsafe-verification

Go: Promote `go/missing-jwt-signature-check` from experimental
This commit is contained in:
Tony Torralba
2024-02-19 15:35:44 +01:00
committed by GitHub
27 changed files with 346 additions and 142 deletions

View File

@@ -3,5 +3,19 @@ extensions:
pack: codeql/go-all
extensible: sinkModel
data:
- ["github.com/dgrijalva/jwt-go", "Token", True, "SignedString", "", "", "Argument[0]", "credentials-key", "manual"]
- ["github.com/dgrijalva/jwt-go", "SigningMethod", True, "Sign", "", "", "Argument[1]", "credentials-key", "manual"]
- ["github.com/dgrijalva/jwt-go", "Token", True, "SignedString", "", "", "Argument[0]", "credentials-key", "manual"]
- ["github.com/dgrijalva/jwt-go", "Parser", True, "ParseUnverified", "", "", "Argument[0]", "jwt", "manual"]
- addsTo:
pack: codeql/go-all
extensible: summaryModel
data:
- ["github.com/dgrijalva/jwt-go", "", True, "Parse", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/dgrijalva/jwt-go", "Parser", True, "Parse", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/dgrijalva/jwt-go", "", True, "ParseWithClaims", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/dgrijalva/jwt-go", "Parser", True, "ParseWithClaims", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/dgrijalva/jwt-go", "", True, "ParseECPrivateKeyFromPEM", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/dgrijalva/jwt-go", "", True, "ParseECPublicKeyFromPEM", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/dgrijalva/jwt-go", "", True, "ParseRSAPrivateKeyFromPEM", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/dgrijalva/jwt-go", "", True, "ParseRSAPrivateKeyFromPEMWithPassword", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/dgrijalva/jwt-go", "", True, "ParseRSAPublicKeyFromPEM", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]

View File

@@ -0,0 +1,14 @@
extensions:
- addsTo:
pack: codeql/go-all
extensible: sinkModel
data:
- ["github.com/go-jose/go-jose/$ANYVERSION/jwt", "JSONWebToken", True, "UnsafeClaimsWithoutVerification", "", "", "Argument[-1]", "jwt", "manual"]
- addsTo:
pack: codeql/go-all
extensible: summaryModel
data:
- ["github.com/go-jose/go-jose/$ANYVERSION/jwt", "", True, "ParseEncrypted", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/go-jose/go-jose/$ANYVERSION/jwt", "", True, "ParseSigned", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/go-jose/go-jose/$ANYVERSION/jwt", "NestedJSONWebToken", True, "ParseSignedAndEncrypted", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/go-jose/go-jose/$ANYVERSION/jwt", "NestedJSONWebToken", True, "Decrypt", "", "", "Argument[-1]", "ReturnValue[0]", "taint", "manual"]

View File

@@ -3,5 +3,21 @@ extensions:
pack: codeql/go-all
extensible: sinkModel
data:
- ["github.com/golang-jwt/jwt", "Token", True, "SignedString", "", "", "Argument[0]", "credentials-key", "manual"]
- ["github.com/golang-jwt/jwt", "SigningMethod", True, "Sign", "", "", "Argument[1]", "credentials-key", "manual"]
- ["github.com/golang-jwt/jwt", "Token", True, "SignedString", "", "", "Argument[0]", "credentials-key", "manual"]
- ["github.com/golang-jwt/jwt", "Parser", True, "ParseUnverified", "", "", "Argument[0]", "jwt", "manual"]
- addsTo:
pack: codeql/go-all
extensible: summaryModel
data:
- ["github.com/golang-jwt/jwt", "", True, "Parse", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/golang-jwt/jwt", "Parser", True, "Parse", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/golang-jwt/jwt", "", True, "ParseWithClaims", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/golang-jwt/jwt", "Parser", True, "ParseWithClaims", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/golang-jwt/jwt", "", True, "ParseECPrivateKeyFromPEM", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/golang-jwt/jwt", "", True, "ParseECPublicKeyFromPEM", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/golang-jwt/jwt", "", True, "ParseEdPrivateKeyFromPEM", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/golang-jwt/jwt", "", True, "ParseEdPublicKeyFromPEM", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/golang-jwt/jwt", "", True, "ParseRSAPrivateKeyFromPEM", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/golang-jwt/jwt", "", True, "ParseRSAPublicKeyFromPEM", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["github.com/golang-jwt/jwt", "", True, "RegisterSigningMethod", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]

View File

@@ -0,0 +1,14 @@
extensions:
- addsTo:
pack: codeql/go-all
extensible: sinkModel
data:
- ["gopkg.in/square/go-jose.v2/jwt", "JSONWebToken", True, "UnsafeClaimsWithoutVerification", "", "", "Argument[-1]", "jwt", "manual"]
- addsTo:
pack: codeql/go-all
extensible: summaryModel
data:
- ["gopkg.in/square/go-jose.v2/jwt", "", True, "ParseEncrypted", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["gopkg.in/square/go-jose.v2/jwt", "", True, "ParseSigned", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["gopkg.in/square/go-jose.v2/jwt", "NestedJSONWebToken", True, "ParseSignedAndEncrypted", "", "", "Argument[0]", "ReturnValue[0]", "taint", "manual"]
- ["gopkg.in/square/go-jose.v2/jwt", "NestedJSONWebToken", True, "Decrypt", "", "", "Argument[-1]", "ReturnValue[0]", "taint", "manual"]

View File

@@ -52,6 +52,7 @@ import semmle.go.frameworks.GoMicro
import semmle.go.frameworks.GoRestfulHttp
import semmle.go.frameworks.Gqlgen
import semmle.go.frameworks.Iris
import semmle.go.frameworks.Jwt
import semmle.go.frameworks.K8sIoApimachineryPkgRuntime
import semmle.go.frameworks.K8sIoApiCoreV1
import semmle.go.frameworks.K8sIoClientGo

View File

@@ -9,16 +9,40 @@ private import semmle.go.security.HardcodedCredentials
private module GoJose {
private class GoJoseKey extends HardcodedCredentials::Sink {
GoJoseKey() {
exists(Field f, string pkg |
pkg =
[
package("github.com/square/go-jose", ""), package("github.com/go-jose/go-jose", ""),
"gopkg.in/square/go-jose.v2"
]
|
f.hasQualifiedName(pkg, ["Recipient", "SigningKey"], "Key") and
exists(Field f |
f.hasQualifiedName(goJosePackage(), ["Recipient", "SigningKey"], "Key") and
f.getAWrite().getRhs() = this
)
}
}
private string goJosePackage() {
result =
[
package("github.com/square/go-jose", ""), package("github.com/go-jose/go-jose", ""),
"gopkg.in/square/go-jose.v2"
]
}
/**
* Provides classes and predicates for working with the `gopkg.in/square/go-jose/jwt` and
* `github.com/go-jose/go-jose/jwt` packages.
*/
private module Jwt {
private import semmle.go.security.MissingJwtSignatureCheckCustomizations::MissingJwtSignatureCheck
/** The method `JSONWebToken.Claims`. */
private class GoJoseParseWithClaims extends JwtSafeParse {
GoJoseParseWithClaims() {
this.(Method).hasQualifiedName(goJoseJwtPackage(), "JSONWebToken", "Claims")
}
override int getTokenArgNum() { result = -1 }
}
/** Gets the package names `gopkg.in/square/go-jose/jwt` and `github.com/go-jose/go-jose/jwt`. */
private string goJoseJwtPackage() {
result = package(["gopkg.in/square/go-jose", "github.com/go-jose/go-jose"], "jwt")
}
}
}

View File

@@ -0,0 +1,57 @@
/**
* Provides classes and predicates for working with the `github.com/golang-jwt/jwt` and
* `github.com/dgrijalva/jwt-go` packages.
*/
import go
private import semmle.go.security.MissingJwtSignatureCheckCustomizations::MissingJwtSignatureCheck
/** The function `jwt.Parse` or the method `Parser.Parse`. */
private class GolangJwtParse extends JwtSafeParse {
GolangJwtParse() {
this.hasQualifiedName(golangJwtPackage(), "Parse")
or
this.(Method).hasQualifiedName(golangJwtPackage(), "Parser", "Parse")
}
override int getTokenArgNum() { result = 0 }
}
/** The function `jwt.ParseWithClaims` or the method `Parser.ParseWithClaims`. */
private class GolangJwtParseWithClaims extends JwtSafeParse {
GolangJwtParseWithClaims() {
this.hasQualifiedName(golangJwtPackage(), "ParseWithClaims")
or
this.(Method).hasQualifiedName(golangJwtPackage(), "Parser", "ParseWithClaims")
}
override int getTokenArgNum() { result = 0 }
}
/** The function `jwt.ParseFromRequest`. */
private class GolangJwtParseFromRequest extends JwtSafeParse {
GolangJwtParseFromRequest() {
this.hasQualifiedName(golangJwtRequestPackage(), "ParseFromRequest")
}
override int getTokenArgNum() { result = 0 }
}
/** The function `jwt.ParseFromRequestWithClaims`. */
private class GolangJwtParseFromRequestWithClaims extends JwtSafeParse {
GolangJwtParseFromRequestWithClaims() {
this.hasQualifiedName(golangJwtRequestPackage(), "ParseFromRequestWithClaims")
}
override int getTokenArgNum() { result = 0 }
}
/** Gets the pakcage names `github.com/golang-jwt/jwt` and `github.com/dgrijalva/jwt-go`. */
private string golangJwtPackage() {
result = package(["github.com/golang-jwt/jwt", "github.com/dgrijalva/jwt-go"], "")
}
/** Gets the package names `github.com/golang-jwt/jwt/request` and `github.com/dgrijalva/jwt-go/request`. */
private string golangJwtRequestPackage() {
result = package(["github.com/golang-jwt/jwt", "github.com/dgrijalva/jwt-go"], "request")
}

View File

@@ -0,0 +1,42 @@
/**
* Provides a taint tracking flow for reasoning about JWT vulnerabilities.
*
* Note: for performance reasons, only import this file if `MissingJwtSignatureCheck::Config` or `MissingJwtSignatureCheck::Flow` are needed,
* otherwise `MissingJwtSignatureCheckCustomizations` should be imported instead.
*/
import go
/** Provides a taint-tracking flow for reasoning about JWT vulnerabilities. */
module MissingJwtSignatureCheck {
import MissingJwtSignatureCheckCustomizations::MissingJwtSignatureCheck
/** Config for reasoning about JWT vulnerabilities. */
module Config implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) {
source instanceof Source and
not SafeParse::flow(source, _)
}
predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
any(AdditionalFlowStep s).step(nodeFrom, nodeTo)
}
}
/** Tracks taint flow for reasoning about JWT vulnerabilities. */
module Flow = TaintTracking::Global<Config>;
private module SafeParseConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source instanceof Source }
predicate isSink(DataFlow::Node sink) { sink = any(JwtSafeParse jwtParse).getTokenArg() }
predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
any(AdditionalFlowStep s).step(nodeFrom, nodeTo)
}
}
private module SafeParse = TaintTracking::Global<SafeParseConfig>;
}

View File

@@ -0,0 +1,57 @@
/**
* Provides default sources, sinks, and sanitizers for reasoning about
* JWT vulnerabilities, as well as extension points for adding your own.
*/
import go
private import semmle.go.dataflow.ExternalFlow
private import codeql.util.Unit
/**
* Provides extension points for customizing the data-flow tracking configuration for reasoning
* about JWT vulnerabilities.
*/
module MissingJwtSignatureCheck {
/**
* A data flow source for JWT vulnerabilities.
*/
abstract class Source extends DataFlow::Node { }
/**
* A data flow sink for JWT vulnerabilities.
*/
abstract class Sink extends DataFlow::Node { }
/**
* A sanitizer for JWT vulnerabilities.
*/
abstract class Sanitizer extends DataFlow::Node { }
/** An additional flow step for JWT vulnerabilities. */
class AdditionalFlowStep extends Unit {
/**
* Holds if the step from `node1` to `node2` should be considered a flow
* step for configurations related to JWT vulnerabilities.
*/
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
}
/** A function that parses and correctly validates a JWT token. */
abstract class JwtSafeParse extends Function {
/** Gets the position of the JWT argument in a call to this function. */
abstract int getTokenArgNum();
/** Gets the JWT argument of a call to this function. */
DataFlow::Node getTokenArg() {
this.getTokenArgNum() != -1 and result = this.getACall().getArgument(this.getTokenArgNum())
or
this.getTokenArgNum() = -1 and result = this.getACall().getReceiver()
}
}
private class DefaultSource extends Source instanceof UntrustedFlowSource { }
private class DefaultSink extends Sink {
DefaultSink() { sinkNode(this, "jwt") }
}
}

View File

@@ -0,0 +1,25 @@
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
<qhelp>
<overview>
<p>Applications decoding a JSON Web Token (JWT) may be vulnerable when the
signature is not correctly verified.</p>
</overview>
<recommendation>
<p>Always verify the signature by using the appropriate methods provided by the JWT
library, or use a library that verifies it by default.</p>
</recommendation>
<example>
<p>The following (bad) example shows a case where a JWT is parsed without verifying the
signature.</p>
<sample src="MissingJwtSignatureCheckBad.go" />
<p>The following (good) example uses the appropriate function for parsing a JWT
and verifying its signature.</p>
<sample src="MissingJwtSignatureCheckGood.go" />
</example>
<references>
<li>JWT IO: <a href="https://jwt.io/introduction">Introduction to JSON Web Tokens</a>.</li>
<li>jwt-go: <a href="https://pkg.go.dev/github.com/golang-jwt/jwt/v5">Documentation</a>.</li>
<li>Go JOSE: <a href="https://pkg.go.dev/github.com/go-jose/go-jose/v3">Documentation</a>.</li>
</references>
</qhelp>

View File

@@ -0,0 +1,21 @@
/**
* @name Missing JWT signature check
* @description Failing to check the JSON Web Token (JWT) signature may allow an attacker to forge their own tokens.
* @kind path-problem
* @problem.severity error
* @security-severity 7.8
* @precision high
* @id go/missing-jwt-signature-check
* @tags security
* external/cwe/cwe-347
*/
import go
import semmle.go.security.MissingJwtSignatureCheck
import MissingJwtSignatureCheck::Flow::PathGraph
from MissingJwtSignatureCheck::Flow::PathNode source, MissingJwtSignatureCheck::Flow::PathNode sink
where MissingJwtSignatureCheck::Flow::flowPath(source, sink)
select sink.getNode(), source, sink,
"This JWT is parsed without verification and received from $@.", source.getNode(),
"this user-controlled source"

View File

@@ -0,0 +1,21 @@
package main
import (
"fmt"
"log"
"github.com/golang-jwt/jwt/v5"
)
type User struct{}
func decodeJwt(token string) {
// BAD: JWT is only decoded without signature verification
fmt.Println("only decoding JWT")
DecodedToken, _, err := jwt.NewParser().ParseUnverified(token, &User{})
if claims, ok := DecodedToken.Claims.(*User); ok {
fmt.Printf("DecodedToken:%v\n", claims)
} else {
log.Fatal("error", err)
}
}

View File

@@ -0,0 +1,22 @@
package main
import (
"fmt"
"log"
"github.com/golang-jwt/jwt/v5"
)
type User struct{}
func parseJwt(token string, jwtKey []byte) {
// GOOD: JWT is parsed with signature verification using jwtKey
DecodedToken, err := jwt.ParseWithClaims(token, &User{}, func(token *jwt.Token) (interface{}, error) {
return jwtKey, nil
})
if claims, ok := DecodedToken.Claims.(*User); ok && DecodedToken.Valid && !err {
fmt.Printf("DecodedToken:%v\n", claims)
} else {
log.Fatal(err)
}
}

View File

@@ -0,0 +1,4 @@
---
category: newQuery
---
* The query "Missing JWT signature check" (`go/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 @am0o0](https://github.com/github/codeql/pull/14075).

View File

@@ -1,39 +0,0 @@
package main
import (
"fmt"
"log"
"github.com/golang-jwt/jwt/v5"
)
func main() {
// BAD: only decode jwt without verification
notVerifyJWT(token)
// GOOD: decode with verification or verify plus decode
notVerifyJWT(token)
VerifyJWT(token)
}
func notVerifyJWT(signedToken string) {
fmt.Println("only decoding JWT")
DecodedToken, _, err := jwt.NewParser().ParseUnverified(signedToken, &CustomerInfo{})
if claims, ok := DecodedToken.Claims.(*CustomerInfo); ok {
fmt.Printf("DecodedToken:%v\n", claims)
} else {
log.Fatal("error", err)
}
}
func LoadJwtKey(token *jwt.Token) (interface{}, error) {
return ARandomJwtKey, nil
}
func verifyJWT(signedToken string) {
fmt.Println("verifying JWT")
DecodedToken, err := jwt.ParseWithClaims(signedToken, &CustomerInfo{}, LoadJwtKey)
if claims, ok := DecodedToken.Claims.(*CustomerInfo); ok && DecodedToken.Valid {
fmt.Printf("NAME:%v ,ID:%v\n", claims.Name, claims.ID)
} else {
log.Fatal(err)
}
}

View File

@@ -1,34 +0,0 @@
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
<qhelp>
<overview>
<p>
A JSON Web Token (JWT) is used for authenticating and managing users in an application.
</p>
<p>
Only Decoding JWTs without checking if they have a valid signature or not can lead to security vulnerabilities.
</p>
</overview>
<recommendation>
<p>
Don't use methods that only decode JWT, Instead use methods that verify the signature of JWT.
</p>
</recommendation>
<example>
<p>
In the following code you can see an Example from a popular Library.
</p>
<sample src="Example.go" />
</example>
<references>
<li>
<a href="https://github.com/argoproj/argo-cd/security/advisories/GHSA-q9hr-j4rf-8fjc">JWT audience claim is not verified</a>
</li>
</references>
</qhelp>

View File

@@ -1,57 +0,0 @@
/**
* @name Use of JWT Methods that only decode user provided Token
* @description Using JWT methods without verification can cause to authorization or authentication bypass
* @kind path-problem
* @problem.severity error
* @id go/parse-jwt-without-verification
* @tags security
* experimental
* external/cwe/cwe-347
*/
import go
import experimental.frameworks.JWT
module WithValidationConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource }
predicate isSink(DataFlow::Node sink) {
sink = any(JwtParse jwtParse).getTokenArg() or
sink = any(JwtParseWithKeyFunction jwtParseWithKeyFunction).getTokenArg()
}
predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
golangJwtIsAdditionalFlowStep(nodeFrom, nodeTo)
or
goJoseIsAdditionalFlowStep(nodeFrom, nodeTo)
}
}
module NoValidationConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) {
source instanceof UntrustedFlowSource and
not WithValidation::flow(source, _)
}
predicate isSink(DataFlow::Node sink) {
sink = any(JwtUnverifiedParse parseUnverified).getTokenArg()
}
predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
golangJwtIsAdditionalFlowStep(nodeFrom, nodeTo)
or
goJoseIsAdditionalFlowStep(nodeFrom, nodeTo)
}
}
module WithValidation = TaintTracking::Global<WithValidationConfig>;
module NoValidation = TaintTracking::Global<NoValidationConfig>;
import NoValidation::PathGraph
from NoValidation::PathNode source, NoValidation::PathNode sink
where NoValidation::flowPath(source, sink)
select sink.getNode(), source, sink,
"This JWT is parsed without verification and received from $@.", source.getNode(),
"this user-controlled source"

View File

@@ -1 +0,0 @@
experimental/CWE-347/ParseJWTWithoutVerification.ql

View File

@@ -0,0 +1 @@
Security/CWE-347/MissingJwtSignatureCheck.ql

View File

@@ -39,7 +39,9 @@ module KindValidation<KindValidationConfigSig Config> {
"mongodb.sink", "nosql-injection", "unsafe-deserialization",
// Swift-only currently, but may be shared in the future
"database-store", "format-string", "hash-iteration-count", "predicate-injection",
"preferences-store", "tls-protocol-version", "transmission", "webview-fetch", "xxe"
"preferences-store", "tls-protocol-version", "transmission", "webview-fetch", "xxe",
// Go-only currently, but may be shared in the future
"jwt"
]
or
this.matches([