8000 [3.0.x] Fixed CVE-2020-13254 -- Enforced cache key validation in memc… · django/django@84b2da5 · GitHub
[go: up one dir, main page]
Skip to content

Commit 84b2da5

Browse files
danpalmercarltongibson
danpalmer
authored and
carltongibson
committed
[3.0.x] Fixed CVE-2020-13254 -- Enforced cache key validation in memcached backends.
1 parent 1f2dd37 commit 84b2da5

File tree

6 files changed

+66
-45
lines changed

6 files changed

+66
-45
lines changed

‎django/core/cache/__init__.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,13 @@
1717
from django.conf import settings
1818
from django.core import signals
1919
from django.core.cache.backends.base import (
20-
BaseCache, CacheKeyWarning, InvalidCacheBackendError,
20+
BaseCache, CacheKeyWarning, InvalidCacheBackendError, InvalidCacheKey,
2121
)
2222
from django.utils.module_loading import import_string
2323

2424
__all__ = [
2525
'cache', 'caches', 'DEFAULT_CACHE_ALIAS', 'InvalidCacheBackendError',
26-
'CacheKeyWarning', 'BaseCache',
26+
'CacheKeyWarning', 'BaseCache', 'InvalidCacheKey',
2727
]
2828

2929
DEFAULT_CACHE_ALIAS = 'default'

‎django/core/cache/backends/base.py

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ class CacheKeyWarning(RuntimeWarning):
1414
pass
1515

1616

17+
class InvalidCacheKey(ValueError):
18+
pass
19+
20+
1721
# Stub class to ensure not passing in a `timeout` argument results in
1822
# the default timeout
1923
DEFAULT_TIMEOUT = object()
@@ -241,18 +245,8 @@ def validate_key(self, key):
241245
backend. This encourages (but does not force) writing backend-portable
242246
cache code.
243247
"""
244-
if len(key) > MEMCACHE_MAX_KEY_LENGTH:
245-
warnings.warn(
246-
'Cache key will cause errors if used with memcached: %r '
247-
'(longer than %s)' % (key, MEMCACHE_MAX_KEY_LENGTH), CacheKeyWarning
248-
)
249-
for char in key:
250-
if ord(char) < 33 or ord(char) == 127:
251-
warnings.warn(
252-
'Cache key contains characters that will cause errors if '
253-
'used with memcached: %r' % key, CacheKeyWarning
254-
)
255-
break
248+
for warning in memcache_key_warnings(key):
249+
warnings.warn(warning, CacheKeyWarning)
256250

257251
def incr_version(self, key, delta=1, version=None):
258252
"""
@@ -280,3 +274,18 @@ def decr_version(self, key, delta=1, version=None):
280274
def close(self, **kwargs):
281275
"""Close the cache connection"""
282276
pass
277+
278+
279+
def memcache_key_warnings(key):
280+
if len(key) > MEMCACHE_MAX_KEY_LENGTH:
281+
yield (
282+
'Cache key will cause errors if used with memcached: %r '
283+
'(longer than %s)' % (key, MEMCACHE_MAX_KEY_LENGTH)
284+
)
285+
for char in key:
286+
if ord(char) < 33 or ord(char) == 127:
287+
yield (
288+
'Cache key contains characters that will cause errors if '
289+
'used with memcached: %r' % key, CacheKeyWarning
290+
)
291+
break

‎django/core/cache/backends/memcached.py

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@
44
import re
55
import time
66

7-
from django.core.cache.backends.base import DEFAULT_TIMEOUT, BaseCache
7+
from django.core.cache.backends.base import (
8+
DEFAULT_TIMEOUT, BaseCache, InvalidCacheKey, memcache_key_warnings,
9+
)
810
from django.utils.functional import cached_property
911

1012

@@ -64,24 +66,30 @@ def get_backend_timeout(self, timeout=DEFAULT_TIMEOUT):
6466

6567
def add(self, key, value, timeout=DEFAULT_TIMEOUT, version=None):
6668
key = self.make_key(key, version=version)
69+
self.validate_key(key)
6770
return self._cache.add(key, value, self.get_backend_timeout(timeout))
6871

6972
def get(self, key, default=None, version=None):
7073
key = self.make_key(key, version=version)
74+
self.validate_key(key)
7175
return self._cache.get(key, default)
7276

7377
def set(self, key, value, timeout=DEFAULT_TIMEOUT, version=None):
7478
key = self.make_key(key, version=version)
79+
self.validate_key(key)
7580
if not self._cache.set(key, value, self.get_backend_timeout(timeout)):
7681
# make sure the key doesn't keep its old value in case of failure to set (memcached's 1MB limit)
7782
self._cache.delete(key)
7883

7984
def delete(self, key, version=None):
8085
key = self.make_key(key, version=version)
86+
self.validate_key(key)
8187
self._cache.delete(key)
8288

8389
def get_many(self, keys, version=None):
8490
key_map = {self.make_key(key, version=version): key for key in keys}
91+
for key in key_map:
92+
self.validate_key(key)
8593
ret = self._cache.get_multi(key_map.keys())
8694
return {key_map[k]: v for k, v in ret.items()}
8795

@@ -91,6 +99,7 @@ def close(self, **kwargs):
9199

92100
def incr(self, key, delta=1, version=None):
93101
key = self.make_key(key, version=version)
102+
self.validate_key(key)
94103
# memcached doesn't support a negative delta
95104
if delta < 0:
96105
return self._cache.decr(key, -delta)
@@ -109,6 +118,7 @@ def incr(self, key, delta=1, version=None):
109118

