Merge lp:~canonical-isd-hackers/canonical-identity-provider/bug_553932_oops_password into lp:canonical-identity-provider/release

[Danny Tamez]

Made changes so that sensitive data (defined as any setting with PASSWORD, SECRET and now PRIVATE or any request, post var with password, secret, private in it is kept out of the oops logs)

[Ricardo Kirkner (ricardokirkner)]

Instead of duplicating django's classes and methods for this, we could just monkey-patch it. Also, we are not sanitizing local variables in frames, and sometimes sensitive data is stored locally.

What should have to be monkey-patched:

. HIDDEN_SETTINGS
. ExceptionReporter.get_traceback_frames

That way we still use django's in-place error reporting mechanism, but just extending it to be stricter.

review: Needs Fixing

[Ricardo Kirkner (ricardokirkner)]

All changes discussed and agreed. Thanks.

review: Approve

=== modified file 'identityprovider/middleware/exception.py'
--- identityprovider/middleware/exception.py	2010-04-21 15:29:24 +0000
+++ identityprovider/middleware/exception.py	2010-05-10 15:28:26 +0000
@@ -1,12 +1,18 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import logging
+import re
 import sys
 import traceback
-import logging
+
+from django.conf import settings
+from django.db import connection
+from django.views import debug
 from django.views.debug import ExceptionReporter
-from django.db import connection
-from django.conf import settings
+
+debug.HIDDEN_SETTINGS = re.compile(
+    'SECRET|PASSWORD|PROFANITIES_LIST|PRIVATE|secret|password|private')
 
 
 class ConsoleExceptionMiddleware(object):
@@ -15,15 +21,90 @@
 
 
 class LogExceptionMiddleware(object):
+
+    def get_reporter(self, request, *exc_info):
+        safe = self.sanitize_request(request)
+        reporter = ExceptionReporter(safe, *exc_info)
+        reporter.get_traceback_frames = self.get_traceback_frames
+        self._get_lines_from_file = reporter._get_lines_from_file
+        return reporter
+
     def process_exception(self, request, exception):
         """Log the traceback, so that it can be kept in the oops file"""
+        reporter = self.get_reporter(request, *sys.exc_info())
         template_debug = settings.TEMPLATE_DEBUG
         settings.TEMPLATE_DEBUG = True
         try:
-            reporter = ExceptionReporter(request, *sys.exc_info())
             logging.warn(reporter.get_traceback_html())
             logging.warn(traceback.format_exc())
             for query in connection.queries:
                 logging.warn("time: %(time)s sql: %(sql)s" % query)
         finally:
             settings.TEMPLATE_DEBUG = template_debug
+
+    def sanitize_request(self, request):
+        """Remove sensitive from the request before it is displayed"""
+        dup_post = request.POST.copy()
+        for key in request.POST.iterkeys():
+            if debug.HIDDEN_SETTINGS.search(key):
+                dup_post[key] = '********'
+        request.POST = dup_post
+        dup_get = request.GET.copy()
+        for key in request.GET.iterkeys():
+            if debug.HIDDEN_SETTINGS.search(key):
+                dup_get[key] = '********'
+        request.GET = dup_get
+        return request
+
+    def sanitize_vars(self, items):
+        sanitized = []
+        for item in items:
+            if debug.HIDDEN_SETTINGS.search(item[0]):
+                sanitized.append((item[0], '********'))
+            else:
+                sanitized.append(item)
+        return sanitized
+
+    # This function was taken from the django project.
+    # Please see the license file in the third-party/django directory.
+    def get_traceback_frames(self):
+        frames = []
+        tb = sys.exc_info()[2]
+        while tb is not None:
+            # support for __traceback_hide__ which is used by a few libraries
+            # to hide internal frames.
+            if tb.tb_frame.f_locals.get('__traceback_hide__'):
+                tb = tb.tb_next
+                continue
+            filename = tb.tb_frame.f_code.co_filename
+            function = tb.tb_frame.f_code.co_name
+            lineno = tb.tb_lineno - 1
+            loader = tb.tb_frame.f_globals.get('__loader__')
+            module_name = tb.tb_frame.f_globals.get('__name__')
+            pre_context_lineno, pre_context, context_line, post_context = \
+                    self._get_lines_from_file(filename, lineno, 7, loader,
+                                              module_name)
+            if pre_context_lineno is not None:
+                frames.append({
+                    'tb': tb,
+                    'filename': filename,
+                    'function': function,
+                    'lineno': lineno + 1,
+                    'vars': self.sanitize_vars(tb.tb_frame.f_locals.items()),
+                    'id': id(tb),
+                    'pre_context': pre_context,
+                    'context_line': context_line,
+                    'post_context': post_context,
+                    'pre_context_lineno': pre_context_lineno + 1,
+                })
+            tb = tb.tb_next
+
+        if not frames:
+            frames = [{
+                'filename': '<unknown>',
+                'function': '?',
+                'lineno': '?',
+                'context_line': '???',
+            }]
+
+        return frames

