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

TimedSerializer accepts timestamps from the future #126

Closed
dimaqq opened this issue Feb 7, 2019 · 2 comments · Fixed by #133
Closed

TimedSerializer accepts timestamps from the future #126

dimaqq opened this issue Feb 7, 2019 · 2 comments · Fixed by #133

Comments

@dimaqq
Copy link

dimaqq commented Feb 7, 2019

My team too was "hit" by #120 not having read the change log carefully enough.

Digging deeper, a colleague discovered that while (signs) 0.24 -> 1.1.0 (verifies) path is broken / requires a work-around, the 1.1.0 -> 0.24 path still kinda works.

It works by the virtue of 1.1.0 sending much larger timestamps (numerically), which are interpreted as being far in the future by 0.24. Thus true positives (recent messages) work, and false positives appear (1.1.0 outdated messages are interpreted as valid by 0.24).

This is because TimedSerializer/TimestampSigner only validate that message more recent than X.

I have a gut feeling that it's unsafe (although a point was made that since both sender and recipient are under control of same party, this is hard to exploit).

I think that validation should be message in interval [now, now+max_age), perhaps with affordance for clock drift / imperfect time synchronisation it would be [now - ε, now + max_age + ε)

@davidism
Copy link
Member

davidism commented Feb 7, 2019

I don't understand the change you're proposing. Could you provide a concrete example?

@dimaqq
Copy link
Author

dimaqq commented Feb 7, 2019

Current code, https://github.com/pallets/itsdangerous/blob/master/src/itsdangerous/timed.py#L90

            if age > max_age:
                raise SignatureExpired(...)

Proposed:

            if age > max_age or age < 0:
                raise SignatureExpired(...)

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

Successfully merging a pull request may close this issue.

2 participants