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

Parsing/Validating a JWT token with invalid exp field fails on core 31. #1525

Closed
blowdart opened this issue Aug 31, 2020 · 13 comments
Closed
Assignees
Labels
Bug Product is not functioning as expected Customer reported Indicates issue was opened by customer P1 More important, prioritize highly

Comments

@blowdart
Copy link

From dotnet/aspnetcore#25419

Bug description

JwtPayload.Exp throws if tokens contains a DateTIme instead of numeric value

Expected behavioud

It should return null

Details

The documentation of System.IdentityModel.Tokens.Jwt.JwtPayload.Exp states :

If the 'expiration' claim is not found OR could not be converted to <, null is returned.

However this is not the case. Exp property geter throws an exception if values is a Datetime. Here is a sample token

{
  "exp": "2020-09-08T21:18:18.5297739+02:00",
  "name": "tojename"
}

Please note that the payload is incorrect (exp should be a numeric type as per https://tools.ietf.org/html/rfc7519#section-4.1.4)

The JwtPayload.GetIntClaim() tires exception of type FormatException, InvalidCastException and OverflowException, but it does not handle InvalidCastException which is thrown by DateTime.IConvertible.ToDouble

My scenario is interop with some legacy service that created invalid token. This is not critical issue for me, I am just pointing out inconsistency in implementation/documentation.

The implementation ignores exp if it sets to "foo", but not if it set to a DateTime.
The documentation states that invalid values are ignored, but they are not for DaateTime.

@blowdart
Copy link
Author

@matra774

@keegan-caruso keegan-caruso added Bug Product is not functioning as expected Customer reported Indicates issue was opened by customer labels Aug 31, 2020
@keegan-caruso
Copy link
Contributor

Thanks @matra774 for reporting this.

@brentschmaltz do we want to get this into 6.7.3 or 6.7.3 +1?

@brentschmaltz
Copy link
Member

@blowdart @keegan-caruso @matra774 it looks like our documentation is incorrect. The exp, iat, nbf claims should be NumericDate https://tools.ietf.org/html/rfc7519#section-4.1.4 . We should be throwing.

It would be prudent to ensure that JsonWebToken and JwtSecurityToken handle the same dates in the same way.

@matra774
Copy link

matra774 commented Sep 2, 2020

it looks like our documentation is incorrect.

Not just the docs, the implementation also. All incorrect data should be handled in the same way (now string are handled differently than DateTime when NumerDate is expected)

@brentschmaltz
Copy link
Member

@matra774 yes i agree a "DateTime" such as "2020-09-08T21:18:18.5297739+02:00" is not NumericDate. However, if we modify this behavior and throw, we may break users.

@matra774
Copy link

matra774 commented Sep 6, 2020

@brentschmaltz I do not suggest to modify the behavior so that exp property getter throws. On a contrary. It currently throws, if a value is DateTime string - I suggest that it should NOT throw (as is the case if you use "foo" as the value for exp)

On a related note. The exp property currently returns int32 making it prone to year 2038 problem.

Currently token with high expiry dates behaves as if there is no exp field in the token. If you try to validate such token you will get exception IDX10225: Lifetime validation failed. The token is missing an Expiration Time.

Sample token: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiI1IiwibmJmIjoxNTk5NDExNDkxLCJleHAiOjIyMDQyMTE0OTEsImlhdCI6MTU5OTQxMTQ5MSwiaXNzIjoiaHR0cDovL215c2l0ZS5jb20iLCJhdWQiOiJodHRwOi8vbXlhdWRpZW5jZS5jb20ifQ.sXIRrKBgdnRfm2YLQnSn9fOcl3RGyXXpagkF9Z4jKno

@divyanshumehta
Copy link

@brentschmaltz I am getting a token exception message like this, is it related to this one. My token is valid and exp value is 1599280362

"IDX10223: Lifetime validation failed. The token is expired. ValidTo: 'System.DateTime', Current time: 'System.DateTime'."

@keegan-caruso
Copy link
Contributor

@divyanshumehta

By default all parameters are censored. To show the parameters (potential PII) see:

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/wiki/PII

@divyanshumehta
Copy link

divyanshumehta commented Sep 9, 2020

ohh sorry I looked into it, i think since I am +3 hours from UTC that is why the token is always showing expired.

@brentschmaltz
Copy link
Member

@matra774 i agree we have 2038 issue and will have to add a new API, so we don't break backcompat.

@keegan-caruso i see a couple of issues with out GetIntClaim to handle 'exp', 'iat', 'nbf'

  1. Should we be allowing an array?
  2. we should not be throwing if we can't be converting to int.

@brentschmaltz brentschmaltz added this to the 6.7.3 milestone Sep 29, 2020
@brentschmaltz brentschmaltz modified the milestones: 6.7.3, v6 Backlog, 6.8.1 Oct 26, 2020
@brentschmaltz brentschmaltz added the P1 More important, prioritize highly label Oct 29, 2020
@mafurman mafurman self-assigned this Nov 3, 2020
@henrik-me henrik-me added this to To do in IdentityModel Board Nov 16, 2020
@mafurman mafurman modified the milestones: 6.8.1, 6.8.2 Jan 12, 2021
@mafurman mafurman removed this from To do in IdentityModel Board Jan 13, 2021
@jairofranchi
Copy link

jairofranchi commented Mar 31, 2021

@divyanshumehta

By default all parameters are censored. To show the parameters (potential PII) see:

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/wiki/PII

@keegan-caruso This problem that @divyanshumehta reported, the only way to show the date would be setting this flag to true, right? Because if this exception is throw, in production environment the date will always be hidden, since doesn't make sense to show PII in production environments.

@brentschmaltz brentschmaltz assigned sruke and unassigned mafurman Nov 4, 2021
@brentschmaltz
Copy link
Member

@jairofranchi assigning this to @sruke.

@jennyf19 jennyf19 modified the milestones: 6.9.1, 7.0.0-preview5 Sep 11, 2023
@jennyf19
Copy link
Collaborator

fixed in IdentityModel 7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Product is not functioning as expected Customer reported Indicates issue was opened by customer P1 More important, prioritize highly
Projects
None yet
Development

No branches or pull requests

9 participants