Code

Opened 3 years ago

Closed 3 years ago

#16182 closed Bug (wontfix)

TimestampSigner should use a more precise timestamp

Reported by: floguy Owned by: floguy
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Generating a secure cookie using the TimestampSigner within the same second and the same data actually generates precisely the same cookie.

I'm currently developing a cookie-based session backend, and this line keeps failing as a result, because cycling the cookie isn't really cycling the cookie: https://code.djangoproject.com/browser/django/trunk/django/contrib/sessions/tests.py#L153

I've attached a small patch that fixes the problem by increasing the precision.

Attachments (3)

16182-increased-signing-precision.diff (925 bytes) - added by floguy 3 years ago.
Increased precision for TimestampSigner
16182-increased-signing-precision-2.diff (3.0 KB) - added by floguy 3 years ago.
16182-increased-signing-precision-3.diff (3.1 KB) - added by andrewgodwin 3 years ago.
Modified patch to use time_func

Download all attachments as: .zip

Change History (13)

Changed 3 years ago by floguy

Increased precision for TimestampSigner

comment:1 Changed 3 years ago by nnrcschmdt

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Patch looks good, it applies without problem. The tests for signing and sessions pass.

comment:2 Changed 3 years ago by floguy

I've just updated the patch. This new patch adds tests, and actually improves the code's testability in general. Instead of monkeypatching time.time, we pass the time function as an argument to the TimestampSigner class.

Changed 3 years ago by floguy

comment:3 Changed 3 years ago by andrewgodwin

  • Triage Stage changed from Accepted to Ready for checkin

Looks good to me. Only possible improvement could be renaming self.time to self.time_func (to avoid ambiguity), but that's a very minor change and something the committer can decide on, if needed.

comment:4 Changed 3 years ago by jezdez

  • Patch needs improvement set

time_func should it be.

Changed 3 years ago by andrewgodwin

Modified patch to use time_func

comment:5 Changed 3 years ago by andrewgodwin

  • Resolution set to fixed
  • Status changed from new to closed

In [16356]:

Fixed #16182: Increase timestamp precision on TimestampSigner. Thanks to Eric Florenzano.

comment:6 Changed 3 years ago by PaulM

  • Patch needs improvement unset
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Ready for checkin to Design decision needed

I see this is already committed, but I'd like to raise an objection anyway. The timestamp signer was deliberately only using second-level precision. Multiplying by 10,000 significantly increases the length of our sig that we're storing in the already very limited 4095 bytes of the cookie space. We don't want to take up another couple of those when a user can be storing data there.

I'll go into more detail in #16199, but signed cookies like this don't need this functionality for rotation. We're not invalidating any data on the backend, and a previously signed cookie is still technically valid until it expires even if we've rotated it on the users end.

comment:7 Changed 3 years ago by jezdez

  • Triage Stage changed from Design decision needed to Accepted

Indeed, unfortunately I wasn't involved in the discussion during the DjangoCon sprint that led to committing this and I agree with Paul's argumentation. Reverting seems like the only option to me.

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

comment:8 Changed 3 years ago by andrewgodwin

Given the arguments against, I'm inclined to agree; even with the new code, it's possible to make two cookies the same, so we're not eliminating any timing problems, merely reducing their probability (and possibly hiding some nasty bugs).

comment:9 Changed 3 years ago by andrewgodwin

In [16425]:

Reverting [16376] in preparation for reverting [16356]. See #16182.

comment:10 Changed 3 years ago by andrewgodwin

  • Resolution set to wontfix
  • Status changed from reopened to closed

Changeset [16426] reverts this commit, and means I'll re-close this ticket as wontfix.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.