Code

Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#21098 closed Bug (fixed)

MultiValueDictKeyError leaks sensitive POST data

Reported by: simonpercivall Owned by: timo
Component: Core (Other) Version: master
Severity: Release blocker Keywords:
Cc: jonasborgstrom 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 timo)

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").

Attachments (0)

Change History (11)

comment:1 Changed 10 months ago by timo

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 10 months ago by timo

  • Component changed from Uncategorized to Core (Other)
  • Owner changed from nobody to timo
  • Severity changed from Normal to Release blocker
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug
  • Version changed from 1.5 to master

comment:3 Changed 10 months ago by timo

  • 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 Changed 10 months ago by jonasborgstrom

  • Cc jonasborgstrom added

comment:5 Changed 10 months ago by aaugustin

  • Resolution set to wontfix
  • Status changed from assigned to closed

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

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

  • Resolution wontfix deleted
  • Status changed from closed to new

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

#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 Changed 10 months ago by anonymous

  • 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 Changed 10 months ago by Tim Graham <timograham@…>

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

In 2daada800f8e28cc1ba664b3008efaefab8fb570:

Fixed #21098 -- Applied sensitive_post_parameters to MultiValueDict

Thanks simonpercivall for the report and bmispelon for the review.

comment:11 Changed 10 months ago by Tim Graham <timograham@…>

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

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.