Go: Add JWT Algorithm Confusion Query

[Kwstubbs]

JWT Algorithm Confusion Portion of #14081

[github-actions]

QHelp previews:

go/ql/src/experimental/CWE-347/JWTParsingAlgorithm.qhelp

JWT Method Check

Not verifying the JWT algorithim present in a JWT token when verifying its signature may result in algorithim confusion.

Recommendation

Before presenting a key to verify a signature, ensure that the key corresponds to the algorithim present in the JWT token.

Example

The following code uses the asymmetric public key to verify the JWT token and assumes that the algorithim is RSA. By not checking the signature, an attacker can specify the HMAC option and use the same public key, which is assumed to be public and often at endpoint /jwt/jwks.jso, to sign the JWT token and bypass authentication.

package main

import (
	"fmt"

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

type JWT struct {
	privateKey []byte
	publicKey  []byte
}

func (j JWT) Validate(token string) (interface{}, error) {
	key, err := jwt.ParseRSAPublicKeyFromPEM(j.publicKey)
	if err != nil {
		return "", fmt.Errorf("validate: parse key: %w", err)
	}

	tok, err := jwt.Parse(token, func(jwtToken *jwt.Token) (interface{}, error) {

		return key, nil
	})
	if err != nil {
		return nil, fmt.Errorf("validate: %w", err)
	}

	claims, ok := tok.Claims.(jwt.MapClaims)
	if !ok || !tok.Valid {
		return nil, fmt.Errorf("validate: invalid")
	}

	return claims["dat"], nil
}

References

• Pentesterlab: Exploring Algorithm Confusion Attacks on JWT.
• Common Weakness Enumeration: CWE-347.

[owen-mc]

482 tests passed; 2 tests failed:
FAILED: /home/runner/work/codeql/codeql/go/ql/test/experimental/CWE-347/CONSISTENCY/UnexpectedFrontendErrors.ql
FAILED: /home/runner/work/codeql/codeql/go/ql/test/experimental/CWE-347/ParseJWTWithoutVerification.qlref

The first one means there are build failures in a .go file in that directory.

[owen-mc]

Is it possible to specify which argument?

[owen-mc]

Also, I think this only works if the function literal is specified in place. If it's assigned to a variable and then that variable is used as an argument, would this catch it?

[Kwstubbs]

@owen-mc having a bit of trouble coming up with a good way to see if the function name that is passed into the argument has a field read of the Method or not. If you can find another way that avoids strings altogether please let me know.

[Kwstubbs]

will change to getASyntacticArgument

[owen-mc]

No need for the brackets either.

Suggested change

	not exists(CallNode c2, WithValidMethods wvm, NewParser m, int i |
	c2.getTarget() = m and
	(
	c2.getSyntacticArgument(i) = wvm.getACall() and
	DataFlow::localFlow(c2.getResult(0), c.getReceiver())
	)
	)
	not exists(CallNode c2, WithValidMethods wvm, NewParser m |
	c2.getTarget() = m and
	c2.getASyntacticArgument() = wvm.getACall() and
	DataFlow::localFlow(c2.getResult(0), c.getReceiver())
	)

[owen-mc]

Please update your tests so that something fails when these lines are commented out. (Or, if these lines aren't working at the moment, update the tests so that they fail and will pass when these lines work.) I can then easily see what they're meant to be targeting and suggest how to implement them without using toString.