Opened 6 years ago

Closed 5 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 6 years ago.
Increased precision for TimestampSigner
16182-increased-signing-precision-2.diff (3.0 KB) - added by floguy 6 years ago.
16182-increased-signing-precision-3.diff (3.1 KB) - added by Andrew Godwin 6 years ago.
Modified patch to use time_func

Download all attachments as: .zip

Change History (13)

Changed 6 years ago by floguy

Increased precision for TimestampSigner

comment:1 Changed 6 years ago by Ernesto Rico-Schmidt

Triage Stage: UnreviewedAccepted

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

comment:2 Changed 6 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 6 years ago by floguy

comment:3 Changed 6 years ago by Andrew Godwin

Triage Stage: AcceptedReady 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 6 years ago by Jannis Leidel

Patch needs improvement: set

time_func should it be.

Changed 6 years ago by Andrew Godwin

Modified patch to use time_func

comment:5 Changed 6 years ago by Andrew Godwin

Resolution: fixed
Status: newclosed

In [16356]:

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

comment:6 Changed 5 years ago by Paul McMillan

Patch needs improvement: unset
Resolution: fixed
Status: closedreopened
Triage Stage: Ready for checkinDesign 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 5 years ago by Jannis Leidel

Triage Stage: Design decision neededAccepted

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 5 years ago by Jannis Leidel (previous) (diff)

comment:8 Changed 5 years ago by Andrew Godwin

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 5 years ago by Andrew Godwin

In [16425]:

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

comment:10 Changed 5 years ago by Andrew Godwin

Resolution: wontfix
Status: reopenedclosed

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

Note: See TracTickets for help on using tickets.
Back to Top