Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#19453 closed Bug (fixed)

Sensitive variables decorator does not hide sensitive variables

Reported by: Vlastimil Zíma Owned by: Julien Phalip
Component: Core (Other) Version: 1.4
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

19453-sensitive-variables.diff (11.6 KB ) - added by Julien Phalip 12 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 by Julien Phalip, 12 years ago

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!

comment:2 by Karen Tracey, 12 years ago

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

comment:3 by Julien Phalip, 12 years ago

Owner: changed from nobody to Julien Phalip
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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.

by Julien Phalip, 12 years ago

comment:4 by Julien Phalip, 12 years ago

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!

comment:5 by Vlastimil Zíma, 12 years ago

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.

in reply to:  5 ; comment:6 by Julien Phalip, 12 years ago

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.

in reply to:  6 ; comment:7 by Vlastimil Zíma, 12 years ago

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?

comment:8 by Aymeric Augustin, 12 years ago

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

comment:9 by Florian Apolloner, 12 years ago

Triage Stage: AcceptedReady for checkin

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

comment:10 by Julien Phalip <jphalip@…>, 12 years ago

Resolution: fixed
Status: newclosed

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.

in reply to:  7 comment:11 by Julien Phalip, 12 years ago

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!

comment:12 by Julien Phalip <jphalip@…>, 12 years ago

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

Note: See TracTickets for help on using tickets.
Back to Top