System.system_time can drift due to time warp and cause tokens to be valid for longer then expected

[Schultzer]

After reading the Phoenix docs, I realized that System.system_time is used to sign and encrypt tokens, and using an unreliable timestamp is a security issue, something I've raised before in this thread regarding guardian: https://elixirforum.com/t/guardian-generating-an-expired-token-because-system-system-time-datetime-is-this-time-warp/56110

I recommend using DateTime.to_unix(DateTime.utc_now()) instead.

[josevalim]

The operation you propose is equivalent to System.os_time and it also can drift, so I am not sure if it is any better. The benefit of system_time is that it amortizes the drift in case it happens?

[Schultzer]

Interesting, why are people experiencing the difference?

It makes sense to rely on the System.os_time instead since it would likely already be synchronized with NTP.

[josevalim]

This comment in the discussion summarizes it well: https://elixirforum.com/t/guardian-generating-an-expired-token-because-system-system-time-datetime-is-this-time-warp/56110/28

I disagree with the description of System.system_time being unreliable but I agree that, in this case, we want to err on the side of the different machines agreeing on a time source sooner than later (i.e. use os_time, no amortization) . I don't see this being an actual issue in production machines, except dev machines going to sleep or on the scenarios described as "poor administration".

I will consult security folks anyway and do a new release if necessary. Thanks!

[Schultzer]

Sounds good; there is also a minor doc issue:

plug_crypto/lib/plug/crypto.ex

Line 199 in 2992c69

Defaults to `System.system_time(:second)`

should be millisecond

plug_crypto/lib/plug/crypto.ex

Line 350 in 2992c69

defp now_ms, do: System.system_time(:millisecond)

.

Depending on the outcome above, I'll hold on pushing a PR.

[josevalim]

Can you please send a PR for the docs anyway? And then we fix os_time in a separate PR. :) Thanks!

[josevalim]

Yes, so it seems there is no issuehere. The drift can be arbitrarily big if the OS clocks lie to us but, if they lie to you, os_time will be wrong anyway. So the question is: in case the clock was wrong, would rather have large time jumps or correct over time?

I can't choose one answer here and, given the fact OTP 26 has better warp modes (with warnings) and the lack of evidence this is actually an issue, I will err on the side of no change for now. Glad to change my mind if there is a clear example of this being harmful over os_time. Thanks!

[Schultzer]

I'm afraid I have to disagree. As far as I know, then system_time is not synchronized with NTP, which os_time is very likely to be on all modern OSs. Changing the timezone is a moot point, in my opinion, since the timestamp is UTC.

To be clear, this is about using a synchronized clock, ideally NTP.

Furthermore, this also differs from how phx.gen.auth deals with timestamps.

[josevalim]

As far as I know, then system_time is not synchronized with NTP, which os_time is very likely to be on all modern OSs.

system_time is synchronized with NTP unless you have large gaps on os_time, which has been my argument.

[josevalim]

And can you please expand the phx.gen.auth bits?

[Schultzer]

As far as I know, then system_time is not synchronized with NTP, which os_time is very likely to be on all modern OSs.

system_time is synchronized with NTP unless you have large gaps on os_time, which has been my argument.

Is it continuously synchronized, or could system_time diverge from the OS?

phx.gen.auth uses DateTime to timestamp session tokens.

[josevalim]

Alright, being consistent is probably the most tangible benefit at this point, so I will go ahead with the changes.

[josevalim]

Done, 2.1.0 is out, thank you.