=== modified file 'identityprovider/tests/test_middleware.py'
--- identityprovider/tests/test_middleware.py	2010-04-21 15:29:24 +0000
+++ identityprovider/tests/test_middleware.py	2010-05-10 15:28:26 +0000
@@ -1,13 +1,17 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import logging
+import sys
+
+from django.conf import settings
 from django.contrib.auth.models import User, AnonymousUser
-
-from identityprovider.tests.utils import BasicAccountTestCase, MockRequest
+from identityprovider.tests.utils import (BasicAccountTestCase, MockRequest,
+                                          TestCase)
 from identityprovider.models import EmailAddress, Account
+from identityprovider.middleware.exception import LogExceptionMiddleware
 from identityprovider.middleware.useraccount import (
-    UserAccountConversionMiddleware
-)
+    UserAccountConversionMiddleware)
 
 
 class UserAccountConversionMiddlewareTestCase(BasicAccountTestCase):
@@ -51,3 +55,84 @@
         self.middleware.process_request(request)
 
         self.assertTrue(isinstance(request.user, AnonymousUser))
+
+
+class ExceptionMiddlewareTestCase(TestCase):
+
+    POST_PASSWORD = 'P1!2@3#'
+    POST_SECRET = 'P2@3#4$'
+    POST_PRIVATE = 'P3#4$5%'
+    GET_PASSWORD = 'G1!2@3#'
+    GET_SECRET = 'G2@3#4$'
+    GET_PRIVATE = 'G3#4$5%'
+    POST_PUBLIC = 'Canonical Rocks!'
+    GET_PUBLIC = 'Canonical Rocks!'
+    asterisks = '********'
+    PRIVATE_SETTING = '7&8*9('
+    PUBLIC_SETTING = 'Something public'
+    FRAME_VAR = '*8*8*8*8'
+
+    LOG_FILENAME = '/tmp/logging_example.out'
+    logging.basicConfig(filename=LOG_FILENAME, level=logging.DEBUG)
+
+    logging.debug('This message should go to the log file')
+
+    def setUp(self):
+        request = MockRequest('/login')
+        request.POST = {}
+        request.GET = {}
+        request.COOKIES = {}
+        request.META = {}
+        request.POST['post_password'] = self.POST_PASSWORD
+        request.POST['post_secret'] = self.POST_SECRET
+        request.POST['post_private'] = self.POST_PRIVATE
+        request.POST['post_public'] = self.POST_PUBLIC
+        request.GET['get_password'] = self.GET_PASSWORD
+        request.GET['get_secret'] = self.GET_SECRET
+        request.GET['get_private'] = self.GET_PRIVATE
+        request.GET['get_public'] = self.GET_PUBLIC
+        self.request = request
+        self.middleware = LogExceptionMiddleware()
+        settings.PRIVATE_SETTING = self.PRIVATE_SETTING
+        settings.PUBLIC_SETTING = self.PUBLIC_SETTING
+
+    def test_settings_sanitization(self):
+        try:
+            x = 2 / 0
+        except:
+            reporter = self.middleware.get_reporter(self.request,
+                                                    *sys.exc_info())
+            html = reporter.get_traceback_html()
+        self.assertFalse(self.PRIVATE_SETTING in html)
+        self.assertTrue(self.PUBLIC_SETTING in html)
+
+    def test_frames_sanitization(self):
+        try:
+            FRAME_PASSWORD = self.FRAME_VAR
+            public_variable = 2 / 0
+        except:
+            reporter = self.middleware.get_reporter(self.request,
+                                                    *sys.exc_info())
+            html = reporter.get_traceback_html()
+        self.assertFalse(self.FRAME_VAR in html)
+        self.assertTrue('public_variable' in html)
+
+    def test_request_sanitization(self):
+        req = self.middleware.sanitize_request(self.request)
+        #  Make sure sensitive data is no longer in the request
+        self.assertFalse(self.POST_PASSWORD in req.POST.values())
+        self.assertFalse(self.POST_SECRET in req.POST.values())
+        self.assertFalse(self.POST_PRIVATE in req.POST.values())
+        self.assertFalse(self.GET_PASSWORD in req.GET.values())
+        self.assertFalse(self.GET_SECRET in req.GET.values())
+        self.assertFalse(self.GET_PRIVATE in req.GET.values())
+        #  Make sure sensitive data is replaced by asterisks
+        self.assertEquals(req.POST['post_password'], self.asterisks)
+        self.assertEquals(req.POST['post_secret'], self.asterisks)
+        self.assertEquals(req.POST['post_private'], self.asterisks)
+        self.assertEquals(req.GET['get_password'], self.asterisks)
+        self.assertEquals(req.GET['get_secret'], self.asterisks)
+        self.assertEquals(req.GET['get_private'], self.asterisks)
+        #  Make sure public data is unaffected
+        self.assertEquals(req.POST['post_public'], self.POST_PUBLIC)
+        self.assertEquals(req.GET['get_public'], self.GET_PUBLIC)