[Django] #35492: Replace call to User.set_password with make_password in authenticate

[Django]

#35492: Replace call to User.set_password with make_password in authenticate
------------------------------------------------+------------------------
Reporter: Natalia Bidart | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.auth | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
In the current implementation of `ModelBackend.authenticate()`,
`set_password()` is invoked on an empty User model to conceal timing
differences between existing and non-existing users, thereby preventing
password timing attacks.
However, relying on `set_password()` in this context may lead to
unintended consequences, given it is a public and overridable method of
the model. The Security Team suggested to directly call `make_password()`
instead to achieve the same desired timing effect.
--
Ticket URL: <https://code.djangoproject.com/ticket/35492>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35492: Replace call to User.set_password with make_password in authenticate

-------------------------------------+-------------------------------------

Reporter: Natalia Bidart | Owner: nobody

Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:

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 אורי):

* cc: אורי (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/35492#comment:1>

[Django]

#35492: Replace call to User.set_password with make_password in authenticate
-------------------------------------+-------------------------------------
Reporter: Natalia Bidart | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Claude Paroz):

I'm still a bit undecided on this. The call in `authenticate` is counting
on the fact that `check_password` (itself calling `set_password`) is using
more or less the same time than `set_password`. If we call `make_password`
instead, wouldn't this open a timing effect attack vector for any project
doing any not-negligible work in a customized `set_password`? Hence I'm
not sure that changing the function to call is not simply moving a
potential problem elsewhere.
--
Ticket URL: <https://code.djangoproject.com/ticket/35492#comment:2>

[Django]

#35492: Replace call to User.set_password with make_password in authenticate
-------------------------------------+-------------------------------------
Reporter: Natalia Bidart | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by אורי):

Replying to [comment:2 Claude Paroz]:

> The call in `authenticate` is counting on the fact that `check_password`

(itself calling `set_password`) ...

I think `check_password` only calls `set_password` after validating the
password, and only if the password is correct (`is_correct` is true), and
also if `must_update` is true, so if the password is not valid and is
validated by `set_password`, it will raise an exception only if the
password is correct, and not always when there is no such user (if the
username doesn't exist in the database).
--
Ticket URL: <https://code.djangoproject.com/ticket/35492#comment:3>

[Django]

#35492: Replace call to User.set_password with make_password in authenticate
-------------------------------------+-------------------------------------
Reporter: Natalia Bidart | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by אורי):

Hi,

We patched Django with such a change on Speedy Net:
https://github.com/speedy-net/speedy-
net/commit/3c98b76ffdadb636664aebacfa126db33b5b78f5

If you want you can apply a similar change to Django itself.

Thanks,
Uri (אורי).
--
Ticket URL: <https://code.djangoproject.com/ticket/35492#comment:4>

[Django]

#35492: Replace call to User.set_password with make_password in authenticate
-------------------------------------+-------------------------------------
Reporter: Natalia Bidart | Owner: nobody

Type: | Status: closed

Cleanup/optimization |
Component: contrib.auth | Version: dev

Severity: Normal | Resolution: wontfix

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 Natalia Bidart):

* resolution: => wontfix
* status: new => closed

Comment:

I did a follow up about this with the members Security Team and there is
an inclination to close this as `wontfix` for the reasons outlined by
Claude above. Doing so now, if there is a desire for a further/broader
discussion, a new topic should be started in the
[https://forum.djangoproject.com/c/internals/5 Forum].
--
Ticket URL: <https://code.djangoproject.com/ticket/35492#comment:5>