Re: golang-github-dgrijalva-jwt-go / CVE-2020-26160
Hi Brian,
On Wed, Dec 02, 2020 at 09:01:21AM +1100, Brian May wrote:
> Salvatore Bonaccorso <carnil@debian.org> writes:
>
> > Hi Brian,
> >
> > On Tue, Dec 01, 2020 at 09:01:37AM +1100, Brian May wrote:
> >> I note this package - golang-github-dgrijalva-jwt-go - has been marked
> >> as vulnerable to CVE-2020-26160 in both Debian stretch and buster.
> >>
> >> https://security-tracker.debian.org/tracker/CVE-2020-26160
> >>
> >> But I can't find any code in these versions that even mentions the
> >> aud/audience fields.
> >>
> >> So I plan to mark these versions as not vulnerable.
> >
> > Were you able to track down in which version the vulnerability was
> > introduced?
>
> If I am reading this correctly, the broken code was introduced here:
>
> https://github.com/dgrijalva/jwt-go/commit/44718f8a89b030a85860eef946090aac75faffac
So this would be in
$ git describe --contains 44718f8a89b030a85860eef946090aac75faffac
v3.0.0^2~30^2~11
>
> === cut ===
> func (m MapClaim) VerifyAudience(cmp string) bool {
> val, exists := m["aud"]
> if !exists {
> return true // Don't fail validation if claim doesn't exist
> }
>
> if aud, ok := val.(string); ok {
> return verifyAud(aud, cmp)
> }
> return false
> }
> === cut ===
>
> But this code, while it is faulty, I don't think it is insecure. If a
> list of strings is passed, it will fail the assertion and return false.
> Which is a safe value to return.
>
> That issue is introduced here in the following change, which ignores if
> the type assertion succeeded or not.
>
> https://github.com/dgrijalva/jwt-go/commit/ec042acef733f1a3fdc10291d159e8e7a0b85ce6
And this in
$ git describe --contains ec042acef733f1a3fdc10291d159e8e7a0b85ce6
v3.0.0^2~30^2~6
>
> === cut ===
> -func (m MapClaim) VerifyAudience(cmp string) bool {
> - val, exists := m["aud"]
> - if !exists {
> - return true // Don't fail validation if claim doesn't exist
> - }
> -
> - if aud, ok := val.(string); ok {
> - return verifyAud(aud, cmp)
> - }
> - return false
> +// Compares the aud claim against cmp.
> +// If required is false, this method will return true if the value matches or is unset
> +func (m MapClaim) VerifyAudience(cmp string, req bool) bool {
> + aud, _ := m["aud"].(string)
> + return verifyAud(aud, cmp, req)
> }
>
> [...]
>
> -func verifyAud(aud string, cmp string) bool {
> - return aud == cmp
> +func verifyAud(aud string, cmp string, required bool) bool {
> + if aud == "" {
> + return !required
> + }
> + if subtle.ConstantTimeCompare([]byte(aud), []byte(cmp)) != 0 {
> + return true
> + } else {
> + return false
> + }
> === cut ===
>
> The problem is if the type assertion fails, we get an empty string, and
> latter treat it as if the parameter wasn't even supplied.
>
> If I am reading this git stuff correctly, both changes were introduced -
> in time - between v2.3.0 and v2.4.0, but weren't actually released until
> version 4.0.0-preview1. Which matches what is in the CVE.
Your above tracking of the commits seems correct, which would mean
that the issue was firstly introduced actually in v3.0.0 and given the
code is not found in the buster and stretch version this would not
affect hose versions indeed.
Note that the stretch and buster versions are actually based on the
2.6.0 releases (3.0.0+REALLY.2.6.0-1, 3.0.0.1+REALLY.2.6.0-3
respectively, the reason is that a 3.0.0 version was prematurely
uploaded, and needed to be rolled back,
https://tracker.debian.org/news/804345/accepted-golang-github-dgrijalva-jwt-go-300really260-1-source-into-unstable/
but needs to keep a version higher without using epochs).
So to me updating the CVE entry to not-affected for buster and stretch
(as the respective vulnerable code was introduced later) seems correct
to me.
> The last commit also references a PR, unfortunately it doesn't say which
> one. I think it might be #73:
>
> https://github.com/dgrijalva/jwt-go/pull/73
>
> Here is a great quote:
>
> https://github.com/dgrijalva/jwt-go/pull/73#discussion_r34926597
>
> "Seems like this will lead to security issues. Also, this behavior is
> different from StandardClaims which just compare directly regardless of
> presence."
>
> Oh joy. They knew about the risk here, but looks like they went ahead
> anyway with an insecure solution without properly testing it :-(
Oh :-( It at least confirms the above discussion that it was
introduced upstream in the v3.0.0 release.
Thanks for narrowing that down.
Regards,
Salvatore
Reply to: