diff --git a/django/core/cache/__init__.py b/django/core/cache/__init__.py index a6b956fdf2f9..f9ce005757f8 100644 --- a/django/core/cache/__init__.py +++ b/django/core/cache/__init__.py @@ -17,13 +17,13 @@ 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 0d0b1ddc8202..70de0a8b709d 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 @@ def validate_key(self, key): 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 @@ def decr_version(self, key, delta=1, version=None): 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 bb27cb53bba4..4a6d510de1ab 100644 --- a/django/core/cache/backends/memcached.py +++ b/django/core/cache/backends/memcached.py @@ -4,7 +4,9 @@ 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,10 +66,12 @@ def get_backend_timeout(self, timeout=DEFAULT_TIMEOUT): 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) val = self._cache.get(key) if val is None: return default @@ -75,16 +79,20 @@ def get(self, key, default=None, version=None): 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) 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()} @@ -94,6 +102,7 @@ def close(self, **kwargs): 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) @@ -112,6 +121,7 @@ def incr(self, key, delta=1, version=None): 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) @@ -133,6 +143,7 @@ def set_many(self, data, timeout=DEFAULT_TIMEOUT, version=None): 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)) @@ -144,6 +155,10 @@ def delete_many(self, keys, version=None): 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 ee381fdccea2..3e455e7b4a5d 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/tests/cache/tests.py b/tests/cache/tests.py index 3fbd05728914..4bc428a98b96 100644 --- a/tests/cache/tests.py +++ b/tests/cache/tests.py @@ -15,7 +15,7 @@ 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.utils import make_template_fragment_key from django.db import close_old_connections, connection, connections @@ -605,10 +605,10 @@ def test_zero_cull(self): 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 @@ -1251,24 +1251,14 @@ def test_location_multiple_servers(self): 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 @@ -1377,15 +1367,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,