Fix: High Severity Security Vulnerability - Access Restriction Bypass

[torrayne]

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

[codecov]

Codecov Report

Merging #1663 into master will decrease coverage by 1.50%.
The diff coverage is n/a.

@@            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              

Impacted Files	Coverage Δ
middleware/method_override.go	80.95% <0.00%> (-3.67%)	⬇️
context.go	86.38% <0.00%> (-3.34%)	⬇️
middleware/redirect.go	85.00% <0.00%> (-3.24%)	⬇️
middleware/proxy.go	63.21% <0.00%> (-3.12%)	⬇️
middleware/basic_auth.go	65.62% <0.00%> (-2.95%)	⬇️
echo.go	83.58% <0.00%> (-2.36%)	⬇️
middleware/util.go	88.88% <0.00%> (-1.74%)	⬇️
response.go	85.18% <0.00%> (-1.66%)	⬇️
middleware/cors.go	77.94% <0.00%> (-1.52%)	⬇️
middleware/key_auth.go	66.66% <0.00%> (-1.13%)	⬇️
... and 18 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 151ed6b...d0c3a50. Read the comment docs.

[pafuent]

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)

If you're referring to the fork at github.com/form3tech-oss/jwt-go, there's a bug in the code that doesn't properly handle the audience claim. The RFC specifies the AUD claim can either be a string or an array of strings.
The code in the fork assumes it's an array of string and any AUD claim with a string (e.g. Azure AD id tokens) will cause a panic.

BTW, Why you did a replace instead of directly changing the dependency?

[torrayne]

I thought that since it isn't the new "official" repo we might want to keep a reference to the original.

Go Jose handles the audience claim properly. You can take a look at the claims here. Maybe they would be a better fit.

[lammel]

It is quite unsure what the actual "official" repository for jwt-go is actually.
For now we can only assume that the orignal project dgrijalva/jwt-go is unmaintained. Not sure if the proclaimed successor form3tech-oss/jwt-go can establish itself.

The current maintained repo for go-jose seems to be go-jose/go-jose.
The disclaimer for US export limitations needs to be considered, as it would limit echo in the same way.

But I guess we need to consider moving away from dgrijalva/jwt-go anyway.
Discussion and comments appreciated.

@vishr Any comments from your side?

[torrayne]

If I'm understanding their documentation correctly, /square/go-jose is version 2 and /go-jose/go-jose is version 3.

Versions

Version 2 (branch, doc) is the current stable version:

import "gopkg.in/square/go-jose.v2"

Version 3 (branch, doc) is the under development/unstable version (not released yet):

import "github.com/go-jose/go-jose/v3"

Other libraries

jwt.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.

• https://github.com/gbrlsnchs/jwt
• https://github.com/lestrrat-go/jwx
• https://github.com/pascaldekloe/jwt
• https://github.com/cristalhq/jwt

[lammel]

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.

[aldas]

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.

[torrayne]

Honestly I kinda like this idea

[aldas]

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 "github.com/dgrijalva/jwt-go" library and provide small snippet with that wrapper in cookbook. This way main jwt middleware function implementation will be in Echo codebase and maintained but user has control over actual JWT implementation library.

[lammel]

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.

[aldas]

it is possible to move dependency to "github.com/dgrijalva/jwt-go" to being to test dependency. So it is only referenced in test code as default token parser interface implementation and therefore not being direct dependency anymore (see golang/go#26913 (comment) )

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 Parse(tokenString string, config JWTConfig) (interface{}, error) function that must be set for middleware

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

[ascotan]

So a few notes here:

1. This issue has a high severity CVE filed against it in the upstream library. I recommend that a point release be issued to address this issue alone as this is not just a feature/bug.
2. It appears that v4.0.0-preview1 doesn't actually fix the CVE vulnerability. There is a fork that should be used as a temporary workaround: https://github.com/form3tech-oss/jwt-go/releases/tag/v3.2.1

References: dgrijalva/jwt-go#428

[aldas]

Guys has anyone checked that issue?

The problem is that aud field in claim is used as string in vulnerable version and more secure way would be to use []string. So if you create secure JWT token with []string as aud and then parse it with insecure version you would get ""

dgrijalva/jwt-go#422 (comment)

Now regarding to Echo - JWT middleware does not set or use aud field. StandardClaims.VerifyAudience is standalone method and it is up to client to implement this check.

So this is problem when:

• JWT tokens are created aud containing multiple urls ([]string instead of string)
  • those tokens are probably created externally as jwt.StandardClaims assumes Audience to be string not []string
  • as token is verified agains signing key this would mean that token needs to be originated from authoritative source (key)
• Echo user explicitly checks (codes thoses check by own) claims after JWT middleware with StandardClaims.VerifyAudience(cmp string, req bool) bool
• aud is check set to optional (by application developer) by StandardClaims.VerifyAudience("https://mysite.com", false) call thus making

    if aud == "" {
    	return !required
    }

  return wrongly true and not checking if aud actually matches to token audiences values

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 aud)

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.

[pedromss]

Just a heads up, this will still fail security checks of Snyk. Looking at the output of go list -m all the replacement seems to be recognized by go mod, but Snyk still reports a vulnerability.

I suspect Snyk isn't aware of the replace directive, but its a guess.

[aldas]

I think this is not a 'real' security issue - to exploit this you need:

• token to be valid (from authorative source) - means attacked has composed key or uses token meant for other site/application that is signed with same key
• token to be with aud value of array instead of single value (string)
• you have written EXPLICIT go code for validating claims that says that aud claim check is optional if ok := StandardClaims.VerifyAudience("https://mysite.com", false)

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) -

IF token exists
THEN  
   RETURN checkToken(token)
ELSE 
  RETURN TRUE  // here we are with optionally existing tokens

[stale]

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.

[aldas]

done in #1946