#33212: Incorrect cookie parsing by django.http.cookie.parse_cookie
-----------------------------------+--------------------------------------
Reporter: Christos Georgiou | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 3.2
Severity: Normal | Resolution:
Keywords: cookies | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------
Comment (by Collin Anderson):
Hi All,
Author of Django's current `parse_cookie()` code here. I re-wrote
`parse_cookie()` because python's cookie parsing was not matching
browsers, and as you say, parsing mismatches can lead to security issues.
Here's how you can create this non-RFC6265 compliant cookie string using
javascript. You need to be on '/test/' or some url that's not the root
path (/), and have no other cookies set:
{{{
document.cookie = 'django_cookie=good_value'
document.cookie = 'third_party="some_cookie=some_value'
document.cookie = 'django_cookie=bad_value"; path=/'
var expected = 'django_cookie=good_value;
third_party="some_cookie=some_value; django_cookie=bad_value"'
if(document.cookie === expected) { alert('matches')}
}}}
That creates 3 cookies as far as the browser is concerned, and if you look
in the inspector, you can see the 3 cookies listed. This works in Chrome,
Firefox, Safari, Internet Explorer, etc. If you reload the page, the
browser will send the exact `Cookie` header mentioned above to the server.
Now, two of the 3 cookies have the same `django_cookie` name, and Django
has historically always used the _last_ cookie if there are multiple, but
I could see changing that to use the first one instead, as most other
server software seem to look at the _first_ cookie when there are multiple
with the same name.
Here's javascript code to create a case where the "bad" cookie comes
before the "good" cookie:
{{{
document.cookie = 'django_cookie=good_value; path=/'
document.cookie = 'third_party="some_cookie=some_value'
document.cookie = 'django_cookie=bad_value"'
var expected = 'third_party="some_cookie=some_value;
django_cookie=bad_value"; django_cookie=good_value'
if(document.cookie === expected) { alert('matches')}
}}}
Here's how the two cases are parsed with different server software:
`django_cookie=good_value; third_party="some_cookie=some_value;
django_cookie=bad_value"`
PHP: `<?php print_r($_COOKIE); ?>` says `Array ( [django_cookie] =>
good_value [third_party] => "some_cookie=some_value)`
Rails: debug(cookies) says `{"django_cookie"=>"good_value",
"third_party"=>"\"some_cookie=some_value"}`
Node require('cookie').parse() says `{ django_cookie: 'good_value',
third_party: 'some_cookie=some_valu' }`
Node require('cookie-parse').parse() says `{ django_cookie: 'good_value',
third_party: 'some_cookie=some_valu' }`
Django currently says `{'django_cookie': 'bad_value"', 'third_party':
'"some_cookie=some_value'}`
`third_party="some_cookie=some_value; django_cookie=bad_value";
django_cookie=good_value`
PHP: `<?php print_r($_COOKIE); ?>` says `Array ( [third_party] =>
"some_cookie=some_value [django_cookie] => bad_value" )`
Rails debug(cookies) says `{"third_party"=>"\"some_cookie=some_value",
"django_cookie"=>"bad_value\""}`
Node require('cookie').parse() says `{ third_party:
'some_cookie=some_valu', django_cookie: 'bad_value"' }`
Node require('cookie-parse').parse() says `{ third_party:
'some_cookie=some_valu', django_cookie: 'bad_value"' }`
Django currently says `{'third_party': '"some_cookie=some_value',
'django_cookie': 'good_value'}`
So it might make sense to switch Django to using the first value, rather
than the last value, so it matches what other frameworks are doing, as it
currently is the opposite of other frameworks.
Here's how to make `user_id="12345"; invalid_cookie={"k1": "v1", "k2":
"v2"}; user_id="; ' DROP TABLE important_data"` using javascript (browsers
always add a space after semicolon, so I added a space in this case).
Doesn't work in all browsers, but it works in Chrome at least. (Again,
doesn't work on / path, but any other subpath works, and you need to have
no other cookies set)
{{{
document.cookie = 'user_id="12345"'
document.cookie = 'invalid_cookie={"k1": "v1", "k2": "v2"}'
document.cookie = 'user_id="; path=/'
document.cookie = '\' DROP TABLE important_data"; path=/'
var expected = 'user_id="12345"; invalid_cookie={"k1": "v1", "k2": "v2"};
user_id="; \' DROP TABLE important_data"'
if(document.cookie === expected) { alert('matches')} else { alert('no
match: ' + document.cookie) }
}}}
Again, you can use the browser's inspector to see the 4 different cookies.
Note that there's a cookie with a value but no name. When refreshing the
page, the cookie header sent to the server matches `document.cookie`.
Here's how it gets parsed by different frameworks:
PHP `Array ( [user_id] => "12345" [invalid_cookie] => {"k1": "v1", "k2":
"v2"} ['_DROP_TABLE_important_data"] => )`
Rails `{"user_id"=>"\"12345\"", "invalid_cookie"=>"{\"k1\": \"v1\",
\"k2\": \"v2\"}", "' DROP TABLE important_data\""=>nil}`
Node cookie `{ user_id: '12345', invalid_cookie: '{"k1": "v1", "k2":
"v2"}' }`
Node cookie-parse `{ user_id: '12345', invalid_cookie: '{"k1": "v1", "k2":
"v2"}', '\' DROP TABLE important_data"': 'true' }`
Django currently says: `{'user_id': '"', 'invalid_cookie': '{"k1": "v1",
"k2": "v2"}', '': '\' DROP TABLE important_data"'}`
So, again, maybe it makes sense for Django to switch to using the _first_
value for user_id, instead of the 2nd. I also tried to make Django handle
the "cookie with a value but no name" case as close as possible to how
browsers handle it, despite other servers not handing that case.
Would using the first value rather than the last value fix this issue for
you? It seems to me it would solve the specific issue in each of your
examples, as long as the "bad" cookies come after the "good" cookies.
Thanks,
Collin
--
Ticket URL: <https://code.djangoproject.com/ticket/33212#comment:6>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/062.e3c90a442222a40d3a0f4cbb7df90add%40djangoproject.com.
