Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#15852 closed Bug (fixed)

Exception when http.parse_cookie recieves bad cookie

Reported by: Fredrik Stålnacke Owned by: nobody
Component: HTTP handling Version: 1.3
Severity: Normal Keywords: parse_cookie
Cc: joonas.kuorilehto@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi!

I'm currently having a problem where the user in some circumstances get a strange cookie (from a sub-domain that I don't control). When this cookie is parsed by django http.parse_cookie an exception occurs that causes the user to get a error message back.

I've managed to reproduce the error with the following code:

from django import cookie
evil_cookie="""test=<#?xml version="1.0" encoding="utf-16"?#><#CookieData xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema"#><#/CookieData#>|<#?xml version="1.0" encoding="utf-16"?#><#CookieData xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema"#><#/CookieData#>"""
http.parse_cookie(evil_cookie)

The error message that are created are:

python2.6/site-packages/django/http/__init__.pyc in parse_cookie(cookie)
    461         try:
    462             c = SimpleCookie()
--> 463             c.load(cookie, ignore_parse_errors=True)
    464         except Cookie.CookieError:
    465             # Invalid cookie


python2.6/site-packages/django/http/__init__.pyc in load(self, rawdata, ignore_parse_errors)
     95                     self.bad_cookies = []
     96                     self._BaseCookie__set = self._loose_set
---> 97                 super(SimpleCookie, self).load(rawdata)
     98                 if ignore_parse_errors:
     99                     self._BaseCookie__set = self._strict_set

/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/Cookie.pyc in load(self, rawdata)
    623         """
    624         if type(rawdata) == type(""):
--> 625             self.__ParseString(rawdata)
    626         else:
    627             self.update(rawdata)

/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/Cookie.pyc in __ParseString(self, str, patt)
    654             else:
    655                 rval, cval = self.value_decode(V)
--> 656                 self.__set(K, rval, cval)
    657                 M = self[K]
    658     # end __ParseString


python2.6/site-packages/django/http/__init__.pyc in _loose_set(self, key, real_value, coded_value)
    105             def _loose_set(self, key, real_value, coded_value):
    106                 try:
--> 107                     self._strict_set(key, real_value, coded_value)
    108                 except Cookie.CookieError:
    109                     self.bad_cookies.append(key)

/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/Cookie.pyc in __set(self, key, real_value, coded_value)
    576         """Private method for setting a cookie's value"""
    577         M = self.get(key, Morsel())
--> 578         M.set(key, real_value, coded_value)
    579         dict.__setitem__(self, key, M)
    580     # end __set


AttributeError: 'NoneType' object has no attribute 'set'

I am using Django 1.3 on Mac OS X with Python 2.6.1

Attachments (2)

patch.diff (461 bytes ) - added by Frank Hoffsümmer 13 years ago.
patched exception handling in parse_cookie method
15852_repeated_bad_cookie_name.diff (1.7 KB ) - added by vung 13 years ago.
Fix failure when a bad cookie name is repeated

Download all attachments as: .zip

Change History (11)

by Frank Hoffsümmer, 13 years ago

Attachment: patch.diff added

patched exception handling in parse_cookie method

comment:1 by Aymeric Augustin, 13 years ago

Easy pickings: unset
Has patch: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

I am not sure about what happens exactly. However, I confirmed that the bug occurs with Django's patched version of SimpleCookie and not with Python's original version.

>>> from Cookie import SimpleCookie
>>> SimpleCookie().load(evil_cookie)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/Cookie.py", line 627, in load
    self.__ParseString(rawdata)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/Cookie.py", line 660, in __ParseString
    self.__set(K, rval, cval)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/Cookie.py", line 580, in __set
    M.set(key, real_value, coded_value)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/Cookie.py", line 455, in set
    raise CookieError("Illegal key value: %s" % key)
Cookie.CookieError: Illegal key value: xmlns:xsi

(that exception would be catched properly in Django).

The patch does not seem appropriate. We should fix the root cause and not mask it by arbitrarily catching an exception.

by vung, 13 years ago

Fix failure when a bad cookie name is repeated

comment:2 by vung, 13 years ago

Needs tests: unset

This is related to #13007.

Here is a short example:

from django import http
http.parse_cookie('a:=b; a:=c; d=e')

The problem is that when a CookieError is raised http.SimpleCookie._loose_set bypasses regular code paths to store a key whose value is None. The normal code path would ensure that the value is a Morsel object.

None works fine when the key occurs only once, so this isn't catched by the test commited in r15523.

When the same key is encountered a second time, though, this value is used in BaseCookie under the assumption that it is a Morsel instance and consequently it has a set() method. Of course, None doesn't have one, hence the bug.

The immediate fix is to use a Morsel instance. It doesn't matter if it supports httponly or not, it will be removed anyway.

Fixing this brings a second problem: bad cookies are colected in a list, to be removed when loading finishes. This will result in calling del self[key] more than once for the same key and will fail.

15852_repeated_bad_key.diff is a short patch that fixes the above.

Last edited 13 years ago by vung (previous) (diff)

