Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions go/ql/src/experimental/CWE-321/HardcodedKeys.qhelp
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.
Comment thread
This conversation was marked as resolved.
Outdated
</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>
18 changes: 18 additions & 0 deletions go/ql/src/experimental/CWE-321/HardcodedKeys.ql
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"
9 changes: 9 additions & 0 deletions go/ql/src/experimental/CWE-321/HardcodedKeysBad.go
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)
23 changes: 23 additions & 0 deletions go/ql/src/experimental/CWE-321/HardcodedKeysGood.go
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)
271 changes: 271 additions & 0 deletions go/ql/src/experimental/CWE-321/HardcodedKeysLib.qll
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)
)
Comment thread
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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:

  • It handles a few cases where an empty string is returned alongside an error value, but this would be better handled by looking for the pattern of an empty string "" guarded by a test if x != nil, where x is of error type.
  • The other two cases were truly unreachable code: x := "hello"; if x != "hello" { ... }, which doesn't seem like a useful test (but perhaps you could change the test to better represent a real-world situation?)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smowton

It handles a few cases where an empty string is returned alongside an error value, but this would be better handled by looking for the pattern of an empty string "" guarded by a test if x != nil, where x is of error type.

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 if x!=nil and if y !="" cases very well.

The other two cases were truly unreachable code: x := "hello"; if x != "hello" { ... }, which doesn't seem like a useful test (but perhaps you could change the test to better represent a real-world situation?)

I have added a better test case now. The test case for this sanitizer was earlier in main.go. I have moved that to sanitizer.go. Also, I have removed the test-case you mention.


/** 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(
Comment thread
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*())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar enough to the previous case, you could replace getASuccessor* with an augmented get-a-successor that includes the taint step across big.Int.Int64 (or always include that as a taint step, and use localTaint like the case below)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 AdditionalTaintStep or FunctionModel to make Int64() taint-conserving and then use a single localTaint call to track flow from rand.Int() through to an array-read that you want to sanitize. Similarly you could make % modulus operations taint-conserving so they wouldn't have to be sanitized.

One other related question: it looks like in your sanitizers you sanitize the input to constant[random], whereas logically you should probably sanitize the result instead.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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
Comment thread
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)
Comment thread
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
}
}
}
Loading