Skip to content
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

Fix security vulnerability (CVE-2020-26160) #6

Closed
mfridman opened this issue May 27, 2021 · 6 comments · Fixed by #12
Closed

Fix security vulnerability (CVE-2020-26160) #6

mfridman opened this issue May 27, 2021 · 6 comments · Fixed by #12
Assignees
Labels
Milestone

Comments

@mfridman
Copy link
Member

More detailed explanation here: dgrijalva/jwt-go#428

How should we approach a fix?

There is a v4.0.0-preview1 version in dgrijalva/jwt-go that apparently fixes this, and a bunch of PRs.

There was a fix in this fork, https://github.com/form3tech-oss/jwt-go by @Waterdrips and co. as well.

@oxisto oxisto changed the title Fix security vulnerability Fix security vulnerability (CVE-2020-26160) May 27, 2021
@lggomez
Copy link
Member

lggomez commented May 27, 2021

We can either import one of the PRs or replicate it it ourselves, if you want I can look into it. The fix is in the claims implementation and how it handles string slices instead of single strings

@oxisto
Copy link
Collaborator

oxisto commented May 27, 2021

The patch of @Waterdrips as seen here looks good to me: https://patch-diff.githubusercontent.com/raw/form3tech-oss/jwt-go/pull/3.patch

Just need to remove the changes in .gitignore, .travis.yml and the first change in example_test.go

@lggomez
Copy link
Member

lggomez commented May 27, 2021

It passed a long time since I checked the v4 branch out of curiosity but it contained a lot of changes if I remember correctly, both on the API and implementation levels. We'd need to take some time to take a look at it and decide how to proceed with that (because that will also affect how our branching model will be with respect to v3)

@mfridman
Copy link
Member Author

It passed a long time since I checked the v4 branch out of curiosity but it contained a lot of changes if I remember correctly, both on the API and implementation levels

Same here. iirc there were also breaking changes as described in the README of the release_4_0_0 branch

NEW VERSION: Version 4 of this library is now available. This is the first non-backward-compatible version in a long time. There are a few changes that all users will notice, such as the new types introduced in members of StandardClaims. More changes are additive or only impact more advanced use. See VERSION_HISTORY.md for a list of changes as well as TODO MIGRATION_GUIDE.md for help updating your code.

So maybe instead of branching from the v4 preview branch, we could aim to work with the v3 in a backwards-compatible way?

Being able to replace the existing repo with a patched, module-aware, backwards compatible repo would make migrating a straightforward process for users. Either through the replace directive or a search/replace of import paths.

@lggomez
Copy link
Member

lggomez commented May 27, 2021

Being able to replace the existing repo with a patched, module-aware, backwards compatible repo would make migrating a straightforward process for users. Either through the replace directive or a search/replace of import paths.

+1 totally, we want to be the most user friendly possible here considering we are already inducing a dependency change acting as an update

@Waterdrips
Copy link
Member

I can cover the CVE Fix, there was a little more nuance to it than my initial patch which needed following up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants