Changes between Version 9 and Version 10 of Ticket #35730


Ignore:
Timestamp:
Sep 9, 2024, 4:11:40 AM (3 months ago)
Author:
Remy
Comment:

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!

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #35730 – Description

    v9 v10  
    66
    77To 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).
     8
     9**UPDATE**: moved on from signing to encrypting. Details bellow:
     10
     11**Implementation details**
     12
     13Since 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.
     14
     15The cipher key is derived from Django’s `SECRET_KEY` and salted.
     16
     17**Security considerations**
     18
     19In 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:
     20
     21- 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").
     22- 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.
     23
     24It’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).
     25
     26We 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.
     27
     28With 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>/`).
Back to Top