Sensitive variables decorator does not hide sensitive variables

[Vlastimil Zíma]

When an exception is raised and nicely formatted email is sent, sensitive variables are not hidden. They are hidden in the last frame, but they can be clearly seen in the arguments of the sensitive_variables_wrapper itself.

Code:

@method_decorator(sensitive_variables('token'))
def verify_totp(self, token):
    raise ValueError

Traceback in HTML email:

/home/vlastimil/git/django/django/views/decorators/debug.py in sensitive_variables_wrapper

34. return func(*args, **kwargs)

Local Vars
Variable 	Value
sensitive_variables_wrapper 	<function bound_func at 0xb4434fc>
variables 	('token',)
args 	(45485464,)
func 	<function bound_func at 0xbde0d14>
kwargs 	{}

/home/vlastimil/git/django/django/utils/decorators.py in bound_func

21. return func(self, *args2, **kwargs2)

Local Vars
Variable 	Value
args2 	(45485464,)
func 	<function verify_totp at 0xadc3ed4>
self 	<OTP: OTP object>
kwargs2 	{}

/home/vlastimil/git/ginger/mojeid/mojeid/models/otp.py in verify_totp

81. raise ValueError

Local Vars
Variable 	Value
token 	u'********************'

self 	<OTP: OTP object>

It can be easily seen the value 45485464 in the local vars of the sensitive_variables_wrapper. I could not determine any simple solution to avoid this problem.

Note: I was not able to make django 1.5 work yet, but checking the changes in relevant files I did not found any change that might solve this bug.

Note2: Variables are also shown in the method_decorator arguments which is another story, which can not be fixed until this one is resolved.

[Julien Phalip]

Thanks for the report. I'm curious, what did you do to have the Local Vars displayed in the email in the first place? I've quickly checked and by default the email seems to only contain the traceback, without the Local Vars.

If you could also provide an actual test case (see examples in regressiontests.views.tests.debug.ExceptionReportTestMixin), that would be really helpful. Thanks!

[Karen Tracey]

To get local vars in the error email configure the admin email handler in logging config to send html email: ​https://docs.djangoproject.com/en/1.4/topics/logging/#django.utils.log.AdminEmailHandler

[Julien Phalip]

Thank you Karen. I can reproduce the problem. This is a security-related issue and a bug in a feature shipped in 1.4, so I'm marking as release blocker.

[Julien Phalip]

So, the core issue here is that in this case the sensitive variables are in fact function arguments. And those arguments leak through the stack of decorators (i.e. in this case sensitive_variables and method_decorator).

Now, I don't see a simple way for the sensitive_variables to anticipate which of these arguments may or may not be sensitive in the frames below, unless perhaps via using some black magic with the inspect module. So for the sake of simplicity, in the patch above I've opted for systematically obfuscating the function arguments within the sensitive_variables decorator's frame.

As for the variables leaking through the stacks above the sensitive_variables decorator's frames (i.e. in this case method_decorator), I'm not sure if there is anything we can do about. Perhaps this should be noted as a limitation in the documentation.

Any feedback would be welcome. Thanks!

[Vlastimil Zíma]

I checked original ticket #14614, but I still do not quite get why sensitive decorators wrap the function into a wrapper with sensitive attribute and not just add the sensitive attribute directly to the function.

[Julien Phalip]

Replying to vzima:

I checked original ticket #14614, but I still do not quite get why sensitive decorators wrap the function into a wrapper with sensitive attribute and not just add the sensitive attribute directly to the function.

The main idea is to not clutter the wrapped function's attribute dictionary and to avoid any potential conflicts. Also, adding an attribute to the wrapped function wouldn't necessarily help at all, especially in the case where we have multiple decorators chained together and @sensitive_variables is at the top of the chain.

[Vlastimil Zíma]

Replying to julien:

The main idea is to not clutter the wrapped function's attribute dictionary and to avoid any potential conflicts.

Indeed, there could be such conflict. On the other side, it is still possible although it is much less probable.

Also, adding an attribute to the wrapped function wouldn't necessarily help at all, especially in the case where we have multiple decorators chained together and @sensitive_variables is at the top of the chain.

This does not work anyway, see Note2 in the description. It is necessary to either decorate every decorator with @sensitive_variables (IMHO not dry) or solve it another way (any ideas?). Should we solve this in this ticket or should I make another one for this issue?

[Aymeric Augustin]

I've converted the latest patch into a pull request to make reviews easier: ​https://github.com/django/django/pull/613

[Florian Apolloner]

I just reviewed the code and it looks good to me.

[Julien Phalip <jphalip@…>]

In 9180146d21cf2a31eec994b4adc0e50c7120f17f:

Fixed #19453 -- Ensured that the decorated function's arguments are obfuscated in the @sensitive_variables decorator's frame, in case the variables associated with those arguments were meant to be obfuscated from the decorated function's frame.
Thanks to vzima for the report.

[Julien Phalip]

Replying to vzima:

Replying to julien:

Also, adding an attribute to the wrapped function wouldn't necessarily help at all, especially in the case where we have multiple decorators chained together and @sensitive_variables is at the top of the chain.

This does not work anyway, see Note2 in the description. It is necessary to either decorate every decorator with @sensitive_variables (IMHO not dry) or solve it another way (any ideas?). Should we solve this in this ticket or should I make another one for this issue?

In your example, sensible_variables is not at the top of the chain. method_decorator is. I've added a note about this in the documentation.

In the specific case of method_decorator, you shouldn't need to use it as sensible_variables should already work well with methods. See #18379.

If you find any genuine bug or can suggest a better approach, please feel free to open a separate ticket. Thanks!

[Julien Phalip <jphalip@…>]

In dfd8623de4e225e33c334086ff4e2ccdfb07247f:

[1.5.x] Fixed #19453 -- Ensured that the decorated function's arguments are obfuscated in the @sensitive_variables decorator's frame, in case the variables associated with those arguments were meant to be obfuscated from the decorated function's frame.
Thanks to vzima for the report.
Backport of 9180146d21cf2a31eec