Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#53236 new defect (bug)

Nonce lifespans are inaccurate and unintuitively affected by timezones

Reported by: lev0's profile lev0 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 2.5
Component: Date/Time Keywords: has-patch needs-testing needs-unit-tests
Focuses: docs Cc:

Description

The docs on wp_verify_nonce() specify that nonces are either 0-12 or 12-24 hours old by default, but this isn't true. In reality, the value 1 means < 12 hours old, but 2 means anywhere from 1 second to < 24 hours old.

Observe what happens to the nonce tick value over a day:

local timeseconds since epochtick
2021-05-20T00:00:00+03:00162145800037534
2021-05-20T01:00:00+03:00162146160037534
2021-05-20T02:00:00+03:00162146520037534
2021-05-20T03:00:00+03:00162146880037534
2021-05-20T04:00:00+03:00162147240037535
2021-05-20T05:00:00+03:00162147600037535
2021-05-20T06:00:00+03:00162147960037535
2021-05-20T07:00:00+03:00162148320037535
2021-05-20T08:00:00+03:00162148680037535
2021-05-20T09:00:00+03:00162149040037535
2021-05-20T10:00:00+03:00162149400037535
2021-05-20T11:00:00+03:00162149760037535
2021-05-20T12:00:00+03:00162150120037535
2021-05-20T13:00:00+03:00162150480037535
2021-05-20T14:00:00+03:00162150840037535
2021-05-20T15:00:00+03:00162151200037535
2021-05-20T16:00:00+03:00162151560037536
2021-05-20T17:00:00+03:00162151920037536
2021-05-20T18:00:00+03:00162152280037536
2021-05-20T19:00:00+03:00162152640037536
2021-05-20T20:00:00+03:00162153000037536
2021-05-20T21:00:00+03:00162153360037536
2021-05-20T22:00:00+03:00162153720037536
2021-05-20T23:00:00+03:00162154080037536

…and over the boundary of a tick:

local timeseconds since epochtick
2021-05-20T14:59:58+03:0016215119987535
2021-05-20T14:59:59+03:0016215119997535
2021-05-20T15:00:00+03:0016215120007535
2021-05-20T15:00:01+03:0016215120017536
2021-05-20T15:00:02+03:0016215120027536

In this example, you can see that a nonce generated at 3pm and verified one second later will return 2 because of the tick change. The ticks do not align with timezones due to their basis in universal time, so nonces will always appear “old” to your code at certain times of the day, as touched on in ticket:33635#comment:2.

I haven't thought of a way to reduce the huge variance in ages that have equal nonce values, but I did think of a way to make them more predictable. I've attached a patch that would align ticks to WP's timezone so there would be a predictable two ticks per calendar day, a.m. and p.m., starting at 00:00.

Attachments (2)

wp_nonce_tick-align.patch (526 bytes) - added by lev0 3 years ago.
nonce-age-resolution.patch (4.6 KB) - added by lev0 3 years ago.

Download all attachments as: .zip

Change History (12)

#1 @johnbillion
3 years ago

  • Component changed from General to Date/Time
  • Keywords has-patch needs-testing needs-unit-tests added

#2 @Rarst
3 years ago

Ok, so I think the problem is that documentation lies. It says "A nonce is valid for 24 hours" however from quick look the accurate statement would be "A nonce is valid within time tick it was created in and the one following it". So something like at most 24 hours (nonce created at the very start of a tick) and at least 12 hours plus 1 second (nonce created at the very end of a tick).

However I do not follow what aligning nonces to time zone accomplishes. It seems the issue will remain exactly the same, it would only move ticks relatively to UTC time. The generation and check logic would be exactly the same. Plus any weird timezone issues that would drag into it.

I think documentation could be improved to reflect the real logic. So far I do not see the need to change tick generation logic.

And we cannot make nonce last 24 hours without completely changing nonce format and including actual time stamp in readable form into it (which I am guessing we are probably not doing without really good reason to mess with it).

For trivia I don't seem to see a single case of core relying on 1/2 state returned, it's used exclusively as boolean check in practice.

Last edited 3 years ago by Rarst (previous) (diff)

#3 @lev0
3 years ago

