Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#21098 closed Bug (fixed)

MultiValueDictKeyError leaks sensitive POST data

Reported by: Simon Percivall Owned by: Tim Graham
Component: Core (Other) Version: dev
Severity: Release blocker Keywords:
Cc: Jonas Borgström Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

Getting an error in MultiValueDict on a POST, such as doing request.POST['foo'], will leak the POST data without any escaping by Django, i.e. the MultiValueDictKeyError contains an unescaped repr of request.POST, no matter if you've added for instance @sensitive_post_parameters("password").

Change History (11)

comment:1 by Tim Graham, 11 years ago

Description: modified (diff)

comment:2 by Tim Graham, 11 years ago

Component: UncategorizedCore (Other)
Owner: changed from nobody to Tim Graham
Severity: NormalRelease blocker
Status: newassigned
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug
Version: 1.5master

comment:3 by Tim Graham, 11 years ago

Has patch: set
Patch needs improvement: set

This seems like a tricky one. I took a stab at some initial ideas, but it's still possible to leak data with this approach.

https://github.com/django/django/pull/1624

comment:4 by Jonas Borgström, 11 years ago

Cc: Jonas Borgström added

comment:5 by Aymeric Augustin, 11 years ago

Resolution: wontfix
Status: assignedclosed

If I understand correctly, this ticket is about Django's debug page, which is only displayed when DEBUG = True.

sensitive_post_parameters is only supported when DEBUG = False.

Relevant excerpt from the error reporting docs:

Django offers a set of function decorators to help you control which information should be filtered out of error reports in a production environment (that is, where DEBUG is set to False)

I'm going to close this ticket for this reason, and for a few others I describe below.


Tim's first attemps shows that it will be very hard to scrub sensitive variables. They may end up anywhere in the stack trace. For instance, if a function contains hash_src = (salt + password).encode('utf-8'), and Django displays the value of local variables in that frame, the value of the password will appear on screen. It seems impossible to defend against all leaking scenarios in the context of a debug page that aims at displaying as much information as possible.

Even if it we had a strong mechanism to mask sensitive variables in the debug page (like looking at their value and replacing it with a placeholder in the generated HTML), I'm not sure it would be a good idea. It's very frustrating when a debugging tool hides the information you need. In a debug page used only in development (when DEBUG = True), it makes sense to priorize debugging capabilities. While it's annoying to read one's password on screen, the cure may be worse than the disease.

Leaking POST data in the debug page isn't much of an issue. The data was entered by a developer working on the project, and it's leaked to the same developer.

sensitive_post_parameters is designed to avoid leaking data in logs or error reports, not in generated HTML, and especially not in the debug page.

comment:6 by anonymous, 11 years ago

I don't believe the concern here is the debug page, repr of the request, including post data, is sent by default email handler (not HTML version).

comment:7 by Simon Percivall, 11 years ago

Resolution: wontfix
Status: closednew

Yes, correct; this information is included by the AdminEmailHandler, and by any other loggers, and that is why it's a real issue.

comment:8 by Tim Graham, 11 years ago

#15625 (removing the dictionary from the MultiValueDictKeyError message) will fix this for plain text emails sent by AdminEmailHandler.

HTML email is another matter because local variables are included which includes the self variable in MultiValueDict.__getitem__ which is the unsanitized POST dict. We may be better off adding some warnings to the documentation than trying to make this protection bulletproof as it seems exceedingly difficult to do so.

comment:9 by anonymous, 11 years ago

Patch needs improvement: unset

After a fix for #15625, sanitizing MultiValueDict is more manageable. I'm not 100% sold it's the right thing to do since you might have other MultiValueDicts that you don't want to treat similarly, but this might be worth that tradeoff. I've updated the original PR.

comment:10 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 2daada800f8e28cc1ba664b3008efaefab8fb570:

Fixed #21098 -- Applied sensitive_post_parameters to MultiValueDict

Thanks simonpercivall for the report and bmispelon for the review.

comment:11 by Tim Graham <timograham@…>, 11 years ago

In 778d4da9cc249839b059e45d90e3a947bb76dd6d:

[1.6.x] Fixed #21098 -- Applied sensitive_post_parameters to MultiValueDict

Thanks simonpercivall for the report and bmispelon for the review.

Backport of 2daada800f from master

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