Permalink
Browse files

Fixed #19866 -- Added security logger and return 400 for SuspiciousOp…

…eration.

SuspiciousOperations have been differentiated into subclasses, and
are now logged to a 'django.security.*' logger. SuspiciousOperations
that reach django.core.handlers.base.BaseHandler will now return a 400
instead of a 500.

Thanks to tiwoc for the report, and Carl Meyer and Donald Stufft
for review.
  • Loading branch information...
1 parent 36d47f7 commit d228c1192ed59ab0114d9eba82ac99df611652d2 @ptone ptone committed May 15, 2013
Showing with 363 additions and 77 deletions.
  1. +2 −1 django/conf/urls/__init__.py
  2. +6 −0 django/contrib/admin/exceptions.py
  3. +2 −1 django/contrib/admin/views/main.py
  4. +21 −15 django/contrib/auth/tests/test_views.py
  5. +6 −0 django/contrib/formtools/exceptions.py
  6. +2 −2 django/contrib/formtools/wizard/storage/cookie.py
  7. +11 −3 django/contrib/sessions/backends/base.py
  8. +8 −1 django/contrib/sessions/backends/cached_db.py
  9. +8 −2 django/contrib/sessions/backends/db.py
  10. +10 −2 django/contrib/sessions/backends/file.py
  11. +11 −0 django/contrib/sessions/exceptions.py
  12. +16 −4 django/contrib/sessions/tests.py
  13. +27 −7 django/core/exceptions.py
  14. +2 −2 django/core/files/storage.py
  15. +18 −2 django/core/handlers/base.py
  16. +3 −0 django/core/urlresolvers.py
  17. +2 −2 django/http/multipartparser.py
  18. +2 −2 django/http/request.py
  19. +2 −2 django/http/response.py
  20. +20 −0 django/test/utils.py
  21. +5 −0 django/utils/log.py
  22. +15 −0 django/views/defaults.py
  23. +18 −3 docs/ref/exceptions.txt
  24. +7 −0 docs/releases/1.6.txt
  25. +22 −0 docs/topics/http/views.txt
  26. +30 −1 docs/topics/logging.txt
  27. +17 −17 tests/admin_views/tests.py
  28. +9 −0 tests/handlers/tests.py
  29. +1 −0 tests/handlers/urls.py
  30. +4 −0 tests/handlers/views.py
  31. +22 −2 tests/logging_tests/tests.py
  32. +10 −0 tests/logging_tests/urls.py
  33. +11 −0 tests/logging_tests/views.py
  34. +3 −3 tests/test_client_regress/tests.py
  35. +5 −2 tests/test_client_regress/views.py
  36. +3 −1 tests/urlpatterns_reverse/tests.py
  37. +1 −0 tests/urlpatterns_reverse/urls_error_handlers.py
  38. +1 −0 tests/urlpatterns_reverse/urls_error_handlers_callables.py