I agree this is largely a documentation issue, changing the code to more accurately reflect the documented lifespan could probably only be done by increasing the ticks/nonce_life to a multiple of the current 2. This would give more resolution to the age of the nonce at a cost of verification performance (but only for older nonces). Calculating more hashes could retain the current 1 or 2 return values, e.g. 4 ticks/nonce_life could return 1 if the hash matched the current or previous tick, but 2 if it matched either of the two ticks prior to those.

Aligning the ticks does make sense. By your own assessment, you know a nonce is currently only valid inside 2 ticks. Most people sleep at night and are active during the day so it makes sense to have those ticks inside a local calendar day. This is precisely to reduce the "weird timezone issues" you mentioned. It means nonce expiration is the same for an American user on an American site as it is for an Australian user on an Australian site.

#4 @Rarst
3 years ago

I still disagree that this needs a code change.

1/2 code return seems to be a legacy thing that is not in active use by core.

The very worst case scenario here is that nonce is only valid for 12 hours (instead of maximum possible 24), which I think is still reasonable amount that causes no issues in practice (or we would be permanently swamped in complaints about it).

Could you please elaborate why is it important for your needs to rely on 1/2 return?

#5 @lev0
3 years ago

I care less about the return values than making nonce expiry consistent. Retaining 1 & 2 is purely for backwards compatibility. I'm not in the habit of speculating about how others should or do use WP; citing core usage is almost irrelevant given its popularity and extensibility.

Calendar-aligned ticks likely means fewer expired nonces but is only really helpful if ticks remain 2/day. Finer resolution of ticks obviates the benefit of alignment.

Finer ticks have the advantage of making a nonce's guaranteed minimum lifespan much closer to the expected nonce_life value. The effective formula in use is (n-1)/n, so 2 ticks/life = (2-1)/2 = 1/2 = 12 hours, but increasing the ticks to 6/life (as in my example patch) makes this minimum 5/6 = 20 hours. That's a significant difference as it's then longer than most people are contiguously awake.

#6 @Rarst
3 years ago

Ok, thank you! So in other words the justification for the change could be worded as "Should a minimum possible nonce life time be increased from effective current default of 12 hours?".

As a component maintainer my conservative answer is no, I don't want extra code to deal with, in a legacy unused return, for a change that isn't a practical problem in the wild.

The duration is filterable, so filtering nonce_life to two times desired minimum life time achieves it without code changes.

Would be very interested in security perspective on this from someone qualified. WordPress nonces are already not a real nonces (which would only be used once), longer life time sounds detrimental to their security function.

Last edited 3 years ago by Rarst (previous) (diff)

#7 @lev0
3 years ago

In that case, I think the finer resolution argument still holds. If you've filtered the nonce_life down to 1 hour for security, you don't expect it to fail after 34 minutes, but 51 is more reasonable.

#8 @Rarst
3 years ago

Yes, the current situation is that nonce is guaranteed to be valid for a half of nonce_life. Personally I find that practically acceptable in context of ancient implementation and absence of it being reported as a problem for users with default duration.

To be clear it does irk me plenty that it works in this clunky and unintuitive way! :) But it doesn't irk me enough to start messing with a very old core code that isn't an active problem.

Last edited 3 years ago by Rarst (previous) (diff)

#9 follow-up: @peterwilsoncc
3 years ago

  • Focuses docs added
  • Version set to 2.5

I agree with Rarst that the best solution for WordPress here is a documentation improvement to indicate a nonce is valid for between 12 and 24 hours, and the nonce tick is for the maximum life span rather than the minimum.

I can see arguments either way as to whether the defined nonce tick should be maximum or minimum validity but as the codes been in place for many years, it needs to remain as is.

As the nonce functions are pluggable, the changes proposed in nonce-age-resolution.patch could be released as a plugin but I think they're risky to include in WordPress Core.

I've set the version to 2.5 as that's when wp_nonce_tick() and the related filter were added with the tick being the maximum life span.

#10 in reply to: ↑ 9 @lev0
3 years ago

Replying to peterwilsoncc:

I can see arguments either way as to whether the defined nonce tick should be maximum or minimum validity but as the codes been in place for many years, it needs to remain as is.

That's fair.

As the nonce functions are pluggable, the changes proposed in nonce-age-resolution.patch could be released as a plugin but I think they're risky to include in WordPress Core.

Done.

Note: See TracTickets for help on using tickets.