#19324 closed Bug (fixed)
invalid session keys cause unnecessary empty records in django_session table
Reported by: | Owned by: | nobody | |
---|---|---|---|
Component: | contrib.sessions | Version: | 1.4 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
db session store calls self.create when no record is found for the session key, which causes an empty record inserted. Is this necessary? This gives chance to user to fill the session table with empty records by sending invalid session keys.
is it more appropriate to set session_key to be None in this case?
current implementation:
def load(self): try: s = Session.objects.get( session_key=self.session_key, expire_date__gt=timezone.now() ) return self.decode(s.session_data) except (Session.DoesNotExist, SuspiciousOperation): self.create() return {}
suggested implementation:
def load(self): try: s = Session.objects.get( session_key=self.session_key, expire_date__gt=timezone.now() ) return self.decode(s.session_data) except (Session.DoesNotExist, SuspiciousOperation): self.session_key = None return {}
Change History (7)
comment:1 by , 12 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:2 by , 12 years ago
Triage Stage: | Design decision needed → Accepted |
---|
comment:7 by , 9 years ago
Now the issue has been disclosed, could the logic be very briefly documented in the code itself? Stripped of context of this security issue, not immediately obvious why — for example — only if session_key is None we want to call .create(), etc. etc.
You probably meant:
self._session_key = None
.I don't immediately see how this could allow session fixation attacks — but that doesn't prove anything :)
As is, this change causes two test failures:
This could probably be resolved in
save()
, though.In fact, this change would cause
save()
to be called instead ofcreate()
. Currently the roles of these two functions overlap:save()
even has amust_create
argument! See also #18344.To sum up, the behavior described exists, but it has a very low impact, and even with the proposed change it's easy to cause the cache to fill up. I suspect this ticket should be closed in favor of a ticket describing a refactoring of the sessions API to eliminate the redundancy between
save()
andcreate()
.