[Django] #35730: Enhance password reset security by signing 'uid' parameter instead of base64-encoding to prevent possible user count leakage

[Django]

#35730: Enhance password reset security by signing 'uid' parameter instead of
base64-encoding to prevent possible user count leakage
----------------------+------------------------------------------------
Reporter: Remy | Type: Cleanup/optimization
Status: new | Component: contrib.auth
Version: 5.1 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------+------------------------------------------------
When using Django’s default view for requesting a password reset,
`PasswordResetView`, the `PasswordResetForm`’s save() method sends an
email containing a `uid` parameter generated using
`urlsafe_base64_encode(force_bytes(user.pk))`.

This results in the user’s email inbox containing a password reset link
that indirectly reveals the user’s primary key (`user.pk`), which exposes
information about how many users exist on any Django site that uses this
default view.

Surely, organizations that design their entities with non-enumerable
public identifiers (such as by using a `UUIDField` for the primary key)
would not be affected by this, however as the issue is also addressed by
other means, such as a secondary public identifier, or simply a careful
app design, I would still think that many Django site owners who prefer to
keep this information private are likely unaware that it’s being exposed
through this native mechanism.

To prevent the leakage of the `user.pk` value by default, I replaced the
base64 encoding with the signing of the `user.pk` value.
--
Ticket URL: <https://code.djangoproject.com/ticket/35730>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35730: Enhance password reset security by signing 'uid' parameter instead of
base64-encoding to prevent possible user count leakage

-------------------------------------+-------------------------------------
Reporter: Remy | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: contrib.auth | Version: 5.1
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Description changed by Remy:

Old description:

> When using Django’s default view for requesting a password reset,
> `PasswordResetView`, the `PasswordResetForm`’s save() method sends an
> email containing a `uid` parameter generated using
> `urlsafe_base64_encode(force_bytes(user.pk))`.
>
> This results in the user’s email inbox containing a password reset link
> that indirectly reveals the user’s primary key (`user.pk`), which exposes
> information about how many users exist on any Django site that uses this
> default view.
>
> Surely, organizations that design their entities with non-enumerable
> public identifiers (such as by using a `UUIDField` for the primary key)
> would not be affected by this, however as the issue is also addressed by
> other means, such as a secondary public identifier, or simply a careful
> app design, I would still think that many Django site owners who prefer
> to keep this information private are likely unaware that it’s being
> exposed through this native mechanism.
>
> To prevent the leakage of the `user.pk` value by default, I replaced the
> base64 encoding with the signing of the `user.pk` value.

New description:

When using Django’s default view for requesting a password reset,
`PasswordResetView`, the `PasswordResetForm`’s save() method sends an
email containing a `uid` parameter generated using
`urlsafe_base64_encode(force_bytes(user.pk))`.

This results in the user’s email inbox containing a password reset link
that indirectly reveals the user’s primary key (`user.pk`), which exposes
information about how many users exist on any Django site that uses this
default view.

Surely, organizations that design their entities with non-enumerable
public identifiers (such as by using a `UUIDField` for the primary key)
would not be affected by this, however as the issue is also addressed by
other means, such as a secondary public identifier, or simply a careful
app design, I would still think that many Django site owners who prefer to
keep this information private are likely unaware that it’s being exposed
through this native mechanism.

To prevent the leakage of the `user.pk` value by default, I replaced the

base64 encoding with the signing of the `user.pk` value (PR
https://github.com/django/django/pull/18539).

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

[Django]

#35730: Enhance password reset security by signing 'uid' parameter instead of
base64-encoding to prevent possible user count leakage

--------------------------------------+------------------------------------
Reporter: Remy | Owner: (none)
Type: Cleanup/optimization | Status: new

Component: contrib.auth | Version: 5.1
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 Simon Charette):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:2>

[Django]

#35730: Enhance password reset security by signing 'uid' parameter instead of
base64-encoding to prevent possible user count leakage
--------------------------------------+------------------------------------

Reporter: Remy | Owner: Remy
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: dev

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

* owner: (none) => Remy
* status: new => assigned
* version: 5.1 => dev

--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:3>

[Django]

#35730: Enhance password reset security by signing 'uid' parameter instead of
base64-encoding to prevent possible user count leakage
--------------------------------------+------------------------------------
Reporter: Remy | Owner: Remy
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by Natalia Bidart):

* needs_better_patch: 0 => 1
* needs_docs: 0 => 1

Comment:

Made an initial review. I think we need to discuss and agree what will
happen with those password reset links that were created using "the old
way" but haven't been used nor expired yet.
--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:4>

[Django]

#35730: Enhance password reset security by signing 'uid' parameter instead of
base64-encoding to prevent possible user count leakage
--------------------------------------+------------------------------------
Reporter: Remy | Owner: Remy
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Comment (by Antoine Humbert):

How does this adress the uid leakage ? Signing is not encrypting. Even if
signed, the uid is still present and clearly visi le in the reset link.
--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:5>

[Django]

#35730: Enhance password reset security by signing 'uid' parameter instead of
base64-encoding to prevent possible user count leakage
--------------------------------------+------------------------------------
Reporter: Remy | Owner: Remy
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Comment (by Remy):

Replying to [comment:5 Antoine Humbert]:
> How does this address the uid leakage ? Signing is not encrypting. Even
if signed, the uid is still present and clearly visible in the reset link.

Indeed you're right, I initially thought signing would be an improvement
over simple base64 encoding, but the signed string still exposes the uid
in the reset link.

What are our options from here?

My guess is that encryption would be the solution for this kind of case.
We could use the cryptography library to encrypt and decrypt the uid, with
the encryption key derived from the `SECRET_KEY`. However, this would add
a new dependency to Django, since cryptography isn’t part of the standard
library.

We could also implement a simple obfuscation method (like an XOR cipher),
which avoids external dependencies but would be less secure.

I am waiting on community guidance and feedback to help move forward with
this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:6>

[Django]

#35730: Enhance password reset security by signing 'uid' parameter instead of
base64-encoding to prevent possible user count leakage
--------------------------------------+------------------------------------
Reporter: Remy | Owner: Remy
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Comment (by Paolo Melchiorre):

Is there no function in the standard libraries that can be used to
increase the security level, without adding external dependencies?

https://docs.python.org/3/library/crypto.html
--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:7>

[Django]

#35730: Enhance password reset security by signing 'uid' parameter instead of
base64-encoding to prevent possible user count leakage
--------------------------------------+------------------------------------
Reporter: Remy | Owner: Remy
Type: Cleanup/optimization | Status: assigned
Component: contrib.auth | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Comment (by Jake Howard):

I believe everything in the standard library focuses on hashing, which is
1-way. Because the view needs to be able to retrieve the user's id from
the URL, it needs to be reversable.

XOR based on a key derived from `SECRET_KEY` might work, and be fairly
simple to implement:
https://en.wikipedia.org/wiki/XOR_cipher#Example_implementation. However,
what is functionally "rolling our own crypto" fills me with dread a
little. XOR is far from perfect: https://stackoverflow.com/a/1135197
(however most are problematic for particularly large inputs, which isn't
relevant here), so whatever is used for the key needs to be suitably
random and not derivable back into the `SECRET_KEY`.
--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:8>