Opened 3 months ago
Last modified 2 weeks 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, Carlton Gibson, Natalia Bidart, Simon Charette, Ülgen Sarıkavak | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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 , 3 months ago
Description: | modified (diff) |
---|
comment:2 by , 3 months ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 3 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
Version: | 5.1 → dev |
comment:4 by , 3 months ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
follow-up: 6 comment:5 by , 3 months 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.
comment:6 by , 3 months 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 , 3 months ago
Is there no function in the standard libraries that can be used to increase the security level, without adding external dependencies?
comment:8 by , 3 months 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 , 2 months ago
Description: | modified (diff) |
---|---|
Summary: | Enhance password reset security by signing 'uid' parameter instead of base64-encoding to prevent possible user count leakage → Enhance password reset security by encrypting 'uid' parameter instead of base64-encoding to prevent possible user count leakage |
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.