comment:3 by Aymeric Augustin, 13 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:4 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: newclosed

In [16452]:

(The changeset message doesn't reference this ticket)

comment:5 by joonas.kuorilehto@…, 12 years ago

Cc: joonas.kuorilehto@… added
Resolution: fixed
Status: closedreopened
UI/UX: unset

I believe this bug has not been fixed in 1.3.X branch. As I have understood bug fixes like this should land into 1.3.X.

I was testing with Codenomicon HTTP Test Suite fuzzer and noticed a very similar traceback in my server logs. I can reproduce this by sending the following minimized HTTP request:

POST / HTTP/1.1
Host: 10.10.3.83
Cookie: = = = = =

{}

This causes a 'NoneType' 'AttributeError' on both Django test server and Apache mod_wsgi.

Traceback with Django 1.3.1:

Traceback (most recent call last):
  File "/var/env/local/lib/python2.7/site-packages/django/core/servers/basehttp.py", line 283, in run
    self.result = application(self.environ, self.start_response)
  File "/var/env/local/lib/python2.7/site-packages/django/contrib/staticfiles/handlers.py", line 68, in __call__
    return self.application(environ, start_response)
  File "/var/env/local/lib/python2.7/site-packages/django/core/handlers/wsgi.py", line 272, in __call__
    response = self.get_response(request)
  File "/var/env/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 169, in get_response
    response = self.handle_uncaught_exception(request, resolver, sys.exc_info())
  File "/var/env/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 218, in handle_uncaught_exception
    return callback(request, **param_dict)
  File "/var/env/local/lib/python2.7/site-packages/django/utils/decorators.py", line 89, in _wrapped_view
    result = middleware.process_view(request, view_func, args, kwargs)
  File "/var/env/local/lib/python2.7/site-packages/django/middleware/csrf.py", line 116, in process_view
    request.META["CSRF_COOKIE"] = _sanitize_token(request.COOKIES[settings.CSRF_COOKIE_NAME])
  File "/var/env/local/lib/python2.7/site-packages/django/core/handlers/wsgi.py", line 218, in _get_cookies
    self._cookies = http.parse_cookie(self.environ.get('HTTP_COOKIE', ''))
  File "/var/env/local/lib/python2.7/site-packages/django/http/__init__.py", line 468, in parse_cookie
    c.load(cookie, ignore_parse_errors=True)
  File "/var/env/local/lib/python2.7/site-packages/django/http/__init__.py", line 97, in load
    super(SimpleCookie, self).load(rawdata)
  File "/usr/lib/python2.7/Cookie.py", line 632, in load
    self.__ParseString(rawdata)
  File "/usr/lib/python2.7/Cookie.py", line 665, in __ParseString
    self.__set(K, rval, cval)
  File "/var/env/local/lib/python2.7/site-packages/django/http/__init__.py", line 107, in _loose_set
    self._strict_set(key, real_value, coded_value)
  File "/usr/lib/python2.7/Cookie.py", line 585, in __set
    M.set(key, real_value, coded_value)
AttributeError: 'NoneType' object has no attribute 'set'

I think this problem is related to this ticket. Either the fix is not included in the 1.3.X branch or the fix is incomplete.

comment:6 by Aymeric Augustin, 12 years ago

Resolution: fixed
Status: reopenedclosed

Until a few months ago, all bugfixes were backported to the stable branch. This policy was recently changed in order to reduce the risk of regressions and the work load for the core team.

The new policy is documented here: https://docs.djangoproject.com/en/dev/internals/release-process/#supported-versions

in reply to:  6 comment:7 by Steven Cummings, 12 years ago

Replying to aaugustin:

Until a few months ago, all bugfixes were backported to the stable branch. This policy was recently changed in order to reduce the risk of regressions and the work load for the core team.

The new policy is documented here: https://docs.djangoproject.com/en/dev/internals/release-process/#supported-versions

So that policy is clear. My follow-up question would be: what makes this "normal", or otherwise not "critical" in severity (which would get it into the 1.3.X branch by your policy).

My perspective on this bug, is that it would be nice if there were no reproducible ways for any kind of website user (web browser, api consumer, etc.) to cause a 500 error. OTOH, I don't have an idea of how many more tickets that would put into the same category to backport.

So, what is the criteria for various severity levels? If that's documented somewhere, I couldn't find it.

comment:8 by Luke Plant, 12 years ago

The criteria are given on the page already linked:

  • Security issues.
  • Data-loss bugs.
  • Crashing bugs.
  • Major functionality bugs in newly-introduced features.

The rule of thumb is that fixes will be backported to the last minor release for bugs that would have prevented a release in the first place.

Having thought about this, it seems that this would fall under 'crashing bugs' and under the last point. This causes crashes, and since this is a regression relative to Python's SimpleCookie, I think I would have marked this as a release blocker had it been found before 1.3 was released.

comment:9 by Aymeric Augustin, 12 years ago

Given Luke's analysis, and since the patch applies easily to the 1.3 branch, I've backported the fix in r17168.

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