Re: [Django] #35730: Enhance password reset security by encrypting 'uid' parameter instead of base64-encoding to prevent possible user count leakage
20 views
Skip to first unread message
Django
Sep 9, 2024, 5:11:45 AM9/9/24
to django-...@googlegroups.com
#35730: Enhance password reset security by encrypting '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
--------------------------------------+------------------------------------
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~~ encrypting of the `user.pk` value
> (PR https://github.com/django/django/pull/18539).
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~~ 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>/`).
--
Comment (by Remy):
[comment:8 Jake Howard] [comment:5 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:10>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
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
--------------------------------------+------------------------------------
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~~ encrypting of the `user.pk` value
> (PR https://github.com/django/django/pull/18539).
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~~ 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>/`).
--
Comment (by Remy):
[comment:8 Jake Howard] [comment:5 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:10>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Sep 9, 2024, 5:15:23 AM9/9/24
to django-...@googlegroups.com
#35730: Enhance password reset security by encrypting '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):
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
--------------------------------------+------------------------------------
Replying to [comment:4 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:11>
Django
Oct 1, 2024, 3:01:14 PM10/1/24
to django-...@googlegroups.com
#35730: Enhance password reset security by encrypting '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: 1 | Patch needs improvement: 1
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
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by Natalia Bidart):
--------------------------------------+------------------------------------
* needs_tests: 0 => 1
Comment:
Replying to [comment:11 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
still work.
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
[https://docs.djangoproject.com/en/dev/internals/contributing/writing-code
/submitting-patches/#patch-style these docs].
--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:12>
Django
Oct 27, 2024, 3:49:22 PM10/27/24
to django-...@googlegroups.com
#35730: Enhance password reset security by encrypting '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: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by Simon Charette):
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: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* cc: Carlton Gibson, Natalia Bidart, Simon Charette (added)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:13>
Django
Nov 7, 2024, 2:38:14 AM11/7/24
to django-...@googlegroups.com
#35730: Enhance password reset security by encrypting '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: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by Ülgen Sarıkavak):
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: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:14>
Django
Jan 31, 2025, 8:34:37 PM1/31/25
to django-...@googlegroups.com
#35730: Enhance password reset security by encrypting '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: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Adam):
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: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
If the pull request is not going to get merged then maybe a good
compromise would be moving [the logic for encoding the user's primary
key][https://github.com/django/django/blob/5.1.5/django/contrib/auth/forms.py#L481]
into its own function so that it's easier to override in a subclass. The
logic for decoding the user's primary key is already easy to override in a
subclass.
--
Ticket URL: <https://code.djangoproject.com/ticket/35730#comment:15>
Reply all
Reply to author
Forward
0 new messages