Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: Require Key object when decoding JWT objects #364

Closed
wants to merge 9 commits into from

Conversation

bshaffer
Copy link
Collaborator

@bshaffer bshaffer commented Nov 3, 2021

see #352 and #351

Builds off #365, but requires the changes (to be tagged in v6) for better security

  • BC Breaking - requires the use of Firebase\JWT\Key when calling JWT::decode instead of passing in the $allowed_algs array.
  • Key accepts a resource, string, or instance of OpenSSLAsymmetricKey
  • JWK now requires the JWK Keyset to have alg set, which is an optional parameter. If there's a straight foward (and reliable) way to detect the algorithm from the other parameters in JWK, we should add this logic as well.

cc @paragonie-security

This aims to provide backwards compatibility by guessing the algorithm for a key based on the key's contents, but it will likely fail in corner cases.

If this is merged, users **SHOULD** be explicit about the algorithms they're using.

i.e. Instead of $keyAsString, pass in (new JWTKey($keyAsString, 'ES384'))
@google-cla

This comment has been minimized.

@google-cla google-cla bot added the cla: no label Nov 3, 2021
@firebase firebase deleted a comment from google-cla bot Nov 3, 2021
@firebase firebase deleted a comment from google-cla bot Nov 3, 2021
@firebase firebase deleted a comment from google-cla bot Nov 3, 2021
@google-cla

This comment has been minimized.

1 similar comment
@google-cla

This comment has been minimized.

@bshaffer bshaffer added the v6.0 label Nov 3, 2021
// for parsing in this library. Add it manually to your JWK
// array if it doesn't already exist.
// @see https://datatracker.ietf.org/doc/html/rfc7517#section-4.4
throw new InvalidArgumentException('JWK key is missing "alg"');
Copy link
Collaborator Author

@bshaffer bshaffer Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paragonie-security if the "alg" parameter is missing from a JWK, is there a reliable way to detect it? Or is it best to just throw the exception and require the users to specify it manually?

We can use the "kty" field to find out of the key is of type RSA or EC. I imagine there is a way to detect more algorithms from there as well, but I don't want to add magic unless I know it's reliable.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible, but it's going to require a bit of additional complexity.

First, you have to know if you're dealing with a DER- and PEM-encoded key.

  1. If it's not base64-encoded, you aren't. (Thus, not PEM.) At this point, you might want to assume symmetric (AES or HMAC).
    • Without a breadcrumb to distinguish them, you're safe assuming HMAC. However, in other libraries (i.e. ones with JWE support), our recommendation would be to instead throw an exception. Failure to do so can reintroduce alg confusion.
  2. If you can base64-decode the string and get a valid byte string, you can then verify that the first few bytes corresponds to a valid ASN.1 object identifier. This will map uniquely to a specific key algorithm.

This table below maps some hex-encoded ASN.1 prefixes to the respective algorithms, based on this encoder and the OIDs specified in IETF RFCs.

ASN.1 Prefix Key Type
06 09 2A 86 48 86 F7 0D 01 01 01 RSA
06 07 2A 86 48 CE 3D 02 01 ECDSA
30 2E 02 01 00 30 05 06 03 2B 65 70 04 22 04 20 EdDSA (Curve25519)

So that's what you would need to do.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is on top of the -----BEGIN PUBLIC KEY----- and -----END PUBLIC KEY-----, of course.)

@firebase firebase deleted a comment from google-cla bot Nov 3, 2021
@firebase firebase deleted a comment from google-cla bot Nov 3, 2021
@firebase firebase deleted a comment from google-cla bot Nov 3, 2021
@google-cla

This comment has been minimized.

@firebase firebase deleted a comment from google-cla bot Nov 3, 2021
@firebase firebase deleted a comment from google-cla bot Nov 3, 2021
@google-cla

This comment has been minimized.

5 similar comments
@google-cla

This comment has been minimized.

@google-cla

This comment has been minimized.

@google-cla

This comment has been minimized.

@google-cla

This comment has been minimized.

@google-cla

This comment has been minimized.

@paragonie-security
Copy link

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 3, 2021
@paragonie-security
Copy link

paragonie-security commented Nov 4, 2021

