JWT parsing + verification with KeySet doesn't use VerifySet?

[mrbanzai]

It seems that jwt.Parse along with the WithKeySet option ends up using jws.Verify in conjunction with the header algorithm (by way of jwt.lookupMatchingKey), instead of jws.VerifySet, which would use/require the key's algorithm. Given that the header algorithm is generally considered to be untrusted, would it make sense to have jwt.lookupMatchingKey return the key's defined algorithm, perhaps falling back to the header algorithm if not defined? Unfortunately, that's potentially a breaking change in cases where callers were inadvertently using the header algorithm with a different, compatible algorithm defined on the key.

For my usage, I can likely just fetch the key from the keyset manually and use WithVerify(key.Algorithm(), key), but it seemed odd that WithKeySet behaves differently with regard to the algorithm than going through VerifySet.

[lestrrat]

Given that the header algorithm is generally considered to be untrusted ....

jwt.Parse uses jwt.lookupMatchingKey if and only if you specify the key set to be used against a payload.

Then, using that key set, checks if the JWT is signature was generated by a key with a matching Key ID.
Once it finds a matching key, it takes that key, and uses that key's alg value to pass to jws.Verify

What I'm trying to say is that, it never uses the value given in the JWS message. It only trusts the value that you the user have provided, which is the intended behavior. We don't, and will not trust the JWS headers.

So I think we're okay on that front. Let me know if I'm wrong.

---

On the other hand, I jws.VerifySet does indeed differ from jwt.Parse. I suspect that this discrepancy arose from when those functions were written: jws.VerifySet is an artifact from 0.9.x days, and the jwt.WithKeySet option is a slightly newer addition.

IIRC the request I got for jwt.parse(jwt.WithKeySet()) was to match against the kid header, so that's what I provided: I guess at the time I didn't think that we needed to check for "usage" key. But for the record, jws.verifyCompact and jws.verifyJSON they both check for kid, so the only difference really is checking for "usage"

[lestrrat]

lookupMatchingKey is returning the algorithm name, but it's returning from the key provided by the user IIRC (didnt check right now).

[mrbanzai]

I crafted a small example to demonstrate what I saw when reading through the code. Just to be clear, however, I don't believe it's a vulnerability, as the user-provided key is still used ... but verified with the header-specified algorithm. I raised the question because it seemed that the behavior was inconsistent, with one manner of keyset-based verification using the algorithm on the key, and the other using the value from the header.

token := jwt.New()

rawPrivKey, err := rsa.GenerateKey(rand.Reader, 2048)
if err != nil {
    panic(err)
}

signingKey, err := jwk.New(rawPrivKey)
if err != nil {
    panic(err)
}
signingKey.Set(jwk.KeyIDKey, `test`)

signingAlg := jwa.RS512
fmt.Printf("signing with %s\n", signingAlg)
raw, err := jwt.Sign(token, jwa.RS512, signingKey)
if err != nil {
    panic(err)
}

verificationAlg := jwa.RS256
verificationKey, err := jwk.New(rawPrivKey.Public())
if err != nil {
    panic(err)
}
verificationKey.Set(jwk.KeyIDKey, `test`)
// note:  different algorithm than signed with
verificationKey.Set(jwk.AlgorithmKey, verificationAlg)
fmt.Printf("verification key configured with %s alg\n", verificationKey.Algorithm())

keySet := jwk.NewSet()
keySet.Add(verificationKey)

_, err = jwt.Parse(raw, jwt.WithKeySet(keySet))
if err != nil {
    panic(err)
}

To see which algorithm was being used for the verification, I added the following debug print to the top of jwt.parse:

fmt.Printf("parsing with algorithm %s\n", alg)

And the resulting output is:

signing with RS512
verification key configured with RS256 alg
parsing with algorithm RS512

So it seems jwt.Parse is using the header algorithm for verification when used in conjunction with jwt.WithKeySet.

[lestrrat]

Hi, first of all, thanks for the research, really appreciate it.

Now while I intend to take a closer look, I want to stress the importance of coming up with a test case that is conclusive for both of us. PRs (or patches) against a know revision/tree is really important to avoid possible misunderstanding or accidentally working with the wrong assumptions.

It would help immensely if you could just create a PR against the main branch, which contains a standalone testcase for what you are describing.

[lestrrat]

Alright, so yes. this time I was able to pinpoint the problem quickly enough, I read my code wrong. header.Algorithm() should really be key.Algorithm(). Will create an issue

[lestrrat]

@mrbanzai #381 should fix this issue. It has already been merged. Can you please check?

As a side effect, jwt.WithKeySet() now has a limitation that all valid keys MUST have its "alg" field populated

[mrbanzai]

I've confirmed that #381 fixes the issue with the header algorithm being used for verification. Thanks!

[lestrrat]

Thanks, v1.2.0 has been released. Apologies for my slow response earlier.