Opened 7 years ago

Last modified 7 weeks ago

#28215 assigned Bug

sensitive_post_parameters/sensitive_variables leaking sensitive values into the http 500 exception email

Reported by: Peter Zsoldos Owned by: Calvin Vu
Component: Error reporting Version: dev
Severity: Normal Keywords:
Cc: Calvin Vu Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description (last modified by Peter Zsoldos)

tl;dr

despite using sensitive_xxx decorator, sensitive data can end up in the 500 error emails Django sends, as these decorators only protect the data inside the very function they are decorated

repro

class ReproTestCase(TransactionTestCase):

    def test_when_login_view_raises_an_exception_password_is_not_in_the_500_email(self):  # noqa: E501
        password = '$0m3 P4$$w0rd'
        exception_email_html_body = self.get_500_email_html_for_login_error(
            username='some_user', password=password
        )
        self.assertNotIn(
                member=password, container=exception_email_html_body)

    def get_500_email_html_for_login_error(self, username, password):
        # patch this methodd so AuthenticationForm.clean is
        # called which has local password variable
        login_view_raising_value_error = patch(
            'django.contrib.auth.forms.authenticate',
            side_effect=ValueError('some error')
        )

        self.goto_login_page()

        with TestClientNotRaisingExceptionButCapturing(self.client) as capture:  # see implementation details in attachment
            with login_view_raising_value_error:
                self.submit_login(username=username, password=password)

        request = capture.get_captured_request()
        exc_type, exc_value, tb = capture.stored_exc_info
        # based on django.utils.log.AdminEmailHandler.emit
        reporter = ExceptionReporter(
            request=request, is_email=True,
            exc_type=exc_type, exc_value=exc_value, tb=tb)
        self.assertTrue(reporter.filter.is_active(request))
        return reporter.get_traceback_html()

Is attached for all current supported Django versions (1.8, 1.10, 1.11), simply unpack and run tox

The test can seem complicated due to the limitations of the test client in testing 500 responses - see #18707

why I think it is an issue

While I'm aware of the [disclaimers in the documentation about filtering sensitive data (https://docs.djangoproject.com/en/1.8/howto/error-reporting/#custom-error-reports), because of the impact of it - even on users who don't explicitly use any of the sensitive_x decorators themselves, I think it is a leak that should be stopped.

  • typical sensitive data is passwords. We have discovered this issue due to a bug in our custom authentication backend. These passwords could also be used beyond just the single Django system - whether because of single sign on solutions like LDAP/active-directory, or simply because users might reuse their passwords across sites
  • exception emails might be sent through third party providers, which may keep track of the sent message body. Internal IT departments might also be considered such 3rd parties too.
  • support people (admins receiving 500 emails) see supposedly private data

potential solution ideas (which might be wrong of course :))

writing a custom exception filter

wrapping sensitive variables into a special object

Instead of just using the sensitive data in reporting, wrap these variables in an object that has 'contains_sensitive_data' attribute, i.e.: if it is stored into another variable, as it is a 'pointer' to the original, it will have that attribute, and thus can be filtered out in the exception report.

This isn't perfect either, e.g.: password = password.strip(), though by overriding a lot of methods or using __getattr__ magic, it could work. Might only be 'reasonable' to do so for request parameters, as there at least we know the limited set of variable types we receive

  @sensitive_request_params
  def view(request):
      ....

  # inside sensitive_request_params
  for sensitive_variable_name in sensitive_variable_names:
      if sensitive_variable_name in request.POST:
         request.POST[sensitive_variable_name] = SensitiveVariable(request.POST[sensitive_variable_name])
      ....

Attachments (1)

django-sensitive-parameter-leaking.tar.gz (2.9 KB ) - added by Peter Zsoldos 7 years ago.
a minimal tox.ini/django project to repro the case

Download all attachments as: .zip

Change History (28)

by Peter Zsoldos, 7 years ago

a minimal tox.ini/django project to repro the case

comment:1 by Peter Zsoldos, 7 years ago

Description: modified (diff)

comment:2 by Peter Zsoldos, 7 years ago

Version: 1.111.8

comment:3 by Tim Graham, 7 years ago

It would be helpful to give a high level overview of the issue in the ticket's summary and description. The current summary is rather general and the description doesn't detail the cause in much detail. Currently, I have to open the sample project, unzip it, and read the code to try to understand the issue.

If I'm reading it correctly, it looks like this particular case could be fixed by marking credentials as a sensitive variable in contrib.auth.authenticate() -- is that correct?

comment:4 by Peter Zsoldos, 7 years ago

Description: modified (diff)

comment:5 by Peter Zsoldos, 7 years ago

Tim,

thanks for the feedback. For the time being, I copypasted the repro unittest's main body into the ticket so it's more accessible.

