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

upstream fix for security vulnerability from form3tech-oss/jwt-go fork #40

Merged
merged 1 commit into from Jul 30, 2021

Conversation

thaJeztah
Copy link
Contributor

This forwards the changes of form3tech-oss/jwt-go#14 to the upstream repository.

I am not the author of the original PR (nor do I know much about JWT), but thought I'd give this an attempt, following the discussion in Azure/go-autorest#642 (and issues linked from there).

Fixes a security vulnerability where a jwt token could potentially be validated having invalid string characters.

(cherry picked from commit a211650c6ae1cff6d7347d3e24070d65dcfb1122)
form3tech-oss/jwt-go#14

@thaJeztah
Copy link
Contributor Author

thaJeztah commented Jul 29, 2021

/cc @dims @brackendawson

This is just a quick cherry-pick; there were some conflicts though, so if someone with more knowledge could have a close look (or wants to carry this), let me know

edit: conflicts were in a comment, and in the test (both easy to resolve), but more eyes won't hurt 😅

oxisto
oxisto previously approved these changes Jul 29, 2021
Copy link
Collaborator

@oxisto oxisto left a comment

Choose a reason for hiding this comment

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

LGTM in general. Thanks for porting this over from the other fork.

Just some smaller things that should be corrected.

map_claims.go Outdated Show resolved Hide resolved
map_claims_test.go Outdated Show resolved Hide resolved
map_claims_test.go Outdated Show resolved Hide resolved
map_claims_test.go Outdated Show resolved Hide resolved
Fixes a security vulnerability where a jwt token could potentially be validated having invalid string characters.

(cherry picked from commit a211650c6ae1cff6d7347d3e24070d65dcfb1122)
form3tech-oss/jwt-go#14

Co-Authored-By: Giorgos Lampadakis <82932062+giorgos-f3@users.noreply.github.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah changed the title [RFC] upstream fix for security vulnerability from form3tech-oss/jwt-go fork upstream fix for security vulnerability from form3tech-oss/jwt-go fork Jul 29, 2021
@thaJeztah
Copy link
Contributor Author

@oxisto thanks for reviewing; I think I addressed your comments; PTAL

@oxisto oxisto mentioned this pull request Jul 30, 2021
2 tasks
Copy link
Collaborator

@oxisto oxisto left a comment

Choose a reason for hiding this comment

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

LGTM. I will tacke the test consolidation in a future issue, I need to rework the TestVerifyAud method first.

@dims
Copy link

dims commented Jul 30, 2021

LGTM ( non-binding of course! )

@mfridman
Copy link
Member

Based on a quick glance over distribution/distribution#3361, wasn't that issue addressed in #12

I don't see how this PR fixes that.

My only concern is the implementation is a breaking change, we're modifying the behaviour of what happens when the field is set but is not the correct type. I do agree with this implementation though.

@thaJeztah
Copy link
Contributor Author

Based on a quick glance over distribution/distribution#3361, wasn't that issue addressed in #12

I don't see how this PR fixes that.

Correct, #12 is to address CVE-2020-26160. This patch is unrelated, but was in the form3tech-oss/jwt-go, and marked as a security issue. I doubt a CVE has been requested for this one. (neither am I familiar enough with this to determine what the impact is and/or if it's indeed a security issue 😅). Also see my comment on distribution/distribution#3466 (comment)

@thaJeztah
Copy link
Contributor Author

Basically, my goal here was trying to prevent users of the https://github.com/dgrijalva/jwt-go package having to decide between either using the "official" maintained fork and (potentially) miss out on a security fix, or an "unofficial fork" with a security fix. 🤷‍♂️

@mfridman
Copy link
Member

Ye that's fair. Thanks for submitting this PR. We'll call this out in the changelog release.

@oxisto oxisto merged commit 8e9d9eb into golang-jwt:main Jul 30, 2021
@dims
Copy link

dims commented Jul 30, 2021

@mfridman @oxisto could we persuade you to tag a release as well with this change? thanks a ton!

@oxisto
Copy link
Collaborator

oxisto commented Jul 30, 2021

@mfridman @oxisto could we persuade you to tag a release as well with this change? thanks a ton!

Already preparing it :) See #42

@thaJeztah thaJeztah deleted the forward_port_security_fix branch July 30, 2021 20:43
@thaJeztah
Copy link
Contributor Author

Thanks everyone!

If I find some time, I'll write up a ticket in the other fork, to see if maintainers of that fork are ok with abandoning the fork in favor of this one (possibly upstream other changes they have)

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 this pull request may close these issues.

None yet

5 participants