Opened 13 years ago

Closed 13 years ago

#16182 closed Bug (wontfix)

TimestampSigner should use a more precise timestamp

Reported by: floguy Owned by: floguy
Component: Core (Other) Version: dev
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 13 years ago.
Increased precision for TimestampSigner
16182-increased-signing-precision-2.diff (3.0 KB ) - added by floguy 13 years ago.
16182-increased-signing-precision-3.diff (3.1 KB ) - added by Andrew Godwin 13 years ago.
Modified patch to use time_func

Download all attachments as: .zip

Change History (13)

by floguy, 13 years ago

Increased precision for TimestampSigner

comment:1 by Ernesto Rico-Schmidt, 13 years ago

Triage Stage: UnreviewedAccepted

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

comment:2 by floguy, 13 years ago

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.

comment:3 by Andrew Godwin, 13 years ago

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 by Jannis Leidel, 13 years ago

Patch needs improvement: set

time_func should it be.

by Andrew Godwin, 13 years ago

Modified patch to use time_func

comment:5 by Andrew Godwin, 13 years ago

Resolution: fixed
Status: newclosed

In [16356]:

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

comment:6 by Paul McMillan, 13 years ago

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 by Jannis Leidel, 13 years ago

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

comment:8 by Andrew Godwin, 13 years ago

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

In [16425]:

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

comment:10 by Andrew Godwin, 13 years ago

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