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

validation.rs panics due to improper exp while calculating less_then window/leeway #388

Open
0xd-0 opened this issue May 8, 2024 · 5 comments · May be fixed by #390
Open

validation.rs panics due to improper exp while calculating less_then window/leeway #388

0xd-0 opened this issue May 8, 2024 · 5 comments · May be fixed by #390
Labels

Comments

@0xd-0
Copy link

0xd-0 commented May 8, 2024

    if matches!(claims.exp, TryParse::Parsed(exp) if options.validate_exp
        && exp - options.reject_tokens_expiring_in_less_than < now - options.leeway )

e.g. claims.exp can be "1" and pass the parse check but overflow in the calculation and lead to panic. stack trace below.


thread 'tokio-runtime-worker' panicked at /Users/0xd/.cargo/registry/src/index.crates.io-6f17d22bba15001f/jsonwebtoken-9.3.0/src/validation.rs:258:16:
attempt to subtract with overflow
stack backtrace:
0: rust_begin_unwind
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/std/src/panicking.rs:647:5
1: core::panicking::panic_fmt
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/panicking.rs:72:14
2: core::panicking::panic
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/panicking.rs:144:5
3: jsonwebtoken::validation::validate
at /Users/0xd/.cargo/registry/src/index.crates.io-6f17d22bba15001f/jsonwebtoken-9.3.0/src/validation.rs:258:16
4: jsonwebtoken::decoding::decode
at /Users/0xd/.cargo/registry/src/index.crates.io-6f17d22bba15001f/jsonwebtoken-9.3.0/src/decoding.rs:267:13

@Keats Keats added the bug label May 8, 2024
@Keats
Copy link
Owner

Keats commented May 8, 2024

Should be an easy bug to fix if someone wants to try

@0xd-0
Copy link
Author

0xd-0 commented May 8, 2024

Taking a crack at it. I stumbled on it while testing my implementation of JWT in Postman and lazily threw a 1 in the claims.exp to see if I had the minimum info setup and thats when it panic'd. Please let me know what you think.
issue_388.patch

@Keats
Copy link
Owner

Keats commented May 18, 2024

I think we could probably just check whether all the timestamp values are sane before?

@0xd-0
Copy link
Author

0xd-0 commented May 19, 2024

Agreed, the exp timestamp should be cast into a valid Date obj or fail (DateTime::from_timestamp). I noticed that you were using wasm bindings to get the timestamps, so I'm not sure how you would want to check the timestamp is valid without adding a new binding.

My thinking was simply to fix the the panic and preventing anything smaller from overflowing it. Invalid timestamps greater than this window, ex (exp = 6, window = 5) would succeed this patch check but should still fail the expiration checks that follow:

exp - options.reject_tokens_expiring_in_less_than < now - options.leeway

This just seemed much simpler. I welcome your thoughts.

Keats added a commit that referenced this issue May 27, 2024
@Keats Keats linked a pull request May 27, 2024 that will close this issue
@Keats
Copy link
Owner

Keats commented May 27, 2024

Your initial patch is much simpler and works, let's go with that ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants