Re: [Django] #36542: AdminSite views (such as login) leak sensitive POST data

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
--------------------------------+------------------------------------
Reporter: Olivier Dalang | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+------------------------------------
Comment (by Ankan Giri):

Hi! I’m interested in working on this issue.
I have experience working with Django, though I’m new to contributing to
Django itself.
Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data

--------------------------------+--------------------------------------
Reporter: Olivier Dalang | Owner: Ankan Giri
Type: Bug | Status: assigned

Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------+--------------------------------------
Changes (by Ankan Giri):

* owner: nobody => Ankan Giri
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:4>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
--------------------------------+--------------------------------------
Reporter: Olivier Dalang | Owner: Ankan Giri
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------------------------

Comment (by kanin.kearpimy@…):

Hi,

I noticed there is no updating for four months. If no one is working on
this, Can I take one?
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:5>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data

-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy

Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* owner: Ankan Giri => (James) Kanin Kearpimy

--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:6>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by kanin.kearpimy@…):

* has_patch: 0 => 1

Comment:

Hello,

I open patch to this issue https://github.com/django/django/pull/20959.
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:7>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim McCurrach):

I’ve just taken a look at the PR, and I think it’s worth expanding on a
few things in this ticket for the sake of clarity:

- If `DEBUG` is `False`, both sensitive variables and sensitive post
parameters are always cleaned/redacted from debug-reports. This holds,
even when `include_html=True`.
- As far as I can tell, all of the built-in auth views are decorated with
the appropriate `sensitive_post_parameters` decorators.
- The admin login views use the aforementioned built-in views for most of
the heavy lifting, and so are protected to. (This is because even if the
“top-level” view isn’t decorated, `sensitive_post_parameters` gets set on
the request further down - which is all that matters).
 - Most of the
associated authentication functions are decorated with the appropriate
sensitive_variables decorators.

The above is to say, that all-in-all, even with `include_html=True`
everything is actually already pretty well locked down. The reason the
password appears in the screenshot above is that some of the functions
deeper down the stack (at the auth-backend / model level) aren’t decorated
with the `sensitive_variables` decorator.

I wonder if the docs are potentially misleading on this. They say:
>The include_html argument of AdminEmailHandler is used to control whether
the traceback email includes an HTML attachment containing the full
content of the debug web page that would have been produced if DEBUG were
True.

This is maybe a little bit ambiguous. My reading of the above would be
that sensitive variables would remain unfiltered - but that isn't true.


----

Regarding the original suggestion:

> why don't we just apply the same filter used for settings
`(API|AUTH|TOKEN|KEY|SECRET|PASS|SIGNATURE|HTTP_COOKIE)` to POST
parameters as well as variables in the full trace ?

This won’t actually help the POST parameters since they are already
filtered when `DEBUG` is `False`. It would cover a few more of the
sensitive variables, but I think we could (maybe should) patch up the last
few remaining functions in the auth app, that have any local variables
that are password-y in them with the `sensitive_variables` decorator.
Doing a quick search there don’t seem to be too many. That way, we still
have all the local variable data for debugging.
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:8>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim McCurrach):

The other thing that might help is to provide clearer guidance about what
makes `include_html` dangerous. Taking a quick look at
`techincal_500.html` and `techincal_500.txt` - the only difference that
jumps out at me is that the former includes local variables. The docs
mention this:

> The reporter_class argument of AdminEmailHandler allows providing an
django.views.debug.ExceptionReporter subclass to customize the traceback
text sent in the email body.

We could also provide a simple example of how that is done if users wanted
to have a more nuclear/blanket approach to filtering local variables.
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:9>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

> The reason the password appears in the screenshot above is that some of
the functions deeper down the stack (at the auth-backend / model level)
aren’t decorated with the sensitive_variables decorator.

Noting we have an accepted ticket for that in #35930.
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:10>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by kanin.kearpimy@…):

Hi,

Let me sum it up and please correct me if something doesn't make sense.
So, now the problem is we doesn't have `sensitive_post_parameters` and
`sensitive_variables` to cover sensitive function parameters, in this
case `password`-related for POST request. That's why in `DEBUG=True`,
there is `password`-related field appear?

Regarding below

why don't we just apply the same filter used for settings
(API|AUTH|TOKEN|KEY|SECRET|PASS|SIGNATURE|HTTP_COOKIE) to POST parameters
as well as variables in the full trace ?

