Fix: High Severity Security Vulnerability - Access Restriction Bypass #1663
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1663 +/- ##
==========================================
- Coverage 85.28% 83.78% -1.51%
==========================================
Files 28 28
Lines 2216 1906 -310
==========================================
- Hits 1890 1597 -293
+ Misses 212 195 -17
Partials 114 114
Continue to review full report at Codecov.
|
|
I'm not already familiar with the code that fixes this on form3tech-oss's fork but check this comment on an Issue on go-kit go-kit/kit#1026 (comment)
BTW, Why you did a replace instead of directly changing the dependency? |
|
It is quite unsure what the actual "official" repository for jwt-go is actually. The current maintained repo for go-jose seems to be go-jose/go-jose. But I guess we need to consider moving away from dgrijalva/jwt-go anyway. @vishr Any comments from your side? |
|
If I'm understanding their documentation correctly, VersionsVersion 2 (branch, doc) is the current stable version: Version 3 (branch, doc) is the under development/unstable version (not released yet): Other librariesjwt.io has a filterable list of libraries that shows their supports. The list below contains libraries that look maintained and pass all the claims checks but don't necessarily support all encryption methods. |
|
There is also PR #1713 now, using the 4.0-preview1 release from jwt-go. Using go-jwt v4 (preview1) is also an option, but this branch is not marked stable yet and no development activity is seen. |
|
just an heretic idea - move jwt middleware to recipe/cookbook as an example. This would remove that dependency from Echo and some of the maintenance burden is moved to user side. |
|
Honestly I kinda like this idea |
|
well, after some thinking ... this would bean that users will not get updates to middleware. Another idea - create wrapper interface for jwt middleware that wraps dependency to |
|
I think to by us some time by merging PR #1713 with jwt-go v4.0.0-preview1 and having a fix for our current echo v4. The discussion here already has some good ideas on how to proceeed so I'd like to tag this PR v5 for a sane solution with the next major release. Moving the JWT middleware to echo-contrib has it's up´pros and cons. Personally I prefer to a have a simple but usuable authentication middleware for widely used auth mechanisms in the core of echo. This will help to ensure it is documented and well tested. |
|
it is possible to move dependency to in middleware code there is interface // JwtTokenParser is wrapper interface for different JWT token parsing implementations.
type JwtTokenParser interface {
// Parse parses token string to token instance that is set to echo.Context under JWTConfig.ContextKey
// Must return error when parsing failed, token is not valid or otherwise incorrect
Parse(tokenString string, config JWTConfig) (interface{}, error)
}and shortest implementation that I could get is situated in tests - making it test dependecy and this would be what is refenced in cookbook to be copy pasted by user into their code. it could be even shorter if default claims stuff is dropped and instead of interface/struct there is only import "github.com/dgrijalva/jwt-go"
...
...
...
type DgrijalvaJwtGoParser struct {
// DefaultClaimsFunc returns new claims instance for parsed token. This instance is used as destination when
// marshalling json to claims. Use our own custom claims implementation here.
// Defaults to jwt.MapClaims when not set
// NB: ALWAYS return new instance!!! or requests (goroutines) we would see panics runtime
DefaultClaimsFunc func() jwt.Claims
}
func (p *DgrijalvaJwtGoParser) Parse(tokenString string, config JWTConfig) (interface{}, error) {
var claims jwt.Claims = jwt.MapClaims{}
if p.DefaultClaimsFunc != nil {
claims = p.DefaultClaimsFunc()
}
token, err := new(jwt.Parser).ParseWithClaims(tokenString, claims, p.keyFunc(config))
if err != nil {
return nil, err
}
if !token.Valid {
return nil, errors.New("token is not valid")
}
return token, nil
}
func (p *DgrijalvaJwtGoParser) keyFunc(config JWTConfig) jwt.Keyfunc {
return func(t *jwt.Token) (interface{}, error) {
kid, _ := t.Header["kid"]
return config.KeyFunc(t.Method.Alg(), kid)
}
}see all changes to middleware aldas@2f2e986 |
|
So a few notes here:
References: dgrijalva/jwt-go#428 |
|
Guys has anyone checked that issue? The problem is that dgrijalva/jwt-go#422 (comment) Now regarding to Echo - JWT middleware does not set or use So this is problem when:
I would say IT is a problem only when you are reusing token signing keys for multiple sites/applications (even for sites that you are not meaning to use - by aud values) or attacker has compromised your key and uses it on rogue application/site and has coded its stuff to check aud. So easiest workaround till fix is - make sure that you are not reusing signing keys (for applications that have different audience one thing - If you have set aud checking optional and token is from authoritative source (signed with trusted key) is failure to check token aud value matches even an error - because you have made a rule that aud can be empty. |
|
Just a heads up, this will still fail security checks of Snyk. Looking at the output of I suspect Snyk isn't aware of the |
|
I think this is not a 'real' security issue - to exploit this you need:
So, even if this aud check is fixed it would not make security issue go away - because your AUD check is still set to be optional and that means that token without aud is still valid token with valid claims. a over the top example: it is almost as writing following rule for token validation (instead of claims check) - |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed within a month if no further activity occurs. Thank you for your contributions. |
|
done in #1946 |
dgrijalva/jwt-go has an high security issue #428. dgrijalva hasn't updated the repository for a while and there is talk of moving. But for now it seems most people are using form3tech-oss's fork.
References issue #1647