Code

Opened 2 years ago

Closed 18 months ago

Last modified 18 months ago

#18194 closed Bug (fixed)

File-based session never expire

Reported by: ej Owned by: aaugustin
Component: contrib.sessions Version: master
Severity: Release blocker Keywords:
Cc: crodjer@…, tomas.ehrlich@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Unlike other session backends, session expiry is never checked for file-based session when loading a session. The consequence of this is that, although browser will not send an expired cookie with Django session ID, user can workaround this by modifying his system time so the browser still sends the expired cookie. Since Django doesn't check for the session expiry on the server, this expired session ID is seen as valid to Django.

I am running 1.3.1, but looking at 1.4 source, the bug is still there. Looking at the source code, this loading method should check for session expiry:

def load(self):                                                  
    session_data = {}
    try:
        session_file = open(self._key_to_file(), "rb")
        try:
            file_data = session_file.read()
            # Don't fail if there is no data in the session file.
            # We may have opened the empty placeholder file.
            if file_data:
                try:
                    session_data = self.decode(file_data)
                except (EOFError, SuspiciousOperation):
                    self.create()
        finally:
            session_file.close()
    except IOError:
        self.create()
    return session_data

The bug was first brought up in the mailing list:
https://groups.google.com/forum/#!msg/django-developers/tB_F9rRx7Lg/y70AEdejud8J

Attachments (4)

files-sessions-server-expiry-1.patch (1.1 KB) - added by crodjer 2 years ago.
Patch 1 - Use OS file info to expire sessions
files-sessions-server-expiry-2.patch (3.0 KB) - added by crodjer 2 years ago.
Patch 2 - Added a test to represent the problem statement and a separate function to check file session expiry.
files-sessions-server-expiry-3.patch (3.5 KB) - added by crodjer 2 years ago.
Patch 3 - Use the signing frework to handle session expiry and integrity
sessions-cleanup-files-1.patch (3.1 KB) - added by crodjer 2 years ago.
Cleanup management command support for file based sessions.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 2 years ago by PaulM

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

Changed 2 years ago by crodjer

Patch 1 - Use OS file info to expire sessions

comment:2 Changed 2 years ago by crodjer

Similar to expire_date field in loading from database, the patch - 1 checks the last modify time of the file for age. As far as analogy to updating expire_date is concerned, that will be automatically done with os.write.

Where are the existing tests for contrib.sessions? I was unable to find them for any layer of the app.

comment:3 Changed 2 years ago by PaulM

For contrib apps, the tests are stored alongside the apps themselves. In the case of sessions, that's here:

https://github.com/django/django/blob/master/django/contrib/sessions/tests.py

comment:4 Changed 2 years ago by crodjer

  • Cc crodjer@… added

Changed 2 years ago by crodjer

Patch 2 - Added a test to represent the problem statement and a separate function to check file session expiry.

Changed 2 years ago by crodjer

Patch 3 - Use the signing frework to handle session expiry and integrity

Changed 2 years ago by crodjer

Cleanup management command support for file based sessions.

comment:5 Changed 2 years ago by crodjer

  • Has patch set
  • Owner changed from nobody to crodjer
  • Status changed from new to assigned

I made some attempts with this in file-session-server-expiry-{2,3}.patch. The 2nd uses files modification time to decide on expiry. The 3rd uses the signing framework's timed signer for handling the expiration. Also added a test which is supposed to emulate the ticket behaviour.

Moreover, sessions-cleanup-files-1.patch adds support for cleaning of file sessions with management cleanup command. It moves the session backend specific cleanup logic to their own modules and the management command invokes cleanup command according to the current backend. The base backend implements a cleanup class method which is supposed to be overridden by the backends.

comment:6 Changed 2 years ago by crodjer

  • Owner changed from crodjer to PaulM
  • Status changed from assigned to new

comment:7 follow-up: Changed 18 months ago by Elvard

Is it necessary to sign session data for all backends? I would define it for file-based sessions only. For example, signed-cookie backend cares about signing itself. I don't see any advantage in signing database data with TimestampSigner since expiration date is stored along with session data.

Using modification time seems to me interesting (although simple signing would be useful here), but both solutions (TimestampSigner and modification time) have slight caveat: While in database backend we can specify exact expiration date and check that it's < timezone.now(), here we have modification date (or date of signing) and we check that it's < timezone.now() - SESSION_COOKIE_AGE. It could be solved with setting modification date (or date of signing) in future, but i'm not sure if it's allowed for every file system. TimestampSigner unfortunatelly doesn't support change of time.

Last edited 18 months ago by Elvard (previous) (diff)

comment:8 in reply to: ↑ 7 ; follow-up: Changed 18 months ago by crodjer

