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
Is attached for all current supported Django versions (1.8, 1.10, 1.11),
simply unpack and run `tox`
(https://gist.github.com/zsoldosp/1cc4c922e8aa925439516b0c127e70d6 - this
is the first time I'm trying to attach files, thus including a link to a
gist as a backup if that doesn't work)
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
* simply don't report any variables once encountered -
https://gist.github.com/zsoldosp/5710abaa9dedc03417d60bcc714c95d4
* keep track of protected variable names and replace those in frames
further down the stack (i.e.: if parameter 'password', is sensitive,
cleanse variables names 'password' too in all methods)
=== 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
{{{#!python
@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])
....
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28215>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* Attachment "django-sensitive-parameter-leaking.tar.gz" added.
a minimal tox.ini/django project to repro the case
Old description:
New description:
== 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
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:1>
* version: 1.11 => 1.8
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:2>
Comment (by Tim Graham):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:3>
Old description:
> == 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
>
> Is attached for all current supported Django versions (1.8, 1.10, 1.11),
> simply unpack and run `tox`
>
New description:
== 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
{{{#!python
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:4>
Comment (by Peter Zsoldos):
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...
1. 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`.
{{{#!python
@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
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:5>
* stage: Unreviewed => Accepted
Comment:
Feel free to do whatever you think makes sense.
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:6>
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:8>
Comment (by GitHub <noreply@…>):
In [changeset:"4eb756793b64cf153be4fbe0411da6e3e4f1279d" 4eb75679]:
{{{
#!CommitTicketReference repository=""
revision="4eb756793b64cf153be4fbe0411da6e3e4f1279d"
Refs #28215 -- Marked auth credentials as sensitive variables.
Co-authored-by: Collin Anderson <col...@onetencommunications.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:9>
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:10>
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:11>
* owner: Hasan Ramezani => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:12>
Comment (by Collin Anderson):
Simple PR for `password` variables in `django/contrib/auth/forms.py`:
https://github.com/django/django/pull/15482
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:13>
* has_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/28215#comment:14>