Repository navigation
Golang : Add query to detect JWT signing vulnerabilities #9378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
bd1ddc1
e0f74a5
ae2bc1b
1ef42a1
361b703
bfbc1d4
3155771
b48a07e
602495d
e54b29a
d5ac719
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| <!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> | ||
| Using a hard-coded secret key for signing JWT tokens in open source projects | ||
| can leave the application using the token vulnerable to authentication bypasses. | ||
| </p> | ||
|
|
||
| <p> | ||
| A JWT token is safe for enforcing authentication and access control as long as it can't be forged by a malicious actor. However, when a project exposes this secret publicly, these seemingly unforgeable tokens can now be easily forged. | ||
| Since the authentication as well as access control is typically enforced through these JWT tokens, an attacker armed with the secret can create a valid authentication token for any user and may even gain access to other privileged parts of the application. | ||
| </p> | ||
|
|
||
| </overview> | ||
| <recommendation> | ||
|
|
||
| <p> | ||
| Generating a crytograhically secure secret key during application initialization and using this generated key for future JWT signing requests can prevent this vulnerability. | ||
| </p> | ||
|
|
||
| </recommendation> | ||
| <example> | ||
|
|
||
| <p> | ||
| The following code uses a hard-coded string as a secret for signing the tokens. In this case, an attacker can very easily forge a token by using the hard-coded secret. | ||
| </p> | ||
|
|
||
| <sample src="HardcodedKeysBad.go" /> | ||
|
|
||
| </example> | ||
| <example> | ||
|
|
||
| <p> | ||
| In the following case, the application uses a programatically generated string as a secret for signing the tokens. In this case, since the secret can't be predicted, the code is secure. A function like `GenerateCryptoString` can be run to generate a secure secret key at the time of application installation/initialization. This generated key can then be used for all future signing requests. | ||
| </p> | ||
|
|
||
| <sample src="HardcodedKeysGood.go" /> | ||
|
|
||
| </example> | ||
| <references> | ||
| <li> | ||
| CVE-2022-0664: | ||
| <a href="https://nvd.nist.gov/vuln/detail/CVE-2022-0664">Use of Hard-coded Cryptographic Key in Go github.com/gravitl/netmaker prior to 0.8.5,0.9.4,0.10.0,0.10.1. </a> | ||
| </li> | ||
| </references> | ||
|
|
||
| </qhelp> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| /** | ||
| * @name Use of a hardcoded key for signing JWT | ||
| * @description Using a fixed hardcoded key for signing JWT's can allow an attacker to compromise security. | ||
| * @kind path-problem | ||
| * @problem.severity error | ||
| * @id go/hardcoded-key | ||
| * @tags security | ||
| * external/cwe/cwe-321 | ||
| */ | ||
|
|
||
| import go | ||
| import HardcodedKeysLib | ||
| import DataFlow::PathGraph | ||
|
|
||
| from HardcodedKeys::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink | ||
| where cfg.hasFlowPath(source, sink) | ||
| select sink.getNode(), source, sink, "$@ is used to sign a JWT token.", source.getNode(), | ||
| "Hardcoded String" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| mySigningKey := []byte("AllYourBase") | ||
|
|
||
| claims := &jwt.RegisteredClaims{ | ||
| ExpiresAt: jwt.NewNumericDate(time.Unix(1516239022, 0)), | ||
| Issuer: "test", | ||
| } | ||
|
|
||
| token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) | ||
| ss, err := token.SignedString(mySigningKey) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| func GenerateCryptoString(n int) (string, error) { | ||
| const chars = "123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-" | ||
| ret := make([]byte, n) | ||
| for i := range ret { | ||
| num, err := crand.Int(crand.Reader, big.NewInt(int64(len(chars)))) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| ret[i] = chars[num.Int64()] | ||
| } | ||
| return string(ret), nil | ||
| } | ||
|
|
||
| mySigningKey := GenerateCryptoString(64) | ||
|
|
||
|
|
||
| claims := &jwt.RegisteredClaims{ | ||
| ExpiresAt: jwt.NewNumericDate(time.Unix(1516239022, 0)), | ||
| Issuer: "test", | ||
| } | ||
|
|
||
| token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) | ||
| ss, err := token.SignedString(mySigningKey) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,271 @@ | ||
| /** | ||
| * Provides default sources, sinks and sanitizers for reasoning about | ||
| * JWT token signing vulnerabilities as well as extension points | ||
| * for adding your own. | ||
| */ | ||
|
|
||
| import go | ||
| import StringOps | ||
| import DataFlow::PathGraph | ||
|
|
||
| /** | ||
| * Provides default sources, sinks and sanitizers for reasoning about | ||
| * JWT token signing vulnerabilities as well as extension points | ||
| * for adding your own. | ||
| */ | ||
| module HardcodedKeys { | ||
| /** | ||
| * A data flow source for JWT token signing vulnerabilities. | ||
| */ | ||
| abstract class Source extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A data flow sink for JWT token signing vulnerabilities. | ||
| */ | ||
| abstract class Sink extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A sanitizer for JWT token signing vulnerabilities. | ||
| */ | ||
| abstract class Sanitizer extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A sanitizer guard for JWT token signing vulnerabilities. | ||
| */ | ||
| abstract class SanitizerGuard extends DataFlow::BarrierGuard { } | ||
|
|
||
| private predicate isTestCode(Expr e) { | ||
| e.getFile().getAbsolutePath().toLowerCase().matches("%test%") and | ||
| not e.getFile().getAbsolutePath().toLowerCase().matches("%ql/test%") | ||
| } | ||
|
|
||
| private predicate isDemoCode(Expr e) { | ||
| e.getFile().getAbsolutePath().toLowerCase().matches(["%mock%", "%demo%", "%example%"]) | ||
| } | ||
|
|
||
| /** | ||
| * A hardcoded string literal as a source for JWT token signing vulnerabilities. | ||
| */ | ||
| class HardcodedStringSource extends Source { | ||
| HardcodedStringSource() { | ||
| this.asExpr() instanceof StringLit and | ||
| not (isTestCode(this.asExpr()) or isDemoCode(this.asExpr())) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * An expression used to sign JWT tokens as a sink for JWT token signing vulnerabilities. | ||
| */ | ||
| private class GolangJwtSign extends Sink { | ||
| GolangJwtSign() { | ||
| exists(string pkg | | ||
| pkg = | ||
| [ | ||
| "github.com/golang-jwt/jwt/v4", "github.com/dgrijalva/jwt-go", | ||
| "github.com/form3tech-oss/jwt-go", "github.com/ory/fosite/token/jwt" | ||
| ] | ||
| | | ||
| ( | ||
| exists(DataFlow::MethodCallNode m | | ||
| // Models the `SignedString` method | ||
| // `func (t *Token) SignedString(key interface{}) (string, error)` | ||
| m.getTarget().hasQualifiedName(pkg, "Token", "SignedString") | ||
| | | ||
| this = m.getArgument(0) | ||
| ) | ||
| or | ||
| exists(DataFlow::MethodCallNode m | | ||
| // Model the `Sign` method of the `SigningMethod` interface | ||
| // type SigningMethod interface { | ||
| // Verify(signingString, signature string, key interface{}) error | ||
| // Sign(signingString string, key interface{}) (string, error) | ||
| // Alg() string | ||
| // } | ||
| m.getTarget().hasQualifiedName(pkg, "SigningMethod", "Sign") | ||
| | | ||
| this = m.getArgument(1) | ||
| ) | ||
|
This conversation was marked as resolved.
Outdated
|
||
| ) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| private class GinJwtSign extends Sink { | ||
| GinJwtSign() { | ||
| exists(Field f | | ||
| // https://pkg.go.dev/github.com/appleboy/gin-jwt/v2#GinJWTMiddleware | ||
| f.hasQualifiedName("github.com/appleboy/gin-jwt/v2", "GinJWTMiddleware", "Key") and | ||
| f.getAWrite().getRhs() = this | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| private class SquareJoseKey extends Sink { | ||
| SquareJoseKey() { | ||
| exists(Field f, string pkg | | ||
| // type Recipient struct { | ||
| // Algorithm KeyAlgorithm | ||
| // Key interface{} | ||
| // KeyID string | ||
| // PBES2Count int | ||
| // PBES2Salt []byte | ||
| // } | ||
| // type SigningKey struct { | ||
| // Algorithm SignatureAlgorithm | ||
| // Key interface{} | ||
| // } | ||
| f.hasQualifiedName(pkg, ["Recipient", "SigningKey"], "Key") and | ||
| f.getAWrite().getRhs() = this | ||
| | | ||
| pkg = ["github.com/square/go-jose/v3", "gopkg.in/square/go-jose.v2"] | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| private class CrystalHqJwtSigner extends Sink { | ||
| CrystalHqJwtSigner() { | ||
| exists(DataFlow::CallNode m | | ||
| // `func NewSignerHS(alg Algorithm, key []byte) (Signer, error)` | ||
| m.getTarget().hasQualifiedName("github.com/cristalhq/jwt/v3", "NewSignerHS") | ||
| | | ||
| this = m.getArgument(1) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| private class GoKitJwt extends Sink { | ||
| GoKitJwt() { | ||
| exists(DataFlow::CallNode m | | ||
| // `func NewSigner(kid string, key []byte, method jwt.SigningMethod, claims jwt.Claims) endpoint.Middleware` | ||
| m.getTarget().hasQualifiedName("github.com/go-kit/kit/auth/jwt", "NewSigner") | ||
| | | ||
| this = m.getArgument(1) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| private class LestrratJwk extends Sink { | ||
| LestrratJwk() { | ||
| exists(DataFlow::CallNode m, string pkg | | ||
| pkg.matches([ | ||
| "github.com/lestrrat-go/jwx", "github.com/lestrrat/go-jwx/jwk", | ||
| "github.com/lestrrat-go/jwx%/jwk" | ||
| ]) and | ||
| // `func New(key interface{}) (Key, error)` | ||
| m.getTarget().hasQualifiedName(pkg, "New") | ||
| | | ||
| this = m.getArgument(0) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Mark any comparision expression where any operand is tainted as a | ||
| * sanitizer for all instances of the taint | ||
| */ | ||
| private class CompareExprSanitizer extends Sanitizer { | ||
| CompareExprSanitizer() { | ||
| exists(BinaryExpr c | | ||
| c.getAnOperand().getGlobalValueNumber() = this.asExpr().getGlobalValueNumber() | ||
| ) | ||
| } | ||
| } | ||
|
Comment on lines
+160
to
+171
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried removing this sanitizer, and it doesn't seem to be useful:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This may not be ideal. There could be multiple instances where a error code may be returned without a test to nil, say for ex: func t()(string, error){
if config.hasKey(){
key:= config.getKey()
return key,nil
} else {
return getDefaultKey(), errors.New("no key")
}To avoid this, I just check for cases where there the taint is compared with anything and mark that as sanitized. This handles the
I have added a better test case now. The test case for this sanitizer was earlier in |
||
|
|
||
| /** Mark an empty string returned with an error as a sanitizer */ | ||
| class EmptyErrorSanitizer extends Sanitizer { | ||
| EmptyErrorSanitizer() { | ||
| exists(ReturnStmt r, DataFlow::CallNode c | | ||
| c.getTarget().hasQualifiedName("errors", "New") and | ||
| r.getNumChild() > 1 and | ||
| r.getAChild() = c.getAResult().getASuccessor*().asExpr() and | ||
| r.getAChild() = this.asExpr() | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** Mark any formatting string call as a sanitizer */ | ||
| class FormattingSanitizer extends Sanitizer { | ||
| FormattingSanitizer() { exists(Formatting::StringFormatCall s | s.getAResult() = this) } | ||
| } | ||
|
|
||
| /** | ||
| * Mark any taint arising from a read on a tainted slice with a random index as a | ||
| * sanitizer for all instances of the taint | ||
| */ | ||
| private class RandSliceSanitizer extends Sanitizer { | ||
| RandSliceSanitizer() { | ||
| exists(DataFlow::CallNode randint, string name, DataFlow::ElementReadNode r | | ||
| ( | ||
| randint.getTarget().hasQualifiedName("math/rand", name) or | ||
| randint.getTarget().(Method).hasQualifiedName("math/rand", "Rand", name) | ||
| ) and | ||
| name = | ||
| [ | ||
| "ExpFloat64", "Float32", "Float64", "Int", "Int31", "Int31n", "Int63", "Int63n", "Intn", | ||
| "NormFloat64", "Uint32", "Uint64" | ||
| ] and | ||
| r.reads(this, randint.getAResult().getASuccessor*()) | ||
| ) | ||
| or | ||
| // Sanitize flows like this: | ||
| // func GenerateCryptoString(n int) (string, error) { | ||
| // const chars = "123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-" | ||
| // ret := make([]byte, n) | ||
| // for i := range ret { | ||
| // num, err := crand.Int(crand.Reader, big.NewInt(int64(len(chars)))) | ||
| // if err != nil { | ||
| // return "", err | ||
| // } | ||
| // ret[i] = chars[num.Int64()] | ||
| // } | ||
| // return string(ret), nil | ||
| // } | ||
| exists( | ||
|
This conversation was marked as resolved.
Outdated
|
||
| DataFlow::CallNode randint, DataFlow::MethodCallNode bigint, DataFlow::ElementReadNode r | ||
| | | ||
| randint.getTarget().hasQualifiedName("crypto/rand", "Int") and | ||
| bigint.getTarget().hasQualifiedName("math/big", "Int", "Int64") and | ||
| bigint.getReceiver() = randint.getResult(0).getASuccessor*() and | ||
| r.reads(this, bigint.getAResult().getASuccessor*()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is similar enough to the previous case, you could replace
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure I follow. Do you mean to say I should create a new taint tracking config and add this step there?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I mean instead of using getASuccessor* aka localFlow to track flow from rand.Int() etc to Int.Int64() and then using it again to track from Int64() to an array-read, you could extend One other related question: it looks like in your sanitizers you sanitize the input to The ideal version would look something like using source = post-update-node of rand.Read(_) or result of rand.Int() etc, and additional taint steps = big.Int functions and modulus operations, sanitize track taint forwards to any index operand and sanitize that array-read's result.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @smowton I think I partially understand what you wish to say. I have made some changes did you mean something on these lines? |
||
| ) | ||
| or | ||
| // Sanitize flows like : | ||
| // func GenerateRandomString(size int) string { | ||
| // var bytes = make([]byte, size) | ||
| // rand.Read(bytes) | ||
| // for i, x := range bytes { | ||
| // bytes[i] = characters[x%byte(len(characters))] | ||
| // } | ||
| // return string(bytes) | ||
| // } | ||
| exists(DataFlow::CallNode randread, DataFlow::Node rand, DataFlow::ElementReadNode r | | ||
| randread.getTarget().hasQualifiedName("crypto/rand", "Read") and | ||
| TaintTracking::localTaint(randread.getArgument(0).getAPredecessor*().getASuccessor*(), rand) and | ||
|
This conversation was marked as resolved.
Outdated
|
||
| ( | ||
| exists(ModExpr e | e.getAnOperand() = rand.asExpr() | | ||
| r.reads(this, e.getGlobalValueNumber().getANode()) | ||
| ) | ||
| or | ||
| r.reads(this.getAPredecessor*(), rand) | ||
|
This conversation was marked as resolved.
Outdated
|
||
| ) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A configuration depicting taint flow for studying JWT token signing vulnerabilities. | ||
| */ | ||
| class Configuration extends TaintTracking::Configuration { | ||
| Configuration() { this = "Hard-coded JWT Signing Key" } | ||
|
|
||
| override predicate isSource(DataFlow::Node source) { source instanceof Source } | ||
|
|
||
| override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } | ||
|
|
||
| override predicate isSanitizer(DataFlow::Node sanitizer) { sanitizer instanceof Sanitizer } | ||
|
|
||
| override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { | ||
| guard instanceof SanitizerGuard | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.