Re: golang-github-dgrijalva-jwt-go / CVE-2020-26160

[Brian May <bam@debian.org>]

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

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

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

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 :-(
-- 
Brian May <bam@debian.org>