Opened 14 months ago

Closed 7 weeks ago

#22990 closed Cleanup/optimization (fixed)

Sensitive POST data leaks from complex variables

Reported by: vzima Owned by: vzima
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Using @sensitive_post_parameters() decorator does not help much, because POST data are leaked through variables which wraps the request, such as dictionaries and tuples. Since there are a lot of wrappers and decorators this is fairly common problem.

I have added temporarily a ValueError into UserAdmin.add_view and checked the result. The password is leaked on several occasions. Look for 'secret' in the attached email.

Attachments (1)

sensitive-leaked.eml (207.4 KB) - added by vzima 14 months ago.
Error email with sensitive data

Download all attachments as: .zip

Change History (14)

Changed 14 months ago by vzima

Error email with sensitive data

comment:1 Changed 14 months ago by vzima

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

This is most likely a generic version of #21098.

comment:2 Changed 14 months ago by timo

After working on #21098, I think investing more time in trying to make @sensitive_post_parameters() more secure is a losing battle. We should at least document that it's not foolproof, as well as the inherent insecurity in error reporting by email.

comment:3 Changed 14 months ago by vzima

Wouldn't be worth it to handle sensitive post parameters in build_request_repr https://github.com/django/django/blob/master/django/http/request.py#L448.

comment:4 Changed 14 months ago by timo

Perhaps; I'm not sure much complexity it would add. Are you interested in writing a patch?

comment:5 Changed 14 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/optimization

comment:6 Changed 14 months ago by vzima

  • Owner changed from nobody to vzima
  • Status changed from new to assigned

I'll give it a try.

comment:7 Changed 12 months ago by vzima

  • Has patch set

I finally created a patch which solves this issue, pull request is at github: https://github.com/django/django/pull/3145

I've found a serious flaw in the tests of the technical 500 error. The tests directly called the view which rendered technical 500 page itself. This greatly cut down the traceback to single frame, thus hiding potential leakage occuring on higher levels in production environments. So, I've refactored the tests to use test client and common handling of unhandled exception.

Further issues:

  • Some important data may be hidden in DEBUG mode. I'll investigate it further after this ticket is completed.
  • Because of the complexity of the problem, data can't be marked as sensitive only for one channel (e.g. HTML error mails) and affected values should be generally hidden in all cases. IMHO EXCEPTION_REPORTER_FILTER should be replaced with SANITIZER to remove any confusion which may occur from seeing exception reporter filter in request POST representation.
  • Test client may have an attribute to disable reraising the exception from the view.

comment:8 Changed 8 months ago by berkerpeksag

  • Patch needs improvement set

comment:9 Changed 6 months ago by auvipy

  • Owner changed from vzima to auvipy

comment:10 Changed 2 months ago by vzima

  • Owner changed from auvipy to vzima

Finally I had a bit of time to update the patch. It is in new PR https://github.com/django/django/pull/4947.

comment:11 Changed 2 months ago by vzima

  • Patch needs improvement unset

comment:12 Changed 2 months ago by timgraham

  • Patch needs improvement set

comment:13 Changed 7 weeks ago by timgraham

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

Fixed by #25099

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