From 5f061986b9903b335e4bfe41cf172d710604d5cb Mon Sep 17 00:00:00 2001 From: Tim Graham Date: Wed, 21 Aug 2013 20:12:19 -0400 Subject: [PATCH] [1.6.x] Fixed #20922 -- Allowed customizing the serializer used by contrib.sessions Added settings.SESSION_SERIALIZER which is the import path of a serializer to use for sessions. Thanks apollo13, carljm, shaib, akaariai, charettes, and dstufft for reviews. Backport of b0ce6fe656 from master --- django/conf/global_settings.py | 1 + django/contrib/messages/storage/session.py | 17 +++- django/contrib/messages/tests/base.py | 1 + django/contrib/messages/tests/test_session.py | 4 +- django/contrib/sessions/backends/base.py | 21 ++--- .../sessions/backends/signed_cookies.py | 21 +---- django/contrib/sessions/models.py | 2 +- django/contrib/sessions/serializers.py | 20 ++++ django/contrib/sessions/tests.py | 34 ++++--- docs/ref/settings.txt | 26 +++++- docs/releases/1.6.txt | 23 +++++ docs/topics/http/sessions.txt | 92 +++++++++++++++++-- tests/defer_regress/tests.py | 40 ++++---- 13 files changed, 225 insertions(+), 77 deletions(-) create mode 100644 django/contrib/sessions/serializers.py diff --git a/django/conf/global_settings.py b/django/conf/global_settings.py index 596f4ae78a..68d9ded915 100644 --- a/django/conf/global_settings.py +++ b/django/conf/global_settings.py @@ -469,6 +469,7 @@ SESSION_SAVE_EVERY_REQUEST = False # Whether to save the se SESSION_EXPIRE_AT_BROWSER_CLOSE = False # Whether a user's session cookie expires when the Web browser is closed. SESSION_ENGINE = 'django.contrib.sessions.backends.db' # The module to store session data SESSION_FILE_PATH = None # Directory to store session files if using the file session module. If None, the backend will use a sensible default. +SESSION_SERIALIZER = 'django.contrib.sessions.serializers.JSONSerializer' # class to serialize session data ######### # CACHE # diff --git a/django/contrib/messages/storage/session.py b/django/contrib/messages/storage/session.py index 225dfda289..c3e293c22e 100644 --- a/django/contrib/messages/storage/session.py +++ b/django/contrib/messages/storage/session.py @@ -1,4 +1,8 @@ +import json + from django.contrib.messages.storage.base import BaseStorage +from django.contrib.messages.storage.cookie import MessageEncoder, MessageDecoder +from django.utils import six class SessionStorage(BaseStorage): @@ -20,14 +24,23 @@ class SessionStorage(BaseStorage): always stores everything it is given, so return True for the all_retrieved flag. """ - return self.request.session.get(self.session_key), True + return self.deserialize_messages(self.request.session.get(self.session_key)), True def _store(self, messages, response, *args, **kwargs): """ Stores a list of messages to the request's session. """ if messages: - self.request.session[self.session_key] = messages + self.request.session[self.session_key] = self.serialize_messages(messages) else: self.request.session.pop(self.session_key, None) return [] + + def serialize_messages(self, messages): + encoder = MessageEncoder(separators=(',', ':')) + return encoder.encode(messages) + + def deserialize_messages(self, data): + if data and isinstance(data, six.string_types): + return json.loads(data, cls=MessageDecoder) + return data diff --git a/django/contrib/messages/tests/base.py b/django/contrib/messages/tests/base.py index 0d689729aa..f8a089c335 100644 --- a/django/contrib/messages/tests/base.py +++ b/django/contrib/messages/tests/base.py @@ -60,6 +60,7 @@ class BaseTests(object): MESSAGE_TAGS = '', MESSAGE_STORAGE = '%s.%s' % (self.storage_class.__module__, self.storage_class.__name__), + SESSION_SERIALIZER = 'django.contrib.sessions.serializers.JSONSerializer', ) self.settings_override.enable() diff --git a/django/contrib/messages/tests/test_session.py b/django/contrib/messages/tests/test_session.py index 2ce564b773..940e1c02d0 100644 --- a/django/contrib/messages/tests/test_session.py +++ b/django/contrib/messages/tests/test_session.py @@ -11,13 +11,13 @@ def set_session_data(storage, messages): Sets the messages into the backend request's session and remove the backend's loaded data cache. """ - storage.request.session[storage.session_key] = messages + storage.request.session[storage.session_key] = storage.serialize_messages(messages) if hasattr(storage, '_loaded_data'): del storage._loaded_data def stored_session_messages_count(storage): - data = storage.request.session.get(storage.session_key, []) + data = storage.deserialize_messages(storage.request.session.get(storage.session_key, [])) return len(data) diff --git a/django/contrib/sessions/backends/base.py b/django/contrib/sessions/backends/base.py index 759d7ac7ad..7f5e958a60 100644 --- a/django/contrib/sessions/backends/base.py +++ b/django/contrib/sessions/backends/base.py @@ -3,11 +3,6 @@ from __future__ import unicode_literals import base64 from datetime import datetime, timedelta import logging - -try: - from django.utils.six.moves import cPickle as pickle -except ImportError: - import pickle import string from django.conf import settings @@ -17,6 +12,7 @@ from django.utils.crypto import get_random_string from django.utils.crypto import salted_hmac from django.utils import timezone from django.utils.encoding import force_bytes, force_text +from django.utils.module_loading import import_by_path from django.contrib.sessions.exceptions import SuspiciousSession @@ -42,6 +38,7 @@ class SessionBase(object): self._session_key = session_key self.accessed = False self.modified = False + self.serializer = import_by_path(settings.SESSION_SERIALIZER) def __contains__(self, key): return key in self._session @@ -86,21 +83,21 @@ class SessionBase(object): return salted_hmac(key_salt, value).hexdigest() def encode(self, session_dict): - "Returns the given session dictionary pickled and encoded as a string." - pickled = pickle.dumps(session_dict, pickle.HIGHEST_PROTOCOL) - hash = self._hash(pickled) - return base64.b64encode(hash.encode() + b":" + pickled).decode('ascii') + "Returns the given session dictionary serialized and encoded as a string." + serialized = self.serializer().dumps(session_dict) + hash = self._hash(serialized) + return base64.b64encode(hash.encode() + b":" + serialized).decode('ascii') def decode(self, session_data): encoded_data = base64.b64decode(force_bytes(session_data)) try: # could produce ValueError if there is no ':' - hash, pickled = encoded_data.split(b':', 1) - expected_hash = self._hash(pickled) + hash, serialized = encoded_data.split(b':', 1) + expected_hash = self._hash(serialized) if not constant_time_compare(hash.decode(), expected_hash): raise SuspiciousSession("Session data corrupted") else: - return pickle.loads(pickled) + return self.serializer().loads(serialized) except Exception as e: # ValueError, SuspiciousOperation, unpickling exceptions. If any of # these happen, just return an empty dictionary (an empty session). diff --git a/django/contrib/sessions/backends/signed_cookies.py b/django/contrib/sessions/backends/signed_cookies.py index c2b7a3123f..77a6750ce4 100644 --- a/django/contrib/sessions/backends/signed_cookies.py +++ b/django/contrib/sessions/backends/signed_cookies.py @@ -1,26 +1,9 @@ -try: - from django.utils.six.moves import cPickle as pickle -except ImportError: - import pickle - from django.conf import settings from django.core import signing from django.contrib.sessions.backends.base import SessionBase -class PickleSerializer(object): - """ - Simple wrapper around pickle to be used in signing.dumps and - signing.loads. - """ - def dumps(self, obj): - return pickle.dumps(obj, pickle.HIGHEST_PROTOCOL) - - def loads(self, data): - return pickle.loads(data) - - class SessionStore(SessionBase): def load(self): @@ -31,7 +14,7 @@ class SessionStore(SessionBase): """ try: return signing.loads(self.session_key, - serializer=PickleSerializer, + serializer=self.serializer, # This doesn't handle non-default expiry dates, see #19201 max_age=settings.SESSION_COOKIE_AGE, salt='django.contrib.sessions.backends.signed_cookies') @@ -91,7 +74,7 @@ class SessionStore(SessionBase): session_cache = getattr(self, '_session_cache', {}) return signing.dumps(session_cache, compress=True, salt='django.contrib.sessions.backends.signed_cookies', - serializer=PickleSerializer) + serializer=self.serializer) @classmethod def clear_expired(cls): diff --git a/django/contrib/sessions/models.py b/django/contrib/sessions/models.py index 0179c358b3..3a6e31152f 100644 --- a/django/contrib/sessions/models.py +++ b/django/contrib/sessions/models.py @@ -5,7 +5,7 @@ from django.utils.translation import ugettext_lazy as _ class SessionManager(models.Manager): def encode(self, session_dict): """ - Returns the given session dictionary pickled and encoded as a string. + Returns the given session dictionary serialized and encoded as a string. """ return SessionStore().encode(session_dict) diff --git a/django/contrib/sessions/serializers.py b/django/contrib/sessions/serializers.py new file mode 100644 index 0000000000..92a31c054b --- /dev/null +++ b/django/contrib/sessions/serializers.py @@ -0,0 +1,20 @@ +from django.core.signing import JSONSerializer as BaseJSONSerializer +try: + from django.utils.six.moves import cPickle as pickle +except ImportError: + import pickle + + +class PickleSerializer(object): + """ + Simple wrapper around pickle to be used in signing.dumps and + signing.loads. + """ + def dumps(self, obj): + return pickle.dumps(obj, pickle.HIGHEST_PROTOCOL) + + def loads(self, data): + return pickle.loads(data) + + +JSONSerializer = BaseJSONSerializer diff --git a/django/contrib/sessions/tests.py b/django/contrib/sessions/tests.py index cd8191a6a4..ff0a70c34d 100644 --- a/django/contrib/sessions/tests.py +++ b/django/contrib/sessions/tests.py @@ -285,21 +285,25 @@ class SessionTestsMixin(object): def test_actual_expiry(self): - # Regression test for #19200 - old_session_key = None - new_session_key = None - try: - self.session['foo'] = 'bar' - self.session.set_expiry(-timedelta(seconds=10)) - self.session.save() - old_session_key = self.session.session_key - # With an expiry date in the past, the session expires instantly. - new_session = self.backend(self.session.session_key) - new_session_key = new_session.session_key - self.assertNotIn('foo', new_session) - finally: - self.session.delete(old_session_key) - self.session.delete(new_session_key) + # this doesn't work with JSONSerializer (serializing timedelta) + with override_settings(SESSION_SERIALIZER='django.contrib.sessions.serializers.PickleSerializer'): + self.session = self.backend() # reinitialize after overriding settings + + # Regression test for #19200 + old_session_key = None + new_session_key = None + try: + self.session['foo'] = 'bar' + self.session.set_expiry(-timedelta(seconds=10)) + self.session.save() + old_session_key = self.session.session_key + # With an expiry date in the past, the session expires instantly. + new_session = self.backend(self.session.session_key) + new_session_key = new_session.session_key + self.assertNotIn('foo', new_session) + finally: + self.session.delete(old_session_key) + self.session.delete(new_session_key) class DatabaseSessionTests(SessionTestsMixin, TestCase): diff --git a/docs/ref/settings.txt b/docs/ref/settings.txt index 46482b80a6..a137b229dc 100644 --- a/docs/ref/settings.txt +++ b/docs/ref/settings.txt @@ -2392,7 +2392,7 @@ SESSION_ENGINE Default: ``django.contrib.sessions.backends.db`` -Controls where Django stores session data. Valid values are: +Controls where Django stores session data. Included engines are: * ``'django.contrib.sessions.backends.db'`` * ``'django.contrib.sessions.backends.file'`` @@ -2435,6 +2435,30 @@ Whether to save the session data on every request. If this is ``False`` (default), then the session data will only be saved if it has been modified -- that is, if any of its dictionary values have been assigned or deleted. +.. setting:: SESSION_SERIALIZER + +SESSION_SERIALIZER +------------------ + +.. versionadded:: 1.5.3 + +Default: ``'django.contrib.sessions.serializers.JSONSerializer'`` + +.. versionchanged:: 1.6 + + The default switched from + :class:`~django.contrib.sessions.serializers.PickleSerializer` to + :class:`~django.contrib.sessions.serializers.JSONSerializer` in Django 1.6. + +Full import path of a serializer class to use for serializing session data. +Included serializers are: + +* ``'django.contrib.sessions.serializers.PickleSerializer'`` +* ``'django.contrib.sessions.serializers.JSONSerializer'`` + +See :ref:`session_serialization` for details, including a warning regarding +possible remote code execution when using +:class:`~django.contrib.sessions.serializers.PickleSerializer`. Sites ===== diff --git a/docs/releases/1.6.txt b/docs/releases/1.6.txt index b545cbcd64..105e302b7d 100644 --- a/docs/releases/1.6.txt +++ b/docs/releases/1.6.txt @@ -727,6 +727,29 @@ the ``name`` argument so it doesn't conflict with the new url:: You can remove this url pattern after your app has been deployed with Django 1.6 for :setting:`PASSWORD_RESET_TIMEOUT_DAYS`. +Default session serialization switched to JSON +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Historically, :mod:`django.contrib.sessions` used :mod:`pickle` to serialize +session data before storing it in the backend. If you're using the :ref:`signed +cookie session backend` and :setting:`SECRET_KEY` is +known by an attacker, the attacker could insert a string into his session +which, when unpickled, executes arbitrary code on the server. The technique for +doing so is simple and easily available on the internet. Although the cookie +session storage signs the cookie-stored data to prevent tampering, a +:setting:`SECRET_KEY` leak immediately escalates to a remote code execution +vulnerability. + +This attack can be mitigated by serializing session data using JSON rather +than :mod:`pickle`. To facilitate this, Django 1.5.3 introduced a new setting, +:setting:`SESSION_SERIALIZER`, to customize the session serialization format. +For backwards compatibility, this setting defaulted to using :mod:`pickle` +in Django 1.5.3, but we've changed the default to JSON in 1.6. If you upgrade +and switch from pickle to JSON, sessions created before the upgrade will be +lost. While JSON serialization does not support all Python objects like +:mod:`pickle` does, we highly recommend using JSON-serialized sessions. See the +:ref:`session_serialization` documentation for more details. + Miscellaneous ~~~~~~~~~~~~~ diff --git a/docs/topics/http/sessions.txt b/docs/topics/http/sessions.txt index 772ee122d5..1dd8c2ae3d 100644 --- a/docs/topics/http/sessions.txt +++ b/docs/topics/http/sessions.txt @@ -132,8 +132,9 @@ and the :setting:`SECRET_KEY` setting. .. warning:: - **If the SECRET_KEY is not kept secret, this can lead to arbitrary remote - code execution.** + **If the SECRET_KEY is not kept secret and you are using the** + :class:`~django.contrib.sessions.serializers.PickleSerializer`, **this can + lead to arbitrary remote code execution.** An attacker in possession of the :setting:`SECRET_KEY` can not only generate falsified session data, which your site will trust, but also @@ -260,7 +261,9 @@ You can edit it multiple times. in 5 minutes. * If ``value`` is a ``datetime`` or ``timedelta`` object, the - session will expire at that specific date/time. + session will expire at that specific date/time. Note that ``datetime`` + and ``timedelta`` values are only serializable if you are using the + :class:`~django.contrib.sessions.serializers.PickleSerializer`. * If ``value`` is ``0``, the user's session cookie will expire when the user's Web browser is closed. @@ -307,6 +310,77 @@ You can edit it multiple times. Removes expired sessions from the session store. This class method is called by :djadmin:`clearsessions`. +.. _session_serialization: + +Session serialization +--------------------- + +.. versionadded:: 1.5.3 + + The ability to customize the session serialization format using the + :setting:`SESSION_SERIALIZER` settings was added. + +.. versionchanged:: 1.6 + +Before version 1.6, Django defaulted to using :mod:`pickle` to serialize +session data before storing it in the backend. If you're using the :ref:`signed +cookie session backend` and :setting:`SECRET_KEY` is +known by an attacker, the attacker could insert a string into his session +which, when unpickled, executes arbitrary code on the server. The technique for +doing so is simple and easily available on the internet. Although the cookie +session storage signs the cookie-stored data to prevent tampering, a +:setting:`SECRET_KEY` leak immediately escalates to a remote code execution +vulnerability. + +This attack can be mitigated by serializing session data using JSON rather +than :mod:`pickle`. To facilitate this, Django 1.5.3 introduced a new setting, +:setting:`SESSION_SERIALIZER`, to customize the session serialization format. +For backwards compatibility, this setting defaults to +using :class:`django.contrib.sessions.serializers.PickleSerializer` in +Django 1.5.x, but, for security hardening, defaults to +:class:`django.contrib.sessions.serializers.JSONSerializer` in Django 1.6. +Even with the caveats described in :ref:`custom-serializers`, we highly +recommend sticking with JSON serialization *especially if you are using the +cookie backend*. + +Bundled Serializers +^^^^^^^^^^^^^^^^^^^ + +.. class:: serializers.JSONSerializer + + A wrapper around the JSON serializer from :mod:`django.core.signing`. Can + only serialize basic data types. See the :ref:`custom-serializers` section + for more details. + +.. class:: serializers.PickleSerializer + + Supports arbitrary Python objects, but, as described above, can lead to a + remote code execution vulnerability if :setting:`SECRET_KEY` becomes known + by an attacker. + +.. _custom-serializers: + +Write Your Own Serializer +^^^^^^^^^^^^^^^^^^^^^^^^^ + +Note that unlike :class:`~django.contrib.sessions.serializers.PickleSerializer`, +the :class:`~django.contrib.sessions.serializers.JSONSerializer` cannot handle +arbitrary Python data types. As is often the case, there is a trade-off between +convenience and security. If you wish to store more advanced data types +including ``datetime`` and ``Decimal`` in JSON backed sessions, you will need +to write a custom serializer (or convert such values to a JSON serializable +object before storing them in ``request.session``). While serializing these +values is fairly straightforward +(``django.core.serializers.json.DateTimeAwareJSONEncoder`` may be helpful), +writing a decoder that can reliably get back the same thing that you put in is +more fragile. For example, you run the risk of returning a ``datetime`` that +was actually a string that just happened to be in the same format chosen for +``datetime``\s). + +Your serializer class must implement two methods, +``dumps(self, obj)`` and ``loads(self, data)``, to serialize and deserialize +the dictionary of session data, respectively. + Session object guidelines ------------------------- @@ -396,14 +470,15 @@ An API is available to manipulate session data outside of a view:: >>> from django.contrib.sessions.backends.db import SessionStore >>> import datetime >>> s = SessionStore() - >>> s['last_login'] = datetime.datetime(2005, 8, 20, 13, 35, 10) + >>> # stored as seconds since epoch since datetimes are not serializable in JSON. + >>> s['last_login'] = 1376587691 >>> s.save() >>> s.session_key '2b1189a188b44ad18c35e113ac6ceead' >>> s = SessionStore(session_key='2b1189a188b44ad18c35e113ac6ceead') >>> s['last_login'] - datetime.datetime(2005, 8, 20, 13, 35, 0) + 1376587691 In order to prevent session fixation attacks, sessions keys that don't exist are regenerated:: @@ -551,8 +626,11 @@ behavior: Technical details ================= -* The session dictionary should accept any pickleable Python object. See - the :mod:`pickle` module for more information. +* The session dictionary accepts any :mod:`json` serializable value when using + :class:`~django.contrib.sessions.serializers.JSONSerializer` or any + pickleable Python object when using + :class:`~django.contrib.sessions.serializers.PickleSerializer`. See the + :mod:`pickle` module for more information. * Session data is stored in a database table named ``django_session`` . diff --git a/tests/defer_regress/tests.py b/tests/defer_regress/tests.py index ad2546794c..e8690f1c22 100644 --- a/tests/defer_regress/tests.py +++ b/tests/defer_regress/tests.py @@ -7,6 +7,7 @@ from django.contrib.sessions.backends.db import SessionStore from django.db.models import Count from django.db.models.loading import cache from django.test import TestCase +from django.test.utils import override_settings from .models import ( ResolveThis, Item, RelatedItem, Child, Leaf, Proxy, SimpleItem, Feature, @@ -83,24 +84,6 @@ class DeferRegressionTest(TestCase): self.assertEqual(results[0].child.name, "c1") self.assertEqual(results[0].second_child.name, "c2") - # Test for #12163 - Pickling error saving session with unsaved model - # instances. - SESSION_KEY = '2b1189a188b44ad18c35e1baac6ceead' - - item = Item() - item._deferred = False - s = SessionStore(SESSION_KEY) - s.clear() - s["item"] = item - s.save() - - s = SessionStore(SESSION_KEY) - s.modified = True - s.save() - - i2 = s["item"] - self.assertFalse(i2._deferred) - # Regression for #16409 - make sure defer() and only() work with annotate() self.assertIsInstance( list(SimpleItem.objects.annotate(Count('feature')).defer('name')), @@ -147,6 +130,27 @@ class DeferRegressionTest(TestCase): cache.get_app("defer_regress"), include_deferred=True)) ) + @override_settings(SESSION_SERIALIZER='django.contrib.sessions.serializers.PickleSerializer') + def test_ticket_12163(self): + # Test for #12163 - Pickling error saving session with unsaved model + # instances. + SESSION_KEY = '2b1189a188b44ad18c35e1baac6ceead' + + item = Item() + item._deferred = False + s = SessionStore(SESSION_KEY) + s.clear() + s["item"] = item + s.save() + + s = SessionStore(SESSION_KEY) + s.modified = True + s.save() + + i2 = s["item"] + self.assertFalse(i2._deferred) + + def test_ticket_16409(self): # Regression for #16409 - make sure defer() and only() work with annotate() self.assertIsInstance( list(SimpleItem.objects.annotate(Count('feature')).defer('name')),