Linking this issue to my corresponding pull request over github: https://github.com/django/django/pull/453

Replying to Elvard:

Is it necessary to sign session data for all backends? I would define it for file-based sessions only. For example, signed-cookie backend cares about signing itself. I don't see any advantage in signing database data with TimestampSigner since expiration date is stored along with session data.

This makes sense, considering each backend all of the others have alternate ways of verifying expiry. Putting timed signer only in file session seems more appropriate. It'll also save us from compatibility issues for other backends.

Though in database backend the exist seems faulty. It doesn't check for expiry there. But its out of scope of this ticket.

Using modification time seems to me interesting (although simple signing would be useful here), but both solutions (TimestampSigner and modification time) have slight caveat: While in database backend we can specify exact expiration date and check that it's < timezone.now(), here we have modification date (or date of signing) and we check that it's < timezone.now() - SESSION_COOKIE_AGE. It could be solved with setting modification date (or date of signing) in future, but i'm not sure if it's allowed for every file system.

I also think using file system might be unreliable.

TimestampSigner unfortunatelly doesn't support change of time.

I am not sure if I get this point. As far as I know We can modify the time for TimeStameSigner while unsigning. It'll verify based on the second argument of signer.unsign:

signer.unsign(data, age)

We can have settings.SESSION_COOKIE_AGE in place of age parameter.

comment:9 Changed 18 months ago by aaugustin

Related: #18978

comment:10 Changed 18 months ago by aaugustin

  • Owner changed from PaulM to aaugustin

comment:11 in reply to: ↑ 8 Changed 18 months ago by Elvard

  • Cc tomas.ehrlich@… added
  • Version changed from 1.4 to master

Replying to crodjer:

TimestampSigner unfortunatelly doesn't support change of time.

I am not sure if I get this point. As far as I know We can modify the time for TimeStameSigner while unsigning. It'll verify based on the second argument of signer.unsign:

signer.unsign(data, age)

We can have settings.SESSION_COOKIE_AGE in place of age parameter.

It's probably just a detail. DB backend stores datetime of expiration, while file-based backend stores datetime of signing.

In database backend I can do:

session = SessionBase()
session[...] = ...  # set data
session.set_expiry(arbitraty_time_in_future)
session.save()

Now we have database record with expiration_date set to 'arbitrary_time_in_future' (which doesn't depend on SESSION_COOKIE_AGE).

While in file-based backend, using TimestampSigner, I can't set arbitrary expiration_date independent on SESSION_COOKIE_AGE because I can't store expiration_date along with data. Whole file containing data is signed and current timestamp is stored.

Probably it's not a big issue. I've came across it when I was trying to write unit tests.

comment:12 Changed 18 months ago by aaugustin

I've worked on this ticket today. Here's an analysis of how session expiration is handled by django.contrib.sessions, and specifically by the built-in backends.

Expiration is handled both client-side (by setting cookie expiry) and server-side (because we don't want to keep stale data forever).

This ticket is about synchronizing server-side expiration with client-side expiration, especially for the file backend. Historically Django hasn't paid much attention to server-side expiration.


The default session expiration policy is:

  • client-side: expire after settings.SESSION_COOKIE_AGE seconds if settings.SESSION_EXPIRE_AT_BROWSER_CLOSE = False (default), at browser close otherwise
  • server-side: expire after settings.SESSION_COOKIE_AGE (no matter the value of settings.SESSION_EXPIRE_AT_BROWSER_CLOSE)

When a non-default expiration is set with session.set_expiry(...), it is saved in the session under the _session_expiry key. The semantic of this value is:

  • if it's missing or None: use the default expiration policy described above
  • if it's 0: expire at browser close (client side), after settings.SESSION_COOKIE_AGE (server side)
  • if it's an int: expire in that many seconds (both sides)
  • if it's a datetime: expire at that time (both sides)

This analysis is based on the implementation of SessionMiddleware, session.get_expire_at_browser_close(), session.get_expiry_age() (which controls client-side expiry) and session.get_expiry_date() (which controls server-side expiry).

tl;dr The expiry date of a session is always defined by session.get_expiry_date() / session.get_expiry_age() at the time the session is saved — because it relies on timezone.now() in some cases.


How do the various backends deal with that?

  • cache: the expiry age is computed and passed to the cache engine. The cache engine is responsible for not returning stale data. Since correct expiry is a major feature for a cache, I think we can rely on cache engine to enforce expiry properly. There's no need to clear expired sessions, the cache does it by itself.
  • cached_db: there's a bug here — expiry age is hardcoded to settings.SESSION_COOKIE_AGE instead of session.get_expiry_age(). Otherwise it should work like cache and db. EDIT: fixed in 04b00b668d0d56c37460cbed19671f4b1b5916c3.
  • db: the session expiry date is computed and stored alongside the session data when the session is saved. Only sessions whose expiry dates are in the future can be re-loaded. Sessions whose expiry dates are in the past can be cleared.
  • file: the session expiry date isn't stored. It can be rebuilt with the algorithm of session.get_expiry_age(), by substituting the file's last modification time to timezone.now(). The patches above attempt to do that in order to clear expired sessions.
  • signed_cookies: server-side expiration is provided by timestamping and signing the cookies. However non-default expiry dates aren't handled; the maximum expiration time is hardcoded at settings.SESSION_COOKIE_AGE. There's no need to clear expired sessions because they're stored client-side. Edit: could be fixed in the context of #19201

That was a bit long but necessary to frame this ticket :)

Now there are two actions:
1) Clear expired sessions with the file backend. The patches above are on the right track. This depends on #18978.
2) In the longer term, always store the expiration date in the session and refuse to load an expired session in SessionBase.load. This is already how signed cookies work. It overlaps significantly with _session_expiry as described above. It's a major refactor of django.contrib.sessions that will require backwards-compatibility.

