go-chi / jwtauth

JWT authentication middleware for Go HTTP services
MIT License
541 stars 91 forks source link

`null` values in token lead to panic in jwtauth.Authenticator #58

Closed MattBrittan closed 3 years ago

MattBrittan commented 3 years ago

This is an issue with github.com/lestrrat-go but I'm reporting it here because I suspect others upgrading v1.1.0 and above (and hence moving from github.com/dgrijalva/jwt-go to github.com/lestrrat-go/jwx) may encounter it (and it took me a while to trace the cause). If a token contains a null (e.g. "nullValue": null) then calling jwtauth.Authenticator will result in a panic.

panic(0xcd2ae0, 0xc00000d8f0)
    runtime/panic.go:965 +0x1b9
reflect.Value.Type(0x0, 0x0, 0x0, 0x1256801, 0xca1940)
    reflect/value.go:1908 +0x189
github.com/lestrrat-go/iter/mapiter.AsMap(0x124a168, 0xc000096000, 0xdbd8a0, 0xc000d508a0, 0xc84f80, 0xc00000e948, 0xdbd8a0, 0x1253048)
    github.com/lestrrat-go/iter@v1.0.0/mapiter/mapiter.go:180 +0x5c5
github.com/lestrrat-go/jwx/internal/iter.AsMap(0x124a168, 0xc000096000, 0x123a520, 0xc000d508a0, 0xdbd8a0, 0xdbd801, 0x1253048)
    github.com/lestrrat-go/jwx@v1.1.0/internal/iter/mapiter.go:31 +0x86
github.com/lestrrat-go/jwx/jwt.(*stdToken).AsMap(0xc000d508a0, 0x124a168, 0xc000096000, 0x1253048, 0xc000d508a0, 0x1)
    github.com/lestrrat-go/jwx@v1.1.0/jwt/token_gen.go:491 +0x4b
github.com/go-chi/jwtauth/v5.FromContext(0x124a1d8, 0xc0011a1620, 0x7fa161cc75b8, 0x90, 0xc00053aa20, 0xb994de, 0xc000248c00)
    github.com/go-chi/jwtauth/v5@v5.0.0/jwtauth.go:195 +0xb5
github.com/go-chi/jwtauth/v5.Authenticator.func1(0x1246738, 0xc0005fe460, 0xc000984700)
    github.com/go-chi/jwtauth/v5@v5.0.0/jwtauth.go:165 +0x5d

This issue in the upstream project is: https://github.com/lestrrat-go/iter/issues/1 (I have posted my work around there).

The following demonstrates how this can be replicated within github.com/go-chi/jwtauth:

package main

import (
    "context"
    "crypto/rand"
    "crypto/rsa"
    "fmt"
    "net/http"

    "github.com/go-chi/jwtauth"
    "github.com/lestrrat-go/jwx/jwa"
    "github.com/lestrrat-go/jwx/jwk"
    "github.com/lestrrat-go/jwx/jwt"
)

func main() {
    // Generate a new private key so we can sign a test token
    privKey, err := rsa.GenerateKey(rand.Reader, 2048)
    if err != nil {
        fmt.Printf("failed to generate private key: %s\n", err)
        return
    }
    realKey, err := jwk.New(privKey)
    if err != nil {
        fmt.Printf("failed to create JWK: %s\n", err)
        return
    }
    realKey.Set(jwk.KeyIDKey, `mykey`)

    // Generate the test token with a nil value
    token := jwt.New()
    token.Set("nullValue", nil)

    // Sign the token and generate a payload
    signed, err := jwt.Sign(token, jwa.RS256, realKey)
    if err != nil {
        panic(fmt.Sprintf("failed to generate signed payload: %s\n", err))
    }
    fmt.Printf("token generated: %s\n", string(signed))

       // Now we use jwtauth to process the token (simulating extraction from a cookie)
    var r *http.Request // Will not actually be used..
    tokenAuth := jwtauth.New("RS256", nil, privKey.Public())
    token, err = jwtauth.VerifyRequest(tokenAuth, r, func(r *http.Request) string { return string(signed) })
    if err != nil {
        panic(fmt.Sprintf("Failed to process cookie: %s", err))
    }

    // Add the token to the context
    ctx := jwtauth.NewContext(context.Background(), token, nil)

    // Attempt to retrieve from the context (Authenticator makes this call)
    jwtauth.FromContext(ctx) // panic here
}
pkieltyka commented 3 years ago

thanks for the report. I'll solve this one once I can get to it

lestrrat commented 3 years ago

Hi, sorry about not noticing the issue and what not. I have just released v1.1.6 of github.com/lestrrat-go/jwx. I didn't go as far as checking if github.com/lestrrat-go/jwx was actually free of the above problem, but at least lestrrat-go/iter#1 is definitely fixed and v1.0.1 of github.com/lestrrat-go/iter is now being used in jwx.

MattBrittan commented 3 years ago

Thanks very much @lestrrat; I have verified that the updated jwx package resolves this crash. As it's a point upgrade a go get -u picks it up so no changes should be required in the jwtauth package. @pkieltyka - thanks for your response; I figured this needed to be fixed in the upstream project and really only raised it here as it seemed likely someone else would run into it.

lestrrat commented 3 years ago

Well, I would think it would be safer for the dependency in this project to be upgrade too, no?

pkieltyka commented 3 years ago

https://github.com/go-chi/jwtauth/commit/25eecdfa2c9f1f118f5df90e2ccdb5017af0fa42