[Django] #24915: Stricter validation on session key
Django
------------------------------------------------+------------------------
Reporter: erikr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.sessions | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
Recently we had a vulnerability where session keys were set to empty
strings: https://www.djangoproject.com/weblog/2015/may/20/security-
release/
I can't really imagine any case where setting empty strings as session
keys is a sensible thing to do. I therefore think we should add some
basic validation on the key. Perhaps we should have a minimum length of
5-8 characters, because it would be just as problematic if it were only
one or two characters. This doesn't make it impossible to have weak
session keys, but it is a very basic hardening that would protect us from
such a bug in the future.
Without having looked at the code, my first idea is that this belongs in
the session backends somewhere. This breaks backwards compatibility, but
given the rationale I think a mention in the release notes is sufficient.
--
Ticket URL: <https://code.djangoproject.com/ticket/24915>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: erikr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.sessions | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sevenearths):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24915#comment:1>
Django
Reporter: erikr | Owner: sp1ky
Type: Cleanup/optimization | Status: assigned
Component: contrib.sessions | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sp1ky):
* status: new => assigned
* owner: nobody => sp1ky
--
Ticket URL: <https://code.djangoproject.com/ticket/24915#comment:2>
Django
Reporter: erikr | Owner: sp1ky
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sp1ky):
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/4807
Changed _session_key attribute to a property and implemented basic
validation in the setter. The session key must be 'truthy' and
at least 8 characters long. Otherwise, the value is set to None
--
Ticket URL: <https://code.djangoproject.com/ticket/24915#comment:3>
Django
Reporter: erikr | Owner: sp1ky
Type: | Status: assigned
Cleanup/optimization |
Component: contrib.sessions | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24915#comment:4>
Django
Reporter: erikr | Owner: sp1ky
Cleanup/optimization |
Component: contrib.sessions | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f4416b1a8b92e492707a6261b7a1132f8550457f" f4416b1]:
{{{
#!CommitTicketReference repository=""
revision="f4416b1a8b92e492707a6261b7a1132f8550457f"
Fixed #24915 -- Added stricter session key validation
Changed _session_key attribute to a property and implemented basic
validation in the setter. The session key must be 'truthy' and
at least 8 characters long. Otherwise, the value is set to None.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24915#comment:5>
Django
Reporter: erikr | Owner: sp1ky
Type: | Status: closed
Cleanup/optimization |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by erikr):
@sp1ky: thanks for the patch. This patch might qualify under the Google
Patch Rewards programme: http://www.google.com/about/appsecurity/patch-
rewards/
As the patch author, you can try to submit it.
A patch only qualifies once it has been shipped in a release, so you will
have to wait with your submission until 1.9 is actually released. If you
do submit, I'd make sure to mention the blogpost about the vulnerability,
as it shows this is actually a real potential problem that we've now, at
least partially, tackled for the future.
--
Ticket URL: <https://code.djangoproject.com/ticket/24915#comment:6>
Django
Reporter: erikr | Owner: sp1ky
Type: | Status: closed
Cleanup/optimization |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
In my experience you don't need to wait until the release ships to submit
to Google.
--
Ticket URL: <https://code.djangoproject.com/ticket/24915#comment:7>
Django
Reporter: erikr | Owner: sp1ky
Type: | Status: closed
Cleanup/optimization |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by sp1ky):
Cool, thanks for the tip @timgraham, @erikr. I'll fire a message off to
the Patch Rewards program and see what happens!
--
Ticket URL: <https://code.djangoproject.com/ticket/24915#comment:8>