Code

Opened 6 years ago

Closed 6 years ago

#6751 closed (invalid)

django.contrib.auth.views.login can leak session

Reported by: Yuanchen Zhu Owned by: nobody
Component: contrib.sessions Version: master
Severity: Keywords: auth login set_test_cookie session leak
Cc: jacob Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Suppose django.contrib.auth.views.login is configured at a certain url, say www.test.com/login.

Now if a client visits www.test.com/login (using GET) for the first time, "login" calls "set_test_cookie", and a session database record will be created. Suppose the client has cookie disabled, and it visits www.test.com/login again using GET, a differnt session database record will be created to store the test cookie. This process can go on and the session database will be filled with records containing single test cookies, allowing a DoS attack on the database.

I found this problem when I used the apache benchmark tool "ab" to benchmark the login page of my website. After making 10000 requests using "ab" three times in a row, I was really surprised that the session database now contains 30000 records, each only contains a single test cookie. This is done locally so these 30000 requests finish in < 5 minutes. Thus I imagine that someone with enough network bandwidth can force the creation of >3 million records in the session database of some django powered site in an hour.

I think the best way to fix the problem is to change the implementation of set_test_cookie so that no record is created in the database session. For example, it can just set a cookie based on some hash of the source ip of the request. Later on, can just test if the cookie is returned in a second request using only the source ip of the request.

I am kind of new to web programming, so can people comment on if this is indeed a valid problem/solution, before I try to write a patch.

Attachments (0)

Change History (5)

comment:1 Changed 6 years ago by Yuanchen Zhu <yuanchen.zhu@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Ah sorry my math is off. It should be 300k, not 3 million. But that is still a lot of leaked session records.

comment:2 Changed 6 years ago by jacob

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

set_test_cookie doesn't create a session; it's the session middleware that's creating sessions. There's really no way around this -- if you want to use sessions, you have to give a new session to any browser that doesn't have one. Removing this would break session handling in a major way, and wouldn't actually stop DDoS attacks -- anyone who's gonna hit your server 100 times/second can do damage in other ways, and you need to just detect that and banninate him.

comment:3 Changed 6 years ago by Yuanchen Zhu <yuanchen.zhu@…>

  • Cc jacob added
  • Resolution invalid deleted
  • Status changed from closed to reopened

Right. But the session middleware won't actually hit the database and create a record unless the cookie store is not empty. While I do want to use sessions, I don't want everyone who visits a simple login page, without even logging in, to modify my database, and I want to be able to maybe cache the login page as static content.

You're definitely right that anyone who's gonna hit the server 100 times/sec should just be banned, so I guess this is not anything like a security problem. However, it is still a minor performance bug, since any page that performs a simple cookie test requres hitting the database rather unnecessarily.

What I was proposing was not rewriting the session system, but just change how set_test_cookie and its brothers are implemented, so these test cookies don't hit the database. I will gladly pay the overhead when I am actually using the session system to store some meaningful cookie.

Do you think what I've described in my first post constitute a valid alternative implementation for set_test_cookie? If not, why?

comment:4 Changed 6 years ago by jacob

It's just far too brittle -- source IP doesn't work, nor does any other HTTP header. Why not just switch to a different session backend if you're concerned about sessions in the DB?

comment:5 Changed 6 years ago by Yuanchen Zhu <yuanchen.zhu@…>

  • Resolution set to invalid
  • Status changed from reopened to closed

I see. Good point. Thanks.

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.