it may doesn't help to broadly filter `password` in POST, because deeper
down the stack still isn't covered by `sensitive_post_parameters` OR
`sensitive_variables` as Tim mentioned here

This won’t actually help the POST parameters since they are already

filtered when DEBUG is False. It would cover a few more of the sensitive

variables, but I think we could (maybe should) patch up the last few
remaining functions in the auth app, that have any local variables that
are password-y

As Jacob shared there was a PR did such similar thing before in #35930
Noting we have an accepted ticket for that in #35930 with respect to
the methods for initiating a database connection.

So, it's good idea to imitate the #35930 approach and cover up all auth
app function with local password-y varialbes?
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:11>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Timothy Schilling):

@Tim, it looks like the documentation you're looking for exists at
https://docs.djangoproject.com/en/6.0/topics/logging/#adminemailhandler

> The built-in AdminEmailHandler deserves a mention in the context of
security. If its include_html option is enabled, the email message it
sends will contain a full traceback, with names and values of local
variables at each level of the stack, plus the values of your Django
settings (in other words, the same level of detail that is exposed in a
web page when DEBUG is True).

@kanin, I agree with your conclusion of "So, it's good idea to imitate the

#35930 approach and cover up all auth app function with local password-y

variables?". This seems like the better approach since it highlights on
the view which parameters need to be sanitized because the decorator is
physically close to it.
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:12>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim McCurrach):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:13>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim McCurrach):

Replying to [comment:12 Tim Schilling]:

> @Tim, it looks like the documentation you're looking for exists at
https://docs.djangoproject.com/en/6.0/topics/logging/#adminemailhandler

Nice, that's exactly what I was suggesting. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:14>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by kanin.kearpimy@…):

Hi everyone,

I found interesting bug for `reset password`. Well, this card relates to
`login` mainly but reporter left comments of `reset password` scenario as
well. So, I have a look at it.

Please see diagram below for clarification.

[[Image(https://i.postimg.cc/ZRvWWjfW/django-ticket-36542.jpg)]]

For `login` scenario, it's simpler, because when user hit login, it would
invoke `authenticate` function to auth such user a credential. If database
throw error, we can use `sensitive_variables("password")` to hide
`password-y` local variables in relating methods which I did in current
[PR](https://github.com/django/django/pull/20959).

However, `reset password` scenario is another story. because only
**authenticating user** is able to **reset** their password, system
attempts to get user information via `get_user`. If database throw error
there, it will directly go to `debug` page, no going to authenticate or
any functions holding `password-y` local variables. So,
`sensitive_variables("password")` doesn't work out this scenario (I tried
added once, it doesn't work).

Technically, we can hide it in `debug.py` or need time to investigate
more. But I'm not sure it will be too complicated for this ticket (or we
can open it as another issue)?

Please suggest
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:15>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

> So, sensitive_variables("password") doesn't work out this scenario (I
tried added once, it doesn't work).

Can you add the stacktrace? I'm wondering if the failure you are
simulating is causing errors inside `SessionMiddleware`, which would point
at a larger design problem where `sensitive_post_parameters` is useless if
a failure happens in middleware and we never reach the view. In which
case, definitely out of scope for your issue here.
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:16>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by kanin.kearpimy@…):

Replying to [comment:16 Jacob Walls]:

> > So, sensitive_variables("password") doesn't work out this scenario (I
tried added once, it doesn't work).
>
> Can you add the stacktrace? I'm wondering if the failure you are
simulating is causing errors inside `SessionMiddleware`, which would point
at a larger design problem where `sensitive_post_parameters` is useless if
a failure happens in middleware and we never reach the view. In which
case, definitely out of scope for your issue here.

I found it. Well, it depends on exception level in stack trace. For normal
situation, `change_password` method raises exception, we can guard it
there like [https://github.com/django/django/pull/20959/changes#diff-
0b9b76020bca57d146eddea5c47e1e6a99744ce287a365e18a0d7685dd268f18R352-R354].

However, some exception can raise at `get_user` middleware as well. For
example, while user is logging in and Django can't access database / table
of such database.

Now, My PR only guard for `views` cases like Admin login view /
change_password view. Should be enough for this ticket? Otherwise, I can
also investigate more into middleware.
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:17>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

Right, I think the issue with middleware deserves its own triage/design
decision, since there's no way to use `sensitive_post_parameters` for it.
So your proposal seems appropriate for now.
--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:18>

[Django]

#36542: AdminSite views (such as login) leak sensitive POST data
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (James)
| Kanin Kearpimy
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36542#comment:19>