[Django] #21098: MultiValueDictKeyError leaks sensitive POST data
Django
--------------------------------+--------------------
Reporter: simonpercivall | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------
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").
--
Ticket URL: <https://code.djangoproject.com/ticket/21098>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: simonpercivall | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.5
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
> 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").
New description:
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")`.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/21098#comment:1>
Django
Reporter: simonpercivall | Owner: timo
Type: Bug | Status: assigned
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* status: new => assigned
* severity: Normal => Release blocker
* component: Uncategorized => Core (Other)
* version: 1.5 => master
* owner: nobody => timo
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/21098#comment:2>
Django
Reporter: simonpercivall | Owner: timo
Type: Bug | Status: assigned
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/21098#comment:3>
Django
Reporter: simonpercivall | Owner: timo
Type: Bug | Status: assigned
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by jonasborgstrom):
* cc: jonasborgstrom (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21098#comment:4>
Django
Reporter: simonpercivall | Owner: timo
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by aaugustin):
* status: assigned => closed
* resolution: => wontfix
Comment:
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 [https://docs.djangoproject.com/en/dev/howto/error-
reporting/#filtering-sensitive-information 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21098#comment:5>
Django
Reporter: simonpercivall | Owner: timo
Type: Bug | Status: closed
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (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).
--
Ticket URL: <https://code.djangoproject.com/ticket/21098#comment:6>
Django
Reporter: simonpercivall | Owner: timo
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by simonpercivall):
* status: closed => new
* resolution: wontfix =>
Comment:
Yes, correct; this information is included by the AdminEmailHandler, and
by any other loggers, and that is why it's a real issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/21098#comment:7>
Django
Reporter: simonpercivall | Owner: timo
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21098#comment:8>
Django
Reporter: simonpercivall | Owner: timo
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anonymous):
* needs_better_patch: 1 => 0
Comment:
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
[https://github.com/django/django/pull/1624 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/21098#comment:9>
Django
Reporter: simonpercivall | Owner: timo
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"2daada800f8e28cc1ba664b3008efaefab8fb570"]:
{{{
#!CommitTicketReference repository=""
revision="2daada800f8e28cc1ba664b3008efaefab8fb570"
Fixed #21098 -- Applied sensitive_post_parameters to MultiValueDict
Thanks simonpercivall for the report and bmispelon for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21098#comment:10>
Django
Reporter: simonpercivall | Owner: timo
Type: Bug | Status: closed
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"778d4da9cc249839b059e45d90e3a947bb76dd6d"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21098#comment:11>