110119
def decr(self, key, delta=1, version=None):
111120
key = self.make_key(key, version=version)
121+
self.validate_key(key)
112122
# memcached doesn't support a negative delta
113123
if delta < 0:
114124
return self._cache.incr(key, -delta)
@@ -130,6 +140,7 @@ def set_many(self, data, timeout=DEFAULT_TIMEOUT, version=None):
130140
original_keys = {}
131141
for key, value in data.items():
132142
safe_key = self.make_key(key, version=version)
143+
self.validate_key(safe_key)
133144
safe_data[safe_key] = value
134145
original_keys[safe_key] = key
135146
failed_keys = self._cache.set_multi(safe_data, self.get_backend_timeout(timeout))
@@ -141,6 +152,10 @@ def delete_many(self, keys, version=None):
141152
def clear(self):
142153
self._cache.flush_all()
143154

155+
def validate_key(self, key):
156+
for warning in memcache_key_warnings(key):
157+
raise InvalidCacheKey(warning)
158+
144159

145160
class MemcachedCache(BaseMemcachedCache):
146161
"An implementation of a cache binding using python-memcached"

‎docs/releases/2.2.13.txt

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,14 @@ Django 2.2.13 release notes
66

77
Django 2.2.13 fixes two security issues and a regression in 2.2.12.
88

9+
CVE-2020-13254: Potential data leakage via malformed memcached keys
10+
===================================================================
11+
12+
In cases where a memcached backend does not perform key validation, passing
13+
malformed cache keys could result in a key collision, and potential data
14+
leakage. In order to avoid this vulnerability, key validation is added to the
15+
memcached cache backends.
16+
917
CVE-2020-13596: Possible XSS via admin ``ForeignKeyRawIdWidget``
1018
================================================================
1119

‎docs/releases/3.0.7.txt

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,14 @@ Django 3.0.7 release notes
66

77
Django 3.0.7 fixes two security issues and several bugs in 3.0.6.
88

9+
CVE-2020-13254: Potential data leakage via malformed memcached keys
10+
===================================================================
11+
12+
In cases where a memcached backend does not perform key validation, passing
13+
malformed cache keys could result in a key collision, and potential data
14+
leakage. In order to avoid this vulnerability, key validation is added to the
15+
memcached cache backends.
16+
917
CVE-2020-13596: Possible XSS via admin ``ForeignKeyRawIdWidget``
1018
================================================================
1119

‎tests/cache/tests.py

Lines changed: 11 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
from django.conf import settings
1616
from django.core import management, signals
1717
from django.core.cache import (
18-
DEFAULT_CACHE_ALIAS, CacheKeyWarning, cache, caches,
18+
DEFAULT_CACHE_ALIAS, CacheKeyWarning, InvalidCacheKey, cache, caches,
1919
)
2020
from django.core.cache.utils import make_template_fragment_key
2121
from django.db import close_old_connections, connection, connections
@@ -610,10 +610,10 @@ def test_zero_cull(self):
610610

611611
def _perform_invalid_key_test(self, key, expected_warning):
612612
"""
613-
All the builtin backends (except memcached, see below) should warn on
614-
keys that would be refused by memcached. This encourages portable
615-
caching code without making it too difficult to use production backends
616-
with more liberal key rules. Refs #6447.
613+
All the builtin backends should warn (except memcached that should
614+
error) on keys that would be refused by memcached. This encourages
615+
portable caching code without making it too difficult to use production
616+
backends with more liberal key rules. Refs #6447.
617617
"""
618618
# mimic custom ``make_key`` method being defined since the default will
619619
# never show the below warnings
@@ -1256,24 +1256,14 @@ def test_location_multiple_servers(self):
12561256
with self.settings(CACHES={'default': params}):
12571257
self.assertEqual(cache._servers, ['server1.tld', 'server2:11211'])
12581258

1259-
def test_invalid_key_characters(self):
1259+
def _perform_invalid_key_test(self, key, expected_warning):
12601260
"""
1261-
On memcached, we don't introduce a duplicate key validation
1262-
step (for speed reasons), we just let the memcached API
1263-
library raise its own exception on bad keys. Refs #6447.
1264-
1265-
In order to be memcached-API-library agnostic, we only assert
1266-
that a generic exception of some kind is raised.
1261+
Whilst other backends merely warn, memcached should raise for an
1262+
invalid key.
12671263
"""
1268-
# memcached does not allow whitespace or control characters in keys
1269-
# when using the ascii protocol.
1270-
with self.assertRaises(Exception):
1271-
cache.set('key with spaces', 'value')
1272-
1273-
def test_invalid_key_length(self):
1274-
# memcached limits key length to 250
1275-
with self.assertRaises(Exception):
1276-
cache.set('a' * 251, 'value')
1264+
msg = expected_warning.replace(key, ':1:%s' % key)
1265+
with self.assertRaisesMessage(InvalidCacheKey, msg):
1266+
cache.set(key, 'value')
12771267

12781268
def test_default_never_expiring_timeout(self):
12791269
# Regression test for #22845
@@ -1390,15 +1380,6 @@ class PyLibMCCacheTests(BaseMemcachedTests, TestCase):
13901380
# libmemcached manages its own connections.
13911381
should_disconnect_on_close = False
13921382

1393-
# By default, pylibmc/libmemcached don't verify keys client-side and so
1394-
# this test triggers a server-side bug that causes later tests to fail
1395-
# (#19914). The `verify_keys` behavior option could be set to True (which
1396-
# would avoid triggering the server-side bug), however this test would
1397-
# still fail due to https://github.com/lericson/pylibmc/issues/219.
1398-
@unittest.skip("triggers a memcached-server bug, causing subsequent tests to fail")
1399-
def test_invalid_key_characters(self):
1400-
pass
1401-
14021383
@override_settings(CACHES=caches_setting_for_tests(
14031384
base=PyLibMCCache_params,
14041385
exclude=memcached_excluded_caches,

0 commit comments

Comments
 (0)
0