If you need a PHP snippet that will parse a given public key and make a best effort guess at which algorithm is expressed (based on the PEM and DER-encoded bytes of the key), add this method to the JWT class (because private methods):

    /**
     * Attempt to deduce the algorithm for a given key
     *
     * This should only be used as a last resort.
     *
     * @param string $key
     * @return string|null
     * @throws Exception
     */
    public static function guessAlgorithmFromKey($key)
    {
        $length = self::safeStrlen($key);
        if ($length <= 64) {
            if ($length === 64) return 'HS512';
            if ($length === 48) return 'HS384';
            // Default to the most common value
            return 'HS256';
        }

        // First, strip off header/footer, to obtain raw base64 value
        $expect = '-----BEGIN PUBLIC KEY-----';
        $actual = \substr($key, 0, 26);
        if (self::constantTimeEquals($expect, $actual)) {
            $expect = '-----END PUBLIC KEY-----';
            // strlen('-----END PUBLIC KEY-----') === 24
            $actual = \substr($key, $length - 24, 24);
            if (!self::constantTimeEquals($expect, $actual)) {
                // Not a well-formed public key!
                return null;
            }
            // Now we have the base64-encoded portions
            $b64 = \trim(\substr($key, 26, $length - 50));
        } else {
            // Assume base64-encoded
            $b64 = $key;
        }

        // Get raw bytes (provided by sodium_compat)
        $raw = sodium_base642bin($b64, SODIUM_BASE64_VARIANT_ORIGINAL, "\r\n");
        if (!is_string($raw)) {
            // Not a valid PEM-encoded string
            return null;
        }
        $rawLen = self::safeStrlen($raw);

        // Is this an EdDSA public key?
        $expect = "\x30\x2E\x02\x01\x00\x30\x05\x06\x03\x2B\x65\x70\x04\x22\x04\x20";
        $actual = \substr($raw, 0, 16);
        if (self::constantTimeEquals($expect, $actual)) {
            return 'EdDSA';
        }

        // Is this an ECDSA public key?
        $expect = "\x06\x07\x2A\x86\x48\xCE\x3D\x02\x01";
        $actual = \substr($raw, 0, 9);
        if (self::constantTimeEquals($expect, $actual)) {
            // Okay, which length is it?
            if ($rawLen === 91) return 'ES256';
            if ($rawLen === 120) return 'ES384';
            if ($rawLen === 158) return 'ES512';
        }

        // Is this an RSA public key?
        $expect = "\x06\x09\x2A\x86\x48\x86\xF7\x0D\x01\x01\x01";
        $actual = \substr($raw, 0, 11);
        if (self::constantTimeEquals($expect, $actual) && $rawLen > 128) {
            // An RSA public key can be for multiple algorithms, but we fallback to the most common.
            return 'RS256';
        }

        // Last resort: Return nothing
        return null;
    }

And then your JWK loop can look like this:

        foreach ($jwks['keys'] as $k => $v) {
            $kid = isset($v['kid']) ? $v['kid'] : $k;
            if ($key = self::parseKey($v)) {
                if (isset($v['alg'])) {
                    $keys[$kid] = new Key($key, $v['alg']);
                } else {
                    $parsedAlg = JWT::guessAlgorithmFromKey($key);
                    if (!is_string($parsedAlg)) {
                        // The "alg" parameter is optional in a KTY, but is required
                        // for parsing in this library. Add it manually to your JWK
                        // array if it doesn't already exist.
                        // @see https://datatracker.ietf.org/doc/html/rfc7517#section-4.4
                        throw new InvalidArgumentException('JWK key is missing "alg"');
                    }
                    $keys[$kid] = new Key($key, $parsedAlg);
                }
            }
        }

(Provided as a comment so you can adapt it to your own coding style.)

@bshaffer bshaffer changed the title feat: Require Key object when decoding JWT objects feat!: Require Key object when decoding JWT objects Nov 9, 2021
@bshaffer bshaffer mentioned this pull request Nov 9, 2021
6 tasks
@bshaffer
Copy link
Collaborator Author

bshaffer commented Nov 9, 2021

Closing in favor of #373

@bshaffer bshaffer closed this Nov 9, 2021
@bshaffer bshaffer deleted the add-key-object branch November 9, 2021 20:49
This was referenced Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants