-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
Refs #35730: Encrypted 'uid’ parameter instead of base64-encoding to prevent possible user count leakage #18539
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,8 +1,15 @@ | ||||||
import hashlib | ||||||
from datetime import datetime | ||||||
|
||||||
from django.conf import settings | ||||||
from django.utils.crypto import constant_time_compare, salted_hmac | ||||||
from django.utils.http import base36_to_int, int_to_base36 | ||||||
from django.utils.encoding import force_bytes, force_str | ||||||
from django.utils.http import ( | ||||||
base36_to_int, | ||||||
int_to_base36, | ||||||
urlsafe_base64_decode, | ||||||
urlsafe_base64_encode, | ||||||
) | ||||||
|
||||||
|
||||||
class PasswordResetTokenGenerator: | ||||||
|
@@ -128,5 +135,40 @@ def _now(self): | |||||
# Used for mocking in tests | ||||||
return datetime.now() | ||||||
|
||||||
def _xor_encrypt_decrypt(self, uid): | ||||||
""" | ||||||
Performs XOR encryption/decryption on a uid to obfuscate | ||||||
its value in the reset link. | ||||||
This approach avoids adding a new dependency for encryption, | ||||||
which is not natively part of Python's standard library. | ||||||
The cypher key is a salted hash of the SECRET_KEY. | ||||||
It is important that the cipher key is at least as long as the uid. | ||||||
We use the SHA-512 hash algorithm to ensure a long enough cipher key (64 bytes) | ||||||
to encrypt UUID4s (36 bytes) that might be used as primary key. | ||||||
BigAutoField (the default primary key) is also supported since | ||||||
it has a maximum size of 19 bytes. We cycle the key to also support the | ||||||
unlikely scenario that the uid is longer than 64 bytes. | ||||||
""" | ||||||
key = hashlib.sha512(force_bytes(f"{self.key_salt}{self.secret}")).digest() | ||||||
uid_bytes = force_bytes(uid) | ||||||
xor_ciphertext = bytes( | ||||||
a ^ b for a, b in zip(uid_bytes, (key * (len(uid_bytes) // len(key) + 1))) | ||||||
) | ||||||
return xor_ciphertext | ||||||
|
||||||
def encrypt_uid(self, uid): | ||||||
""" | ||||||
Returns a XOR-encrypted user id for use in the password reset mechanism. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docstrings should follow PEP-8 and PEP-257, specifically verbs should be written as a command. An alternative proposal would be:
Suggested change
|
||||||
""" | ||||||
xor_ciphertext = self._xor_encrypt_decrypt(uid) | ||||||
return urlsafe_base64_encode(xor_ciphertext) | ||||||
|
||||||
def decrypt_uid(self, encrypted_uidb64): | ||||||
""" | ||||||
Returns the decrypted user id given the base64-encoded encrypted user id. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
""" | ||||||
xor_ciphertext = urlsafe_base64_decode(encrypted_uidb64) | ||||||
return force_str(self._xor_encrypt_decrypt(xor_ciphertext)) | ||||||
|
||||||
|
||||||
default_token_generator = PasswordResetTokenGenerator() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -298,9 +298,9 @@ def dispatch(self, *args, **kwargs): | |
return self.render_to_response(self.get_context_data()) | ||
|
||
def get_user(self, uidb64): | ||
user = None | ||
try: | ||
# urlsafe_base64_decode() decodes to bytestring | ||
uid = urlsafe_base64_decode(uidb64).decode() | ||
uid = self.token_generator.decrypt_uid(uidb64) | ||
user = UserModel._default_manager.get(pk=uid) | ||
except ( | ||
TypeError, | ||
|
@@ -309,7 +309,22 @@ def get_user(self, uidb64): | |
UserModel.DoesNotExist, | ||
ValidationError, | ||
): | ||
user = None | ||
pass | ||
|
||
# Temporarily support the old format of uid base64-encoded as plain text | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is good, supporting old links. But we can't have this code forever in Django, we need a clear and documented deprecation path. My advice is to follow the deprecating a feature docs and raise a deprecation warning for each successful hit for old-style links. I do wonder if we need a more clever approach here to handle the deprecation... There is an ongoing conversation about signing CSRF cookies which has some common points with this. I will reference this PR in the forum topic but at this point I think we need to involve the community to design a good plan for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, whatever we end up doing for the deprecation/transition of link and token obfuscation, we need to add tests for it. |
||
if user is None: | ||
try: | ||
uid = urlsafe_base64_decode(uidb64).decode() | ||
user = UserModel._default_manager.get(pk=uid) | ||
except ( | ||
TypeError, | ||
ValueError, | ||
OverflowError, | ||
UserModel.DoesNotExist, | ||
ValidationError, | ||
): | ||
pass | ||
|
||
return user | ||
|
||
def get_form_kwargs(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,8 @@ Minor features | |
* The default iteration count for the PBKDF2 password hasher is increased from | ||
870,000 to 1,000,000. | ||
|
||
* The ``uid`` parameter in password reset emails is now obfuscated to prevent potential leakage of user count. This change hides the user's primary key (``user.pk``), which was previously only base64-encoded. The obfuscation is achieved using a simple XOR cipher. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All the docs and docstrings lines should be wrapped at 79cols. Could you please summarize this a bit and ensure it wraps at that line length? |
||
|
||
:mod:`django.contrib.contenttypes` | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two "public" APIs being added to this method should be fully tested, including the corner cases described in the docstring (thanks for the thorough details).