Opened 2 weeks ago

Last modified 11 days ago

#35730 assigned Cleanup/optimization

Enhance password reset security by encrypting 'uid' parameter instead of base64-encoding to prevent possible user count leakage — at Version 9

Reported by: Remy Owned by: Remy
Component: contrib.auth Version: dev
Severity: Normal Keywords:
Cc: Remy Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Remy)

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 encrypting of the user.pk value (PR https://github.com/django/django/pull/18539).

Change History (9)

comment:1 by Remy, 2 weeks ago

Description: modified (diff)

comment:2 by Simon Charette, 2 weeks ago

Triage Stage: UnreviewedAccepted

comment:3 by Natalia Bidart, 2 weeks ago

Owner: set to Remy
Status: newassigned
Version: 5.1dev

comment:4 by Natalia Bidart, 2 weeks ago

Needs documentation: set
Patch needs improvement: set

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.

comment:5 by Antoine Humbert, 2 weeks ago

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.

Last edited 2 weeks ago by Antoine Humbert (previous) (diff)

in reply to:  5 comment:6 by Remy, 2 weeks ago

Replying to 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.

comment:7 by Paolo Melchiorre, 2 weeks ago

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

comment:8 by Jake Howard, 2 weeks ago

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.

comment:9 by Remy, 11 days ago

Description: modified (diff)
Summary: Enhance password reset security by signing 'uid' parameter instead of base64-encoding to prevent possible user count leakageEnhance password reset security by encrypting 'uid' parameter instead of base64-encoding to prevent possible user count leakage
Note: See TracTickets for help on using tickets.
Back to Top