Opened 8 years ago

Closed 7 years ago

Last modified 5 years ago

#9356 closed (invalid)

CSRFMiddleware should take session key from request.session

Reported by: Bob Thomas Owned by: nobody
Component: Contrib apps Version: 1.0
Severity: Keywords:
Cc: jacob.kaplanmoss@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description

Since CSRFMiddleware depends on SessionMiddleware, it should use request.session to get the session key. That way, SessionMiddleware can ensure that there is a valid session ID present before the CSRF hash is generated.

Attachments (2)

csrf_fix.diff (1.0 KB) - added by Bob Thomas 8 years ago.
Get session key from request.session
ticket_9356.2.diff (6.7 KB) - added by Luke Plant 7 years ago.
Updated patch (but please see my comment)

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by Bob Thomas

Attachment: csrf_fix.diff added

Get session key from request.session

comment:1 Changed 8 years ago by Jacob

milestone: 1.1
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 7 years ago by Jacob

Triage Stage: AcceptedDesign decision needed

I actually think there's a good reason for this, but I can't recall why right now. So bumping back to DDN in case someone else can confirm if I'm right or mistaken.

comment:3 Changed 7 years ago by Bob Thomas

Needs tests: set
Patch needs improvement: set
Triage Stage: Design decision neededAccepted

The reasoning behind this was discussed in more detail on #9977, and the patch on that ticket includes the fix for this. Since #9977 is not going to be included in 1.1 (and possibly not ever), either myself or Luke need to make a better patch for this bug based on his improved fix in the #9977 patch.

Also needs tests.

comment:4 Changed 7 years ago by Jacob

Ah, OK, I'd remembered the discussion but not the conclusion. Thanks.

comment:5 Changed 7 years ago by Luke Plant

I've coded up a patch for this, extracting a bit of an early patch from #9977, but I'm not sure why we really need this. I'm particularly concerned because the patch would add a database hit (read) to every request (two for POST requests), which seems unavoidable unless we want to mess around with internals of the SessionStore object.

The middleware currently uses values from cookies (incoming and outgoing), hence avoiding database hits. If the incoming cookie is not a valid session, then:

  • Incoming checks: CsrfViewMiddleware will check the session cookie is accompanied by a CSRF token. The only time this will fail is if the user has manually messed around with the session cookie. Otherwise the middleware is harmlessly checking that an expired session is matched by an unneeded CSRF token.


  • Outgoing processing: as far as the session framework is concerned, there is no session. Any access to the session during the view function will create an outgoing cookie, which the CSRF middleware will notice, which will then do the right thing. If there is no session created, then CsrfResponseMiddleware will harmlessly insert a token that corresponds to the session cookie the user has (which is being ignored by Django).

What real world problem is this bug about? If there isn't one, I'm not inclined to add a database hit for every request to fix it.

The only other thing this patch would fix is the potential abuse of middleware to generate hashes of (SECRET_KEY + user supplied arbitrary string), which is much more easily addressed by adding some salt in the form of a prefix to the SECRET_KEY. A quick check of instances of SECRET_KEY suggests that this abuse is not currently exploitable in Django, so I don't think we need to address this right now (and when we do, this is not the only bit of code we should fix - we should add unique prefixes to every bit of code that uses SECRET_KEY).

Changed 7 years ago by Luke Plant

Attachment: ticket_9356.2.diff added

Updated patch (but please see my comment)

comment:6 Changed 7 years ago by Jacob

milestone: 1.11.2

I pretty much agree with Luke's comment, and so I'm going to push this to 1.2. There's a good chance that it'll then get closed as a dup of #9977, but that's a move for later.

comment:7 Changed 7 years ago by Luke Plant

Resolution: invalid
Status: newclosed

CsrfMiddleware no longer depends on the session, so this isn't valid any more.

comment:8 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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