Since 1.5 is now feature-frozen, let's restrict this ticket to 1) and create a new ticket for 2).

Last edited 18 months ago by aaugustin (previous) (diff)

comment:13 Changed 18 months ago by aaugustin

I've created #19200 to track the bug in cached_db and signed_cookies and #19201 for proposal 2).

I'm now going to attempt to fix 1) which is still marked as a release blocker for 1.5.

comment:14 Changed 18 months ago by aaugustin

This branch fixes this ticket as described above: https://github.com/aaugustin/django/compare/session-fixes

Review appreciated :)

comment:15 Changed 18 months ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

comment:16 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

In cd17a24083f3ef17cf4c40a41c9d03c250d817c6:

Added optional kwargs to get_expiry_age/date.

This change allows for cleaner tests: we can test the exact output.

Refs #18194: this change makes it possible to compute session expiry
dates at times other than when the session is saved.

Fixed #18458: the existence of the modification kwarg implies that you
must pass it to get_expiry_age/date if you call these functions outside
of a short request - response cycle (the intended use case).

comment:17 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

In 882c47cd405cfd29194f2e968678a5aa1d6ec75f:

Improved tests introduced in 04b00b6.

These tests are expected to fail for the file session backend because it
doesn't handle expiry properly. They didn't because of an error in the
test setup sequence.

Refs #19200, #18194.

comment:18 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

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

In 5fec97b9df6ea075483276de159e522a29437773:

Fixed #18194 -- Expiration of file-based sessions

  • Prevented stale session files from being loaded
  • Added removal of stale session files in django-admin.py clearsessions

Thanks ej for the report, crodjer and Elvard for their inputs.

comment:19 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

In 845d8408e7aa09a63c98f160b2c6bc9fa6dbb20f:

[1.5.x] Added optional kwargs to get_expiry_age/date.

This change allows for cleaner tests: we can test the exact output.

Refs #18194: this change makes it possible to compute session expiry
dates at times other than when the session is saved.

Fixed #18458: the existence of the modification kwarg implies that you
must pass it to get_expiry_age/date if you call these functions outside
of a short request - response cycle (the intended use case).

Backport of cd17a24 from master.

comment:20 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

In e6b0ee768c46368a41a5b278180d1d1ecbd3d5c6:

[1.5.x] Improved tests introduced in 04b00b6.

These tests are expected to fail for the file session backend because it
doesn't handle expiry properly. They didn't because of an error in the
test setup sequence.

Refs #19200, #18194.

Backport of 882c47c from master.

comment:21 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

In 39082494e62d70be62cde1eb10620548fd62dfb6:

[1.5.x] Fixed #18194 -- Expiration of file-based sessions

  • Prevented stale session files from being loaded
  • Added removal of stale session files in django-admin.py clearsessions

Thanks ej for the report, crodjer and Elvard for their inputs.

Backport of 5fec97b from master.

comment:22 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

In 93e0ec553dae73a2a86cb23b058414d8f9ae3bd6:

[1.5.x] Fixed #19254 -- Bug in SESSION_FILE_PATH handling.

Thanks simonb for the report.

Refs #18194.

Backport of 11fd00c from master.

comment:23 Changed 18 months ago by Aymeric Augustin <aymeric.augustin@…>

In 11fd00c46e2826ce5852e096487762a96909b717:

Fixed #19254 -- Bug in SESSION_FILE_PATH handling.

Thanks simonb for the report.

Refs #18194.

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.