From 45a42aabfa1a86d1806bec93b31ef6ed7ccd51a7 Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Thu, 30 Jan 2020 09:28:32 +0000 Subject: [PATCH] Fixed #29708 -- Deprecated PickleSerializer. --- django/contrib/sessions/serializers.py | 1 + django/core/serializers/base.py | 7 +++++++ docs/internals/deprecation.txt | 2 ++ docs/ref/settings.txt | 7 ++----- docs/releases/4.1.txt | 3 +++ docs/topics/http/sessions.txt | 27 +++++++++++++------------- tests/defer_regress/tests.py | 4 +++- tests/serializers/tests.py | 13 ++++++++++++- tests/sessions_tests/tests.py | 10 ++++------ 9 files changed, 48 insertions(+), 26 deletions(-) diff --git a/django/contrib/sessions/serializers.py b/django/contrib/sessions/serializers.py index 5b6c343daa..6a9452bba0 100644 --- a/django/contrib/sessions/serializers.py +++ b/django/contrib/sessions/serializers.py @@ -1,3 +1,4 @@ +# RemovedInDjango50Warning. from django.core.serializers.base import ( PickleSerializer as BasePickleSerializer, ) diff --git a/django/core/serializers/base.py b/django/core/serializers/base.py index 3bd492ec09..45c43a77d6 100644 --- a/django/core/serializers/base.py +++ b/django/core/serializers/base.py @@ -2,10 +2,12 @@ Module for abstract serializer/unserializer base classes. """ import pickle +import warnings from io import StringIO from django.core.exceptions import ObjectDoesNotExist from django.db import models +from django.utils.deprecation import RemovedInDjango50Warning DEFER_FIELD = object() @@ -16,6 +18,11 @@ class PickleSerializer: cache backends. """ def __init__(self, protocol=None): + warnings.warn( + 'PickleSerializer is deprecated due to its security risk. Use ' + 'JSONSerializer instead.', + RemovedInDjango50Warning, + ) self.protocol = pickle.HIGHEST_PROTOCOL if protocol is None else protocol def dumps(self, obj): diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index 4ab7a4135d..23fa942ef9 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -79,6 +79,8 @@ details on these changes. ``SimpleTestCase.assertFormError()`` and ``assertFormsetError()`` will be removed. +* ``django.contrib.sessions.serializers.PickleSerializer`` will be removed. + .. _deprecation-removed-in-4.1: 4.1 diff --git a/docs/ref/settings.txt b/docs/ref/settings.txt index 6c3d7530a1..e65f92b4ff 100644 --- a/docs/ref/settings.txt +++ b/docs/ref/settings.txt @@ -3384,14 +3384,11 @@ sessions won't be created, even if this setting is active. Default: ``'django.contrib.sessions.serializers.JSONSerializer'`` Full import path of a serializer class to use for serializing session data. -Included serializers are: +Included serializer is: -* ``'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`. +See :ref:`session_serialization` for details. Sites ===== diff --git a/docs/releases/4.1.txt b/docs/releases/4.1.txt index 3b36e7ea54..32e816b940 100644 --- a/docs/releases/4.1.txt +++ b/docs/releases/4.1.txt @@ -403,6 +403,9 @@ Miscellaneous * The ``exc_info`` argument of the undocumented ``django.utils.log.log_response()`` function is replaced by ``exception``. +* ``django.contrib.sessions.serializers.PickleSerializer`` is deprecated due to + the risk of remote code execution. + Features removed in 4.1 ======================= diff --git a/docs/topics/http/sessions.txt b/docs/topics/http/sessions.txt index ce92af7b71..20502208a8 100644 --- a/docs/topics/http/sessions.txt +++ b/docs/topics/http/sessions.txt @@ -124,7 +124,7 @@ and the :setting:`SECRET_KEY` setting. .. warning:: **If the SECRET_KEY is not kept secret and you are using the** - :class:`~django.contrib.sessions.serializers.PickleSerializer`, **this can + ``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 @@ -362,19 +362,23 @@ Bundled serializers remote code execution vulnerability if :setting:`SECRET_KEY` becomes known by an attacker. + .. deprecated:: 4.1 + + Due to the risk of remote code execution, this serializer is deprecated + and will be removed in Django 5.0. + .. _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 often straightforward +Note that 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 often straightforward (:class:`~django.core.serializers.json.DjangoJSONEncoder` 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 @@ -664,10 +668,7 @@ Technical details ================= * The session dictionary accepts any :mod:`json` serializable value when using - :class:`~django.contrib.sessions.serializers.JSONSerializer` or any - picklable Python object when using - :class:`~django.contrib.sessions.serializers.PickleSerializer`. See the - :mod:`pickle` module for more information. + :class:`~django.contrib.sessions.serializers.JSONSerializer`. * 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 ccd6556c80..300524a151 100644 --- a/tests/defer_regress/tests.py +++ b/tests/defer_regress/tests.py @@ -4,7 +4,8 @@ from django.contrib.contenttypes.models import ContentType from django.contrib.sessions.backends.db import SessionStore from django.db import models from django.db.models import Count -from django.test import TestCase, override_settings +from django.test import TestCase, ignore_warnings, override_settings +from django.utils.deprecation import RemovedInDjango50Warning from .models import ( Base, Child, Derived, Feature, Item, ItemAndSimpleItem, Leaf, Location, @@ -91,6 +92,7 @@ class DeferRegressionTest(TestCase): list(SimpleItem.objects.annotate(Count('feature')).only('name')), list) + @ignore_warnings(category=RemovedInDjango50Warning) @override_settings(SESSION_SERIALIZER='django.contrib.sessions.serializers.PickleSerializer') def test_ticket_12163(self): # Test for #12163 - Pickling error saving session with unsaved model diff --git a/tests/serializers/tests.py b/tests/serializers/tests.py index 4bd6acd982..1b375a4a1e 100644 --- a/tests/serializers/tests.py +++ b/tests/serializers/tests.py @@ -10,7 +10,8 @@ from django.core.serializers.base import PickleSerializer, ProgressBar from django.db import connection, transaction from django.http import HttpResponse from django.test import SimpleTestCase, override_settings, skipUnlessDBFeature -from django.test.utils import Approximate +from django.test.utils import Approximate, ignore_warnings +from django.utils.deprecation import RemovedInDjango50Warning from .models import ( Actor, Article, Author, AuthorProfile, BaseModel, Category, Child, @@ -420,6 +421,7 @@ class SerializersTransactionTestBase: class PickleSerializerTests(SimpleTestCase): + @ignore_warnings(category=RemovedInDjango50Warning) def test_serializer_protocol(self): serializer = PickleSerializer(protocol=3) self.assertEqual(serializer.protocol, 3) @@ -427,12 +429,21 @@ class PickleSerializerTests(SimpleTestCase): serializer = PickleSerializer() self.assertEqual(serializer.protocol, pickle.HIGHEST_PROTOCOL) + @ignore_warnings(category=RemovedInDjango50Warning) def test_serializer_loads_dumps(self): serializer = PickleSerializer() test_data = 'test data' dump = serializer.dumps(test_data) self.assertEqual(serializer.loads(dump), test_data) + def test_serializer_warning(self): + msg = ( + 'PickleSerializer is deprecated due to its security risk. Use ' + 'JSONSerializer instead.' + ) + with self.assertRaisesMessage(RemovedInDjango50Warning, msg): + PickleSerializer() + def register_tests(test_class, method_name, test_func, exclude=()): """ diff --git a/tests/sessions_tests/tests.py b/tests/sessions_tests/tests.py index 816777bc97..367c40fcc3 100644 --- a/tests/sessions_tests/tests.py +++ b/tests/sessions_tests/tests.py @@ -7,6 +7,7 @@ import unittest from datetime import timedelta from http import cookies from pathlib import Path +from unittest import mock from django.conf import settings from django.contrib.sessions.backends.base import UpdateError @@ -24,9 +25,7 @@ from django.contrib.sessions.exceptions import ( ) from django.contrib.sessions.middleware import SessionMiddleware from django.contrib.sessions.models import Session -from django.contrib.sessions.serializers import ( - JSONSerializer, PickleSerializer, -) +from django.contrib.sessions.serializers import JSONSerializer from django.core import management from django.core.cache import caches from django.core.cache.backends.base import InvalidCacheBackendError @@ -880,9 +879,8 @@ class CookieSessionTests(SessionTestsMixin, SimpleTestCase): # by creating a new session self.assertEqual(self.session.serializer, JSONSerializer) self.session.save() - - self.session.serializer = PickleSerializer - self.session.load() + with mock.patch('django.core.signing.loads', side_effect=ValueError): + self.session.load() @unittest.skip("Cookie backend doesn't have an external store to create records in.") def test_session_load_does_not_create_record(self):