jsonwebtoken icon indicating copy to clipboard operation
jsonwebtoken copied to clipboard

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

Open 0xd-0 opened this issue 1 year ago • 5 comments

    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

0xd-0 avatar May 08 '24 14:05 0xd-0

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

Keats avatar May 08 '24 20:05 Keats

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

0xd-0 avatar May 08 '24 22:05 0xd-0

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

Keats avatar May 18 '24 19:05 Keats

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.

0xd-0 avatar May 19 '24 21:05 0xd-0

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

Keats avatar May 27 '24 19:05 Keats