diff --git a/django/core/cache/__init__.py b/django/core/cache/__init__.py index 735b83e94f..ed72fe88e1 100644 --- a/django/core/cache/__init__.py +++ b/django/core/cache/__init__.py @@ -17,13 +17,13 @@ from asgiref.local import Local from django.conf import settings from django.core import signals from django.core.cache.backends.base import ( - BaseCache, CacheKeyWarning, InvalidCacheBackendError, + BaseCache, CacheKeyWarning, InvalidCacheBackendError, InvalidCacheKey, ) from django.utils.module_loading import import_string __all__ = [ 'cache', 'caches', 'DEFAULT_CACHE_ALIAS', 'InvalidCacheBackendError', - 'CacheKeyWarning', 'BaseCache', + 'CacheKeyWarning', 'BaseCache', 'InvalidCacheKey', ] DEFAULT_CACHE_ALIAS = 'default' diff --git a/django/core/cache/backends/base.py b/django/core/cache/backends/base.py index cf9629bc03..8322172fc1 100644 --- a/django/core/cache/backends/base.py +++ b/django/core/cache/backends/base.py @@ -14,6 +14,10 @@ class CacheKeyWarning(RuntimeWarning): pass +class InvalidCacheKey(ValueError): + pass + + # Stub class to ensure not passing in a `timeout` argument results in # the default timeout DEFAULT_TIMEOUT = object() @@ -242,18 +246,8 @@ class BaseCache: backend. This encourages (but does not force) writing backend-portable cache code. """ - if len(key) > MEMCACHE_MAX_KEY_LENGTH: - warnings.warn( - 'Cache key will cause errors if used with memcached: %r ' - '(longer than %s)' % (key, MEMCACHE_MAX_KEY_LENGTH), CacheKeyWarning - ) - for char in key: - if ord(char) < 33 or ord(char) == 127: - warnings.warn( - 'Cache key contains characters that will cause errors if ' - 'used with memcached: %r' % key, CacheKeyWarning - ) - break + for warning in memcache_key_warnings(key): + warnings.warn(warning, CacheKeyWarning) def incr_version(self, key, delta=1, version=None): """ @@ -281,3 +275,18 @@ class BaseCache: def close(self, **kwargs): """Close the cache connection""" pass + + +def memcache_key_warnings(key): + if len(key) > MEMCACHE_MAX_KEY_LENGTH: + yield ( + 'Cache key will cause errors if used with memcached: %r ' + '(longer than %s)' % (key, MEMCACHE_MAX_KEY_LENGTH) + ) + for char in key: + if ord(char) < 33 or ord(char) == 127: + yield ( + 'Cache key contains characters that will cause errors if ' + 'used with memcached: %r' % key, CacheKeyWarning + ) + break diff --git a/django/core/cache/backends/memcached.py b/django/core/cache/backends/memcached.py index 8202005045..cd0a6984ec 100644 --- a/django/core/cache/backends/memcached.py +++ b/django/core/cache/backends/memcached.py @@ -4,7 +4,9 @@ import pickle import re import time -from django.core.cache.backends.base import DEFAULT_TIMEOUT, BaseCache +from django.core.cache.backends.base import ( + DEFAULT_TIMEOUT, BaseCache, InvalidCacheKey, memcache_key_warnings, +) from django.utils.functional import cached_property @@ -64,24 +66,30 @@ class BaseMemcachedCache(BaseCache): def add(self, key, value, timeout=DEFAULT_TIMEOUT, version=None): key = self.make_key(key, version=version) + self.validate_key(key) return self._cache.add(key, value, self.get_backend_timeout(timeout)) def get(self, key, default=None, version=None): key = self.make_key(key, version=version) + self.validate_key(key) return self._cache.get(key, default) def set(self, key, value, timeout=DEFAULT_TIMEOUT, version=None): key = self.make_key(key, version=version) + self.validate_key(key) if not self._cache.set(key, value, self.get_backend_timeout(timeout)): # make sure the key doesn't keep its old value in case of failure to set (memcached's 1MB limit) self._cache.delete(key) def delete(self, key, version=None): key = self.make_key(key, version=version) + self.validate_key(key) return bool(self._cache.delete(key)) def get_many(self, keys, version=None): key_map = {self.make_key(key, version=version): key for key in keys} + for key in key_map: + self.validate_key(key) ret = self._cache.get_multi(key_map.keys()) return {key_map[k]: v for k, v in ret.items()} @@ -91,6 +99,7 @@ class BaseMemcachedCache(BaseCache): def incr(self, key, delta=1, version=None): key = self.make_key(key, version=version) + self.validate_key(key) # memcached doesn't support a negative delta if delta < 0: return self._cache.decr(key, -delta) @@ -109,6 +118,7 @@ class BaseMemcachedCache(BaseCache): def decr(self, key, delta=1, version=None): key = self.make_key(key, version=version) + self.validate_key(key) # memcached doesn't support a negative delta if delta < 0: return self._cache.incr(key, -delta) @@ -130,6 +140,7 @@ class BaseMemcachedCache(BaseCache): original_keys = {} for key, value in data.items(): safe_key = self.make_key(key, version=version) + self.validate_key(safe_key) safe_data[safe_key] = value original_keys[safe_key] = key failed_keys = self._cache.set_multi(safe_data, self.get_backend_timeout(timeout)) @@ -141,6 +152,10 @@ class BaseMemcachedCache(BaseCache): def clear(self): self._cache.flush_all() + def validate_key(self, key): + for warning in memcache_key_warnings(key): + raise InvalidCacheKey(warning) + class MemcachedCache(BaseMemcachedCache): "An implementation of a cache binding using python-memcached" diff --git a/docs/releases/2.2.13.txt b/docs/releases/2.2.13.txt index ee381fdcce..3e455e7b4a 100644 --- a/docs/releases/2.2.13.txt +++ b/docs/releases/2.2.13.txt @@ -6,6 +6,14 @@ Django 2.2.13 release notes Django 2.2.13 fixes two security issues and a regression in 2.2.12. +CVE-2020-13254: Potential data leakage via malformed memcached keys +=================================================================== + +In cases where a memcached backend does not perform key validation, passing +malformed cache keys could result in a key collision, and potential data +leakage. In order to avoid this vulnerability, key validation is added to the +memcached cache backends. + CVE-2020-13596: Possible XSS via admin ``ForeignKeyRawIdWidget`` ================================================================ diff --git a/docs/releases/3.0.7.txt b/docs/releases/3.0.7.txt index 51ac0d7edd..5a608a35e4 100644 --- a/docs/releases/3.0.7.txt +++ b/docs/releases/3.0.7.txt @@ -6,6 +6,14 @@ Django 3.0.7 release notes Django 3.0.7 fixes two security issues and several bugs in 3.0.6. +CVE-2020-13254: Potential data leakage via malformed memcached keys +=================================================================== + +In cases where a memcached backend does not perform key validation, passing +malformed cache keys could result in a key collision, and potential data +leakage. In order to avoid this vulnerability, key validation is added to the +memcached cache backends. + CVE-2020-13596: Possible XSS via admin ``ForeignKeyRawIdWidget`` ================================================================ diff --git a/tests/cache/tests.py b/tests/cache/tests.py index 8e8901e628..c5f1ad6876 100644 --- a/tests/cache/tests.py +++ b/tests/cache/tests.py @@ -16,7 +16,7 @@ from unittest import mock from django.conf import settings from django.core import management, signals from django.core.cache import ( - DEFAULT_CACHE_ALIAS, CacheKeyWarning, cache, caches, + DEFAULT_CACHE_ALIAS, CacheKeyWarning, InvalidCacheKey, cache, caches, ) from django.core.cache.backends.base import InvalidCacheBackendError from django.core.cache.utils import make_template_fragment_key @@ -623,10 +623,10 @@ class BaseCacheTests: def _perform_invalid_key_test(self, key, expected_warning): """ - All the builtin backends (except memcached, see below) should warn on - keys that would be refused by memcached. This encourages portable - caching code without making it too difficult to use production backends - with more liberal key rules. Refs #6447. + All the builtin backends should warn (except memcached that should + error) on keys that would be refused by memcached. This encourages + portable caching code without making it too difficult to use production + backends with more liberal key rules. Refs #6447. """ # mimic custom ``make_key`` method being defined since the default will # never show the below warnings @@ -1270,24 +1270,14 @@ class BaseMemcachedTests(BaseCacheTests): with self.settings(CACHES={'default': params}): self.assertEqual(cache._servers, ['server1.tld', 'server2:11211']) - def test_invalid_key_characters(self): + def _perform_invalid_key_test(self, key, expected_warning): """ - On memcached, we don't introduce a duplicate key validation - step (for speed reasons), we just let the memcached API - library raise its own exception on bad keys. Refs #6447. - - In order to be memcached-API-library agnostic, we only assert - that a generic exception of some kind is raised. + Whilst other backends merely warn, memcached should raise for an + invalid key. """ - # memcached does not allow whitespace or control characters in keys - # when using the ascii protocol. - with self.assertRaises(Exception): - cache.set('key with spaces', 'value') - - def test_invalid_key_length(self): - # memcached limits key length to 250 - with self.assertRaises(Exception): - cache.set('a' * 251, 'value') + msg = expected_warning.replace(key, ':1:%s' % key) + with self.assertRaisesMessage(InvalidCacheKey, msg): + cache.set(key, 'value') def test_default_never_expiring_timeout(self): # Regression test for #22845 @@ -1396,15 +1386,6 @@ class PyLibMCCacheTests(BaseMemcachedTests, TestCase): # libmemcached manages its own connections. should_disconnect_on_close = False - # By default, pylibmc/libmemcached don't verify keys client-side and so - # this test triggers a server-side bug that causes later tests to fail - # (#19914). The `verify_keys` behavior option could be set to True (which - # would avoid triggering the server-side bug), however this test would - # still fail due to https://github.com/lericson/pylibmc/issues/219. - @unittest.skip("triggers a memcached-server bug, causing subsequent tests to fail") - def test_invalid_key_characters(self): - pass - @override_settings(CACHES=caches_setting_for_tests( base=PyLibMCCache_params, exclude=memcached_excluded_caches,