@@ -5,8 +5,9 @@
from django.utils import six
-__all__ = ['handler403', 'handler404', 'handler500', 'include', 'patterns', 'url']
+__all__ = ['handler400', 'handler403', 'handler404', 'handler500', 'include', 'patterns', 'url']
+handler400 = 'django.views.defaults.bad_request'
handler403 = 'django.views.defaults.permission_denied'
handler404 = 'django.views.defaults.page_not_found'
handler500 = 'django.views.defaults.server_error'
@@ -0,0 +1,6 @@
+from django.core.exceptions import SuspiciousOperation
+
+
+class DisallowedModelAdminLookup(SuspiciousOperation):
+ """Invalid filter was passed to admin view via URL querystring"""
+ pass
@@ -14,6 +14,7 @@
from django.utils.http import urlencode
from django.contrib.admin import FieldListFilter
+from django.contrib.admin.exceptions import DisallowedModelAdminLookup
from django.contrib.admin.options import IncorrectLookupParameters
from django.contrib.admin.util import (quote, get_fields_from_path,
lookup_needs_distinct, prepare_lookup_value)
@@ -128,7 +129,7 @@ def get_filters(self, request):
lookup_params[force_str(key)] = value
if not self.model_admin.lookup_allowed(key, value):
- raise SuspiciousOperation("Filtering by %s not allowed" % key)
+ raise DisallowedModelAdminLookup("Filtering by %s not allowed" % key)
filter_specs = []
if self.list_filter:
@@ -10,15 +10,14 @@
from django.contrib.sites.models import Site, RequestSite
from django.contrib.auth.models import User
from django.core import mail
-from django.core.exceptions import SuspiciousOperation
from django.core.urlresolvers import reverse, NoReverseMatch
from django.http import QueryDict, HttpRequest
from django.utils.encoding import force_text
from django.utils.html import escape
from django.utils.http import urlquote
from django.utils._os import upath
from django.test import TestCase
-from django.test.utils import override_settings
+from django.test.utils import override_settings, patch_logger
from django.middleware.csrf import CsrfViewMiddleware
from django.contrib.sessions.middleware import SessionMiddleware
@@ -155,23 +154,28 @@ def test_poisoned_http_host(self):
# produce a meaningful reset URL, we need to be certain that the
# HTTP_HOST header isn't poisoned. This is done as a check when get_host()
# is invoked, but we check here as a practical consequence.
- with self.assertRaises(SuspiciousOperation):
- self.client.post('/password_reset/',
- {'email': 'staffmember@example.com'},
- HTTP_HOST='www.example:dr.frankenstein@evil.tld'
- )
- self.assertEqual(len(mail.outbox), 0)
+ with patch_logger('django.security.DisallowedHost', 'error') as logger_calls:
+ response = self.client.post('/password_reset/',
+ {'email': 'staffmember@example.com'},
+ HTTP_HOST='www.example:dr.frankenstein@evil.tld'
+ )
+ self.assertEqual(response.status_code, 400)
+ self.assertEqual(len(mail.outbox), 0)
+ self.assertEqual(len(logger_calls), 1)
# Skip any 500 handler action (like sending more mail...)
@override_settings(DEBUG_PROPAGATE_EXCEPTIONS=True)
def test_poisoned_http_host_admin_site(self):
"Poisoned HTTP_HOST headers can't be used for reset emails on admin views"
- with self.assertRaises(SuspiciousOperation):
- self.client.post('/admin_password_reset/',
- {'email': 'staffmember@example.com'},
- HTTP_HOST='www.example:dr.frankenstein@evil.tld'
- )
- self.assertEqual(len(mail.outbox), 0)
+ with patch_logger('django.security.DisallowedHost', 'error') as logger_calls:
+ response = self.client.post('/admin_password_reset/',
+ {'email': 'staffmember@example.com'},
+ HTTP_HOST='www.example:dr.frankenstein@evil.tld'
+ )
+ self.assertEqual(response.status_code, 400)
+ self.assertEqual(len(mail.outbox), 0)
+ self.assertEqual(len(logger_calls), 1)
+
def _test_confirm_start(self):
# Start by creating the email
@@ -678,5 +682,7 @@ def test_changelist_disallows_password_lookups(self):
self.login()
# A lookup that tries to filter on password isn't OK
- with self.assertRaises(SuspiciousOperation):
+ with patch_logger('django.security.DisallowedModelAdminLookup', 'error') as logger_calls:
response = self.client.get('/admin/auth/user/?password__startswith=sha1$')
+ self.assertEqual(response.status_code, 400)
+ self.assertEqual(len(logger_calls), 1)
@@ -0,0 +1,6 @@
+from django.core.exceptions import SuspiciousOperation
+
+
+class WizardViewCookieModified(SuspiciousOperation):
+ """Signature of cookie modified"""
+ pass
@@ -1,8 +1,8 @@
import json
-from django.core.exceptions import SuspiciousOperation
from django.core.signing import BadSignature
+from django.contrib.formtools.exceptions import WizardViewCookieModified
from django.contrib.formtools.wizard import storage
@@ -21,7 +21,7 @@ def load_data(self):
except KeyError:
data = None
except BadSignature:
- raise SuspiciousOperation('WizardView cookie manipulated')
+ raise WizardViewCookieModified('WizardView cookie manipulated')
if data is None:
return None
return json.loads(data, cls=json.JSONDecoder)
@@ -2,6 +2,8 @@
import base64
from datetime import datetime, timedelta
+import logging
+
try:
from django.utils.six.moves import cPickle as pickle
except ImportError:
@@ -14,7 +16,9 @@
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
+from django.utils.encoding import force_bytes, force_text
+
+from django.contrib.sessions.exceptions import SuspiciousSession
# session_key should not be case sensitive because some backends can store it
# on case insensitive file systems.
@@ -94,12 +98,16 @@ def decode(self, session_data):
hash, pickled = encoded_data.split(b':', 1)
expected_hash = self._hash(pickled)
if not constant_time_compare(hash.decode(), expected_hash):
- raise SuspiciousOperation("Session data corrupted")
+ raise SuspiciousSession("Session data corrupted")
else:
return pickle.loads(pickled)
- except Exception:
+ except Exception as e:
# ValueError, SuspiciousOperation, unpickling exceptions. If any of
# these happen, just return an empty dictionary (an empty session).
+ if isinstance(e, SuspiciousOperation):
+ logger = logging.getLogger('django.security.%s' %
+ e.__class__.__name__)
+ logger.warning(force_text(e))
return {}
def update(self, dict_):
@@ -2,10 +2,13 @@
Cached, database-backed sessions.
"""
+import logging
+
from django.contrib.sessions.backends.db import SessionStore as DBStore
from django.core.cache import cache
from django.core.exceptions import SuspiciousOperation
from django.utils import timezone
+from django.utils.encoding import force_text
KEY_PREFIX = "django.contrib.sessions.cached_db"
@@ -41,7 +44,11 @@ def load(self):
data = self.decode(s.session_data)
cache.set(self.cache_key, data,
self.get_expiry_age(expiry=s.expire_date))
- except (Session.DoesNotExist, SuspiciousOperation):
+ except (Session.DoesNotExist, SuspiciousOperation) as e:
+ if isinstance(e, SuspiciousOperation):
+ logger = logging.getLogger('django.security.%s' %
+ e.__class__.__name__)
+ logger.warning(force_text(e))
self.create()
data = {}
return data
@@ -1,8 +1,10 @@
+import logging
+
from django.contrib.sessions.backends.base import SessionBase, CreateError
from django.core.exceptions import SuspiciousOperation
from django.db import IntegrityError, transaction, router
from django.utils import timezone
-
+from django.utils.encoding import force_text
class SessionStore(SessionBase):
"""
@@ -18,7 +20,11 @@ def load(self):
expire_date__gt=timezone.now()
)
return self.decode(s.session_data)
- except (Session.DoesNotExist, SuspiciousOperation):
+ except (Session.DoesNotExist, SuspiciousOperation) as e:
+ if isinstance(e, SuspiciousOperation):
+ logger = logging.getLogger('django.security.%s' %
+ e.__class__.__name__)
+ logger.warning(force_text(e))
self.create()
return {}
@@ -1,5 +1,6 @@
import datetime
import errno
+import logging
import os
import shutil
import tempfile
@@ -8,6 +9,9 @@
from django.contrib.sessions.backends.base import SessionBase, CreateError, VALID_KEY_CHARS
from django.core.exceptions import SuspiciousOperation, ImproperlyConfigured
from django.utils import timezone
+from django.utils.encoding import force_text
+
+from django.contrib.sessions.exceptions import InvalidSessionKey
class SessionStore(SessionBase):
"""
@@ -48,7 +52,7 @@ def _key_to_file(self, session_key=None):
# should always be md5s, so they should never contain directory
# components.
if not set(session_key).issubset(set(VALID_KEY_CHARS)):
- raise SuspiciousOperation(
+ raise InvalidSessionKey(
"Invalid characters in session key")
return os.path.join(self.storage_path, self.file_prefix + session_key)
@@ -75,7 +79,11 @@ def load(self):
if file_data:
try:
session_data = self.decode(file_data)
- except (EOFError, SuspiciousOperation):
+ except (EOFError, SuspiciousOperation) as e:
+ if isinstance(e, SuspiciousOperation):
+ logger = logging.getLogger('django.security.%s' %
+ e.__class__.__name__)
+ logger.warning(force_text(e))
self.create()
# Remove expired sessions.
@@ -0,0 +1,11 @@
+from django.core.exceptions import SuspiciousOperation
+
+
+class InvalidSessionKey(SuspiciousOperation):
+ """Invalid characters in session key"""
+ pass
+
+
+class SuspiciousSession(SuspiciousOperation):
+ """The session may be tampered with"""
+ pass
@@ -1,3 +1,4 @@
+import base64
from datetime import timedelta
import os
import shutil
@@ -15,14 +16,16 @@
from django.contrib.sessions.middleware import SessionMiddleware
from django.core.cache import get_cache
from django.core import management
-from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation
+from django.core.exceptions import ImproperlyConfigured
from django.http import HttpResponse
from django.test import TestCase, RequestFactory
-from django.test.utils import override_settings
+from django.test.utils import override_settings, patch_logger
from django.utils import six
from django.utils import timezone
from django.utils import unittest
+from django.contrib.sessions.exceptions import InvalidSessionKey
+
class SessionTestsMixin(object):
# This does not inherit from TestCase to avoid any tests being run with this
@@ -272,6 +275,15 @@ def test_decode(self):
encoded = self.session.encode(data)
self.assertEqual(self.session.decode(encoded), data)
+ def test_decode_failure_logged_to_security(self):
+ bad_encode = base64.b64encode(b'flaskdj:alkdjf')
+ with patch_logger('django.security.SuspiciousSession', 'warning') as calls:
+ self.assertEqual({}, self.session.decode(bad_encode))
+ # check that the failed decode is logged
+ self.assertEqual(len(calls), 1)
+ self.assertTrue('corrupted' in calls[0])
+
+
def test_actual_expiry(self):
# Regression test for #19200
old_session_key = None
@@ -411,12 +423,12 @@ def test_invalid_key_backslash(self):
# This is tested directly on _key_to_file, as load() will swallow
# a SuspiciousOperation in the same way as an IOError - by creating
# a new session, making it unclear whether the slashes were detected.
- self.assertRaises(SuspiciousOperation,
+ self.assertRaises(InvalidSessionKey,
self.backend()._key_to_file, "a\\b\\c")
def test_invalid_key_forwardslash(self):
# Ensure we don't allow directory-traversal
- self.assertRaises(SuspiciousOperation,
+ self.assertRaises(InvalidSessionKey,
self.backend()._key_to_file, "a/b/c")
@override_settings(SESSION_ENGINE="django.contrib.sessions.backends.file")
@@ -1,6 +1,7 @@
"""
Global Django exception and warning classes.
"""
+import logging
from functools import reduce
@@ -9,37 +10,56 @@ class DjangoRuntimeWarning(RuntimeWarning):
class ObjectDoesNotExist(Exception):
- "The requested object does not exist"
+ """The requested object does not exist"""
silent_variable_failure = True
class MultipleObjectsReturned(Exception):
- "The query returned multiple objects when only one was expected."
+ """The query returned multiple objects when only one was expected."""
pass
class SuspiciousOperation(Exception):
- "The user did something suspicious"
+ """The user did something suspicious"""
+
+
+class SuspiciousMultipartForm(SuspiciousOperation):
+ """Suspect MIME request in multipart form data"""
+ pass
+
+
+class SuspiciousFileOperation(SuspiciousOperation):
+ """A Suspicious filesystem operation was attempted"""
+ pass
+
+
+class DisallowedHost(SuspiciousOperation):
+ """HTTP_HOST header contains invalid value"""
+ pass
+
+
+class DisallowedRedirect(SuspiciousOperation):
+ """Redirect to scheme not in allowed list"""
pass
class PermissionDenied(Exception):
- "The user did not have permission to do that"
+ """The user did not have permission to do that"""
pass
class ViewDoesNotExist(Exception):
- "The requested view does not exist"
+ """The requested view does not exist"""
pass
class MiddlewareNotUsed(Exception):
- "This middleware is not used in this server configuration"
+ """This middleware is not used in this server configuration"""
pass
class ImproperlyConfigured(Exception):
- "Django is somehow improperly configured"
+ """Django is somehow improperly configured"""
pass
Oops, something went wrong.

0 comments on commit d228c11

Please sign in to comment.