Code

Opened 17 months ago

Closed 16 months ago

Last modified 16 months ago

#19453 closed Bug (fixed)

Sensitive variables decorator does not hide sensitive variables

Reported by: vzima Owned by: julien
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 17 months ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 17 months ago by julien

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 17 months ago by kmtracey

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 Changed 17 months ago by julien

  • Owner changed from nobody to julien
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

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.

Changed 17 months ago by julien

comment:4 Changed 17 months ago by julien

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 follow-up: Changed 16 months ago by 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.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 16 months ago by julien

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.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 16 months ago by vzima

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 Changed 16 months ago by aaugustin

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

comment:9 Changed 16 months ago by apollo13

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:10 Changed 16 months ago by Julien Phalip <jphalip@…>

  • Resolution set to fixed
  • Status changed from new to closed

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.

comment:11 in reply to: ↑ 7 Changed 16 months ago by julien

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 Changed 16 months ago by 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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.