Token without ExpiresAt runs panic

[82andre]

Token without ExpiresAt runs panic, the panic has a recovery, but it would be better have an error.

Sample code: Code in Playground

package main

import (
	"bytes"
	"errors"
	"fmt"
	"io"
	"log"
	"net/http"
	"net/http/httptest"
	"strings"
	"time"

	"github.com/golang-jwt/jwt/v4"
)

func main() {
	fmt.Println("RUN createToken")
	token, err := createToken()
	fatal(err)
	runTest(token) //this RUN OK

	fmt.Println("RUN createTokenThatNotExpire")
	tokenNotExpire, err := createTokenThatNotExpire()
	fatal(err)
	runTest(tokenNotExpire) //this call panic at parser.go:85
	fmt.Println("finish")
}
func runTest(token string) {
	req := httptest.NewRequest("GET", "http://localhost/test", nil)
	req.Header.Set("Authorization", fmt.Sprintf("Bearer %v", token))
	w := httptest.NewRecorder()
	err := auth(w, req)
	fatal(err)
	res := w.Result()
	// Read the response body
	buf := new(bytes.Buffer)
	io.Copy(buf, res.Body)
	res.Body.Close()
	fmt.Println(buf.String())
	fmt.Println(token)
	fmt.Println("end.")
}

func createToken() (string, error) {
	t := jwt.New(jwt.GetSigningMethod("HS256"))
	t.Claims = &tokenClaims{
		&jwt.RegisteredClaims{
			ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 1)),
		},
		tokenInfo{"test", "1.0", "hi@hi.com.br"},
	}
	return t.SignedString(mySigningKey)
}
func createTokenThatNotExpire() (string, error) {
	t := jwt.New(jwt.GetSigningMethod("HS256"))
	t.Claims = &tokenClaims{
		&jwt.RegisteredClaims{
			//ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 1)),
		},
		tokenInfo{"test", "1.0", "hi@hi.com.br"},
	}
	return t.SignedString(mySigningKey)
}

func auth(w http.ResponseWriter, r *http.Request) error {
	tokenStr, ok := r.Header["Authorization"]
	if !ok || len(tokenStr) != 1 {
		fatal(errors.New("authorization token not found"))
	}
	st := strings.Split(tokenStr[0], " ")
	if len(st) > 0 && strings.TrimSpace(st[0]) != "Bearer" {
		fatal(errors.New(`token should contain "Bearer"`))
	}
	token, err := jwt.ParseWithClaims(st[1], &tokenClaims{}, func(token *jwt.Token) (interface{}, error) {
		if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok {
			return nil, errors.New("signing method invalid")
		}
		return mySigningKey, nil
	})
	fatal(err)
	if !token.Valid {
		fatal(errors.New("invalid token"))
	}
	return nil
}

func fatal(err error) {
	if err != nil {
		log.Fatal("Test error: %s \n", err)
	}
}

type tokenInfo struct {
	System  string `json:"system"`
	Version string `json:"version"`
	Email   string `json:"email"`
}

type tokenClaims struct {
	*jwt.RegisteredClaims
	tokenInfo
}

var mySigningKey = []byte("MyPassword")

[oxisto]

Ok this is a weird one. It is probably 50 % a bug and 50 % a slight mis-use of the API. One of the issues is that your tokenClaims struct is embedding a pointer to *jwt.RegisteredClaims. You then supply an empty &tokenClaims{} to the jwt.ParseWithClaims for it to be processed. Note, that since you are embedding *jwt.RegisteredClaims this will be nil by default within your tokenClaims struct.

This tokenClaims struct will then be copied to token.Claims and used for JSON deserialisation. Now since in the first example your JWT has some attributes set (in this case ExpiresAt), a new *jwt.RegisteredClaims object is created by the JSON deserialiser and everything is fine. In the second case however, you are supplying an empty JWT without any values and thus the serialiser basically does nothing and the embedded *jwt.RegisteredClaims of token.Claims within ParseWithClaims stays nil and it crashes when we call Valid on it.

In general, I would recommend embedding not a pointer to *jwt.RegisteredClaims but only jwt.RegisteredClaims as seen in the example. or at least filling it when you supply the "empty" &tokenClaims{}, such as &tokenClaims{RegisteredClaims: &jwt.RegisteredClaims{}}

I am not even sure that we can really do anything to fix this in the parser to be honest.

[82andre]

Thanks @oxisto remove the pointer in jwt.RegisteredClaims works, you can close this, but I think that the example need to be changed.
I copy the pointer from the example at
https://pkg.go.dev/github.com/golang-jwt/jwt#example-package-UseTokenViaHTTP

type CustomClaimsExample struct {
	*jwt.StandardClaims  ////////////////HERE
	TokenType string
	CustomerInfo
}
...
func createToken(user string) (string, error) {
	t := jwt.New(jwt.GetSigningMethod("RS256"))
	t.Claims = &CustomClaimsExample{
		&jwt.StandardClaims{  //////////////HERE
			ExpiresAt: time.Now().Add(time.Minute * 1).Unix(),
		},
		"level1",
		CustomerInfo{user, "human"},
	}
	return t.SignedString(signKey)
}

[dvasilen]

A fix or an ETA is appreciated. Twistlock reports this vulnerability as PRISMA-2022-0270

github.com/golang-jwt/jwt/v4 module from all versions is vulnerable to Denial of Service (DoS) due to token without ExpiresAt can cause panic.

[oxisto]

A fix or an ETA is appreciated. Twistlock reports this vulnerability as PRISMA-2022-0270

github.com/golang-jwt/jwt/v4 module from all versions is vulnerable to Denial of Service (DoS) due to token without ExpiresAt can cause panic.

Not sure if this is really a vulnerability. This is primarily an API mis-usage and probably a bad example. I can try to fix the example and see whether we can add an extra guard against an empty pointer.

[oxisto]

I looked a bit more into the issue and I am afraid we cannot really "solve" this as this is a shortcoming of the Go language.

The problem lies in this line of the original code:

	token, err := jwt.ParseWithClaims(st[1], &tokenClaims{}, func(token *jwt.Token) (interface{}, error) {

Here, a tokenClaims struct is passed to the jwt.ParseWithClaims without allocating memory to the embedded pointer of jwt.RegisteredClaims. This is basically not a good idea. One should either use a non-pointer version or allocate the memory &tokenClaims{RegisteredClaims: &jwt.RegisteredClaims{}}. Unfortunately, I cannot check if this embedded field is nil without MAJOR reflection magic.

I tried to at least fix the example and add it to the description via #255

[DataMinded]

the 4.4.3 release is still beeing flaged by Prisma Cloud as a vulnerability sadly