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

[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).

UPDATE: moved on from signing to encrypting. Details bellow:

Implementation details

Since Python’s standard library does not include encryption utilities, and because the goal is only to obfuscate data that was previously not obfuscated, the need for a highly secure encryption algorithm is somewhat mitigated. Adding a new dependency to Django just for this seemed unnecessary. Therefore, I opted for a straightforward implementation of a XOR cipher.

The cipher key is derived from Django’s SECRET_KEY and salted.

Security considerations

In the case of a XOR cipher, the worst-case scenario is that an attacker manages to obtain the cipher key. However, even if this happened, all the attacker would be able to do is decrypt the user.pk, which is currently being sent almost as plain text in the default implementation anyway. Regarding other potential uses of the key:

• Since the key is derived from a salted hash of the SECRET_KEY, it is not reversible, and the salt protects against rainbow table attacks (for example, if SECRET_KEY was something weak like "abc").
• The key is not reused anywhere else in the application and remains scoped only to this encryption/decryption functionality, ensuring the attack surface is limited to this single feature.

It’s also worth noting that it is highly unlikely that an attacker would even obtain the full cipher key to begin with, due to the nature of XOR encryption. To do so, they would need to use plain-text messages that are at least as long as the cipher key. For this purpose, we use a SHA-512 derived key, which is 64 bytes long, while most user.pk values use BigAutoField which maximum value 9223372036854775807 is only 19 bytes. An implementation of user.pk using UUIDField and a UUID4 would also be significantly shorter (36 bytes).

We could go further and introduce a salt that changes with each request. For instance, we could use a timestamp as the salt during encryption and decryption. The timestamp would travel with the reset link and return with the request. We actually already have such a timestamp traveling within the token parameter (ts_b36). So if we were to go this route, it would be logical to re-use this ts_b36 for salting the cipher key, and we could even include the encypted uid in the token parameter itself (along with f'{ts_b36}-{hash_string}') and remove uidb64 from url parameters. However, this dynamic salting would require rewriting the logic inside PasswordResetConfirmView and I would argue that we're already doing too much since we're really just obfuscating something that was in clear until now.

With the current implementation, we achieve most of the desired benefits without needing to rewrite that PasswordResetConfirmView logic, and the URL parameters remain unchanged (reset/<uidb64>/<token>/).

[Natalia Bidart]

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.

[Antoine Humbert]

How does this adress the uid leakage ? Signing is not encrypting. Even if signed, the uid is still present and clearly visible in the reset link.

[Remy]

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.

[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

[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.

[Remy]

Jake Howard Antoine Humbert
I went ahead and updated the branch to use encryption instead of signing. I updated the ticket description to include all the details and rationale about the implementation. Please have a look!

[Remy]

Replying to Natalia Bidart:

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.

Thank you for the feedback! I updated the commit so that the old links still work.

[Natalia Bidart]

Replying to Remy:

Thank you for the feedback! I updated the commit so that the old links still work.

Thank you Remy, I have provided further comments in the PR. In the future please be sure to adjust the flags in this ticket to move the PR back to the review queue.
The proper procedure to get a re-review is described in ​these docs.

[Simon Charette]

Hello Remy, thank you for your patch submission.

Over the past week the security team had the chance to review it and after some deliberation we came to the conclusion that we might want to change our stance and consider this ticket as won't fix for the following reasons

• As discussed previously this only addresses one way that user ids can be enumerated and the only true way to prevent this problem from happening is to switch to non-enumerable user-facing identifiers (e.g. UUIDs) which Django supports.
• While your salted XOR implementation appears to be correct (thanks for trying it out btw) we don't believe that the nature of this problem (enumeration) warrants the complexity it introduces.
• We considered that we might want to repurpose to this ticket to make it easier for interested user to rollout their own implementation of User -> uid -> User but we feared that it might be to niche and cause more harm than good if it was to be poorly overridden.
• Lastly we discussed that instead of sending along the UserModel.pk in the uid could send the UserModel.USERNAME_FIELD attribute which is also unique and due to the inclusion of the pk in the hash would be immediately invalidated on a username swap. After all uid is user controlled so any field that allows for unique retrieval of a user should (safely work)?

I'd like to personally apologize for sending you down the rabbit hole and keeping you waiting for so long for feedback, we were truly unsure how to proceed here and needed to reach a consensus to avoid even more confusion. With all that said we'd like to give you the opportunity to chime in before we move forward with what we believe should have been the initial response to your report.