search

duo-labs / webauthn

WebAuthn (FIDO2) server library written in Go
https://webauthn.io/
BSD 3-Clause "New" or "Revised" License
1.03k stars 162 forks source link

Securty Update for CVEs in JWT lib #101

Closed HaBaLeS closed 2 years ago

HaBaLeS commented 3 years ago

Move form JWT: https://github.com/dgrijalva/jwt-go to Community maintained clone https://github.com/dgrijalva/jwt-go for CVE's reported by Dependabot

uberswe commented 3 years ago

I don't think you intended to change go modules to your forked repo in this PR @HaBaLeS ? https://github.com/duo-labs/webauthn/pull/101/commits/9058f7ce2de0e9e271a0a1c8440f1561e9053c7f

HaBaLeS commented 3 years ago

I don't think you intended to change go modules to your forked repo in this PR @HaBaLeS ? 9058f7c

Ups ... yes you are right. Reminds myself why not to force-push :-/

nklaassen commented 3 years ago

Move form JWT: https://github.com/dgrijalva/jwt-go to Community maintained clone https://github.com/dgrijalva/jwt-go for CVE's reported by Dependabot

Typo here, I think you mean that this PR switches from https://github.com/dgrijalva/jwt-go to the community maintained https://github.com/golang-jwt/jwt. Would like to see this merged so that we can continue to use this library without forking

oschwald commented 2 years ago

It would be great to see this merged so that we don't have to switch to a fork.

james-d-elliott commented 2 years ago

It would be great to see this merged so that we don't have to switch to a fork.

For this particular fix you wouldn't need to. A replace on the affected downstream library will do it. Just add this to your go.mod:

replace github.com/dgrijalva/jwt-go => github.com/golang-jwt/jwt/v4 v4.2.0

oschwald commented 2 years ago

@james-d-elliott, thanks for mentioning that. I hadn't looked closely enough at the PR to realize that golang-jwt was completely compatible with jwt-go.

james-d-elliott commented 2 years ago

Yep github.com/golang-jwt/jwt is the "official" community fork of the original and the original is officially unmtaintained. See https://github.com/dgrijalva/jwt-go/issues/462 for more info. The main difference between v3 and v4 is that v3 didn't properly setup the module path (hence the +incompatible suffix); as well as the CVE fix.

Also just as an educated guess, since the JWT lib is only used for MDS, I don't believe the CVE actually affects this library directly.

MasterKale commented 2 years ago

I just got write access to this repo so I can at least merge PRs. I am not in a position to actually review code beyond a PR like this (I'm just not knowledgeable in Golang) so I'll be relying on feedback from @nicksteele and other maintainers to gauge which PR's are suitable to merge.

That said this PR seems fine to me. I'm considering merging this and closing #95 since this PR updates to a more recent version of golang-jwt/jwt. Any objections?

james-d-elliott commented 2 years ago

Yeah that seems like the best course of action in my opinion. For reference I'd rate #93 as the most important fix outstanding by the way, and it's a really small change that Nick should be able to review relatively quickly.

The change in this PR updates a library for a CVE that actually doesn't affect this repos usage of the library, at least as far as I can tell. See here for the CVE description: https://nvd.nist.gov/vuln/detail/CVE-2020-26160

But your usage is actually for parsing MDS data, which I don't believe uses audience and would generally not need validation (of the audience) since you're validating the signature and only using the information for metadata, not authorization/access.