Opened 2 years ago

Closed 2 years ago

Last modified 2 years 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 2 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 2 years 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 2 years 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 2 years 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 2 years ago by julien

comment:4 Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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

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