Security concerns with jwt.InferAlgorithmFromKey(true)

[jordaniversen]

Hello,

I'm just curious as to whether the warning at https://github.com/lestrrat-go/jwx/blob/main/docs/01-jwt.md?plain=1#L184 is still valid. By looking at the code at https://github.com/lestrrat-go/jwx/blob/main/jwt/jwt.go#L282 it seems like it is simply checking for an exact match of the algorithm, and not attempting to try multiple algorithms in a brute force way.

Please forgive any ignorance, thank you!

[lestrrat]

Not sure where you are getting this from

it seems like it is simply checking for an exact match of the algorithm, and not attempting to try multiple algorithms in a brute force way.

There is a for loop there.

jwx/jwt/jwt.go

Lines 274 to 288 in b88feb7

	algs, err := jws.AlgorithmsForKey(key)
	if err != nil {
	return nil, _JwsVerifyInvalid, errors.Wrapf(err, `failed to get a list of signature methods for key type %s`, key.KeyType())
	}
	for _, alg := range algs {
	// bail out if the JWT has a `alg` field, and it doesn't match
	if tokAlg := headers.Algorithm(); tokAlg != "" {
	if tokAlg != alg {
	continue
	}
	}
	return verifyJWSWithParams(ctx, payload, alg, key)
	}

[jordaniversen]

Hi Thanks for the quick response. Ya I saw the for loop but still isn't it only going to do a single action against a single algorithm that exactly matches? I interpreted that documentation to mean that it will try to verify the signature in multiple ways or with multiple algorithms and "hope" that one succeeds or something, so perhaps I just misinterpreted it? Sorry if this is such a trivial question, I just wanted to confirm this case as if it is unsafe to rely on this then I unfortunately need to try combining multiple other libraries, which I dont want to do because this one seems much more flexible and extensible.

…

On Fri., Mar. 11, 2022, 4:15 a.m. lestrrat, ***@***.***> wrote: Not sure where you are getting this from it seems like it is simply checking for an exact match of the algorithm, and not attempting to try multiple algorithms in a brute force way. There is a for loop there. https://github.com/lestrrat-go/jwx/blob/b88feb74d96610906155732f429b5fd3b7ef2ae6/jwt/jwt.go#L274-L288 — Reply to this email directly, view it on GitHub <#643 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHJO2GHYBQVSNAHUF53OZGDU7MFKXANCNFSM5QOTV5ZA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[lestrrat]

I think I see where you're coming from. to give you some history, the AlgorithmsFor() bit was slapped on top of what it used to do to make keys that don't have alg to work... so I think that's why it's the way it is. I think there is discrepancy between what was intended and what the implementation is:

I think what should have been done is (pseudocode)

for alg in algs {
   if token.alg == "" || token.alg == alg {
      tryVerify
    }
}
```

This way it works for cases where keys without alg are used but the token itself has one, or the case when the token itself doesn't have one. Does this seem like a better approach?

[jordaniversen]

Ah if it is simply a slight inefficiency then I'm ok with that, but if it was a security issue that I wasn't seeing then that's a different story. :) Thanks

…

On Fri., Mar. 11, 2022, 4:39 p.m. lestrrat, ***@***.***> wrote: oh I see what you mean. of course! :) it lists a possible set of algorithms that can be tried, and tries to match until it finds one that can be used. so right, it won't try to actually do the verification, but it would try search the entire set of RS256, RS384, RS512, etc for an RSA key and this needs to be done for each key in the keyset. I'd call it a wasteful operatin, and that's what that document means — Reply to this email directly, view it on GitHub <#643 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHJO2GF5K65CY5ODG4NA6J3U7O4PLANCNFSM5QOTV5ZA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[lestrrat]

Oh wait, it is written that way (spoke from memory)

  for _, alg := range algs {
    // if we have a "alg" field in the JWS, we can only proceed if
    // the inferred algorithm matches
    if hdrAlg != "" && hdrAlg != alg {
      continue
    }

[lestrrat]

Well, there is a security issue, no? I mean, if the alg field in the token or key was empty, then how do you know if the verification that succeeded was a result of an unintended bug or not (maybe they meant to rotate RS256+an old key to RS512+new key): You can't tell if you're doing the right thing, or they are still using the old key by mistake. Matching the algorithm is a security concern. I personally sympathize with the programmers that need to deal with such a situation, but I don't with those who made the decision to omit the algorithm

[jordaniversen]

key.Set(jwk.AlgorithmKey, jwa.ES256) // or whatever

Thanks. In my case, i'm using jwt.WithKeySetProvider and autorefresh. So I guess with this approach, I would need to put that for loop and hard coding of the algorithm in my jwt.KeySetProviderFunc function meaning it would need happen each time I need to parse a JWT, correct? I.e. I couldn't do it once, outside of jwt.KeySetProviderFunc due to the auto refresh, I assume. If this is correct, what would be the best way around this (or would it be a future improvement)? I assume some callback to define once auto refresh has completed. And yes I agree the repeating "for" loop calls really isn't the end of the world in terms of performance.

[lestrrat]

I assume some callback to define once auto refresh has completed.

Ah ok. That's something that doesn't exist as of today, and that's definitely something we could do

[lestrrat]

done for v2 #650

[jordaniversen]

Awesome thanks will give it a try!

[jordaniversen]

Hi again @lestrrat,

I realize this isnt an OIDC library, but as per https://openid.net/specs/openid-connect-core-1_0.html#RotateSigKeys is there a way to auto refresh the keys in the JWKS if a "kid" isn't found? If not, I guess that makes sense because again this isnt OIDC, and I'll use the auto refresh internal.