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

The nonstandard "no future iat" check is disruptive #475

Open
gulachek opened this issue Dec 18, 2022 · 6 comments
Open

The nonstandard "no future iat" check is disruptive #475

gulachek opened this issue Dec 18, 2022 · 6 comments
Labels
type: feature request v7.0 changes targeted for the next major version

Comments

@gulachek
Copy link

The JWT.php decode() has a nonstandard check to verify that "the time according to the JWT on the issuing server" is not later than "the time on the machine that is verifying the JWT", w/in some apparently statically configured leeway. This presents a problem when the server that signs the JWT has a clock that is ahead of the machine calling decode(), where the code calling decode() is punished by getting an exception thrown, saying a perfectly valid JWT is invalid. The code calling decode() is expected to configure some arbitrary $leeway variable that is likely to break again or sacrifice security.

More details:

The firebase php-jwt JWT.php has this line:
// Check that this token has been created before 'now'. This prevents
// using tokens that have been created for later use (and haven't
// correctly used the nbf claim).
if (isset($payload->iat) && $payload->iat > ($timestamp + static::$leeway)) {
throw new BeforeValidException(
'Cannot handle token prior to ' . \date(DateTime::ISO8601, $payload->iat)
);
}

According to the RFC:
The "iat" (issued at) claim identifies the time at which the JWT was
issued. This claim can be used to determine the age of the JWT. Its
value MUST be a number containing a NumericDate value. Use of this
claim is OPTIONAL.

Notice that it says nothing about validating that this timestamp is not "in the future according to the validating machine's time". Enforcing this in the JWT.php code seems like an issue, as I've already seen (it looks like the Sign In With Google server is off by about 2 seconds with my machine). It's unreasonable to assume that it's a server maintainer's fault for getting out of sync with google's server's timestamp. It seems like it would be better, if the 'iat' is in the future, to use this to offset the timestamp, instead of throwing an exception.

The current workarounds I'm pondering are if I want to set the $timestamp static variable to this offset, to sleep() until 'iat' if it isn't too far in the future, or if I want to just bite the bullet and set the $leeway and cross my fingers that I guessed a good arbitrary leeway value.

@gulachek
Copy link
Author

gulachek commented Dec 18, 2022

For future readers, here's my current workaround (I chose to use the leeway since that seems to be the supported way of doing this).

```
$old_leeway = \Firebase\JWT\JWT::$leeway;

try
{
  // IF YOU COPY/PASTE, MAKE SURE TO CAREFULLY CONSIDER LEEWAY SECURITY VS TOLERANCE FOR YOUR USE CASE
  \Firebase\JWT\JWT::$leeway = 5*60; // let's give 5min buffer for server time offset
  // see https://github.com/firebase/php-jwt/issues/475
  $payload = $client->verifyIdToken($_POST['credential']);
}
catch (\Exception $e)
{
  $err = $e->getMessage();
  return null;
}
finally
{
  \Firebase\JWT\JWT::$leeway = $old_leeway;
}

@gulachek
Copy link
Author

I will compliment you that i was able to immediately find the code in question and understand what it was trying to do and diagnose what was going on, so nice job on the code cleanliness/readability-for-newcomers

@Krisell
Copy link

Krisell commented Dec 19, 2022

I just wanted to chime in here that I have run inte the same problem a few times and always set a small leeway to avoid it. However 5 minutes seems a bit extreme, and might be insecure (since it's used for expiration checks as well). Something like 10 or 30 seconds should be more reasonable.

@bshaffer
Copy link
Collaborator

I agree that the iat is being used in the way that the nbf claim should be used, and there is nothing in the RFC that implies iat should be used for validation.

I am not sure that fixing this issue is something we'd want to do in a minor version, as it would change behavior in an unexpected way that could be considered breaking. So I think this would be an appropriate fix for the next major version of this library.

@bshaffer bshaffer added the v7.0 changes targeted for the next major version label Dec 19, 2022
@gulachek
Copy link
Author

I think this is all reasonable. @Krisell I think tuning the leeway is absolutely a trade off for security vs risk of this functional issue and the cost vs benefit depends on what the JWT is authorizing the user to do. This is a personal website of mine that doesn't really protect sensitive information or let you do much more once authenticated other than edit some recipes. For sensitive stuff like financial information I totally agree but for my use case I just want to forget about the problem. :) It is worth noting that if someone copy/pastes my workaround, careful attention to the tuning should be given and if someone is creating a php library to handle this generically, the consumer of your library should be able to configure a leeway with a conservative default like Krisell noted.

@vergenzt
Copy link

vergenzt commented Dec 4, 2023

FYI I've submitted an errata request to RFC 7519 to update the spec to explicitly prohibit rejecting tokens with iat from "the future".

Feel free to follow along here: Discussion: JWT tokens containing iat values in the future should not be rejected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request v7.0 changes targeted for the next major version
Projects
None yet
Development

No branches or pull requests

4 participants