As for the suggested fix, I think there are two separate issues here

  1. sensitive data across django's own internal code is not marked everywhere as sensitive. This can be fixed manually once and that would be a great improvement.
    • There is one scenario highlighted in this ticket (error during login),
    • Probably all sensitive_post_parameter decorated views need to be reviewed & followed through the code paths to ensure all sensitive variables in all methods are decorated
    • For future code changes, checking for the need to update the sensitive parameters would need to be done too.
    • this only fixes things in Django's own code though, not issues in third party code, though it could be argued that they should write secure code & users of 3rd party code should do due diligence...
  2. however, some generic code might be used from multiple contexts, even from multiple sensitive_post_parameter views - e.g.: MyModel.objects.get. In some contexts, username field might be sensitive (e.g.: login), but in others (e.g.: admin search) it might not. See the below simplified unittest to repro it - it is displayed for the frame django/db/models/query.py in get.
    @sensitive_variables('username')  # to exclude the local var in the stacktrace here
    def test_leaking_data_due_to_exception_in_generic_method(self):

        class TestError(ValueError):
            pass


        @sensitive_post_parameters('username')
        def some_view(request):
            """
            based on docstring from sensitive_post_parameters itself,
            storing it into a local variable.
            But same issue would happen if I the User.objects.get raised
            the User.DoesNotExist error - and how should the generic
            QuerySet.get be annotated with regards to all sensitive parameters?
            """
            uname = request.POST['username']
            User.objects.get(username=request.POST['username'])
            raise TestError('some error')

        username = 'some_username'
        rf = RequestFactory()
        request = rf.post('/submit/', {'username': username})
        try:
            some_view(request)
            raise ValueError('expected to raise an error')
        except (TestError, User.DoesNotExist) as e:
            exc_type, exc_value, tb = sys.exc_info()
            # based on django.utils.log.AdminEmailHandler.emit
            reporter = ExceptionReporter(
                request=request, is_email=True,
                exc_type=exc_type, exc_value=exc_value, tb=tb)
            self.assertTrue(reporter.filter.is_active(request))
            error_mail_html = reporter.get_traceback_html()
            self.assertNotIn(
                    member=username, container=error_mail_html)

Maybe the ticket should be split in two? 'coz doing 1. would already improve the situation quite a bit, but to support 2. might be a bigger effort. But I like the test repro for 2 better than the original report's - not replacing the ticket description with it until I know whether the ticket will be split

comment:6 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted

Feel free to do whatever you think makes sense.

comment:7 by Jacob Walls, 4 years ago

Has patch: set
Owner: set to Hasan Ramezani
Status: newassigned

comment:8 by Carlton Gibson, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by GitHub <noreply@…>, 4 years ago

In 4eb75679:

Refs #28215 -- Marked auth credentials as sensitive variables.

Co-authored-by: Collin Anderson <collin@…>

comment:10 by Carlton Gibson, 4 years ago

Triage Stage: Ready for checkinAccepted

comment:11 by Carlton Gibson, 4 years ago

Has patch: unset

comment:12 by Jacob Walls, 4 years ago

Owner: Hasan Ramezani removed
Status: assignednew

comment:13 by Collin Anderson, 3 years ago

Simple PR for password variables in django/contrib/auth/forms.py: https://github.com/django/django/pull/15482

comment:14 by Carlton Gibson, 3 years ago

Has patch: set
Needs tests: set

comment:15 by Natalia Bidart, 7 months ago

Easy pickings: set
Has patch: unset
Needs tests: unset
Version: 1.8dev

comment:16 by SREEHARI K.V, 7 months ago

Owner: set to SREEHARI K.V
Status: newassigned

comment:17 by Calvin Vu, 2 months ago

Owner: changed from SREEHARI K.V to Calvin Vu

comment:18 by Calvin Vu, 2 months ago

Owner: Calvin Vu removed
Status: assignednew

comment:19 by Calvin Vu, 2 months ago

Owner: set to Calvin Vu
Status: newassigned

comment:20 by Calvin Vu, 8 weeks ago

Has patch: set
Resolution: fixed
Status: assignedclosed

comment:21 by Natalia Bidart, 8 weeks ago

Resolution: fixed
Status: closednew

Hello Calvin, why did you close this ticket? It hasn't been fully resolved yet. If you want, you can deassign yourself, but not close it. Thank you.

comment:22 by Calvin Vu, 8 weeks ago

Hi there, sorry I didn't know what to do after I created my pull request. Next time what should I do?

comment:23 by Calvin Vu, 8 weeks ago

Status: newassigned

comment:24 by Calvin Vu, 8 weeks ago

Cc: Calvin Vu added

comment:25 by Claude Paroz, 7 weeks ago

Calvin, you should only set the "Has patch" flag, as you did, without closing the ticket. Then your patch should be reviewed during the following weeks.

comment:26 by Calvin Vu, 7 weeks ago

Ahh okay that makes sense, thank you

comment:27 by Sarah Boyce, 7 weeks ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top