encode / django-rest-framework

Web APIs for Django. 🎸
https://www.django-rest-framework.org
Other
28.37k stars 6.84k forks source link

There's an infinitesimally small possibility an existing token gets overwritten. #9250

Open CharString opened 8 months ago

CharString commented 8 months ago

https://github.com/encode/django-rest-framework/blob/5ad467aa2c76a7cfb6218f6dbe22e314131bde3c/rest_framework/authtoken/models.py#L9-L37

The sample space is large 2 ** (8 * 20), but the probability is not 0 that generate_key returns a key that already exists, and then the save would update the existing one.

The smallest change, turns this into an integrity error:

     def save(self, *args, **kwargs): 
         if not self.key: 
             self.key = self.generate_key()
             kwargs["force_insert"] = True
         return super().save(*args, **kwargs) 
tomchristie commented 7 months ago

Yep that looks like a very sensible measure. Would you like to go ahead and raise a pull request for this?

tomchristie commented 7 months ago

Reassuringly "large" is an understatment here. But yes, the force_insert/IntegrityError makes sense.

We should really be pointing towards our security policy and following issues like this up promptly, but the project maintainership has been severely over-extended recently. We're now getting back onto a more even keel.

Kludex commented 7 months ago

I'm not really into this project, but I saw this issue...

To the author of this issue: you shouldn't report security issues publicly.

To the maintainers: this should have been closed as soon as this was read, and recreated on another communication channel (email, private forum, etc) for triage.

tomchristie commented 7 months ago

This isn't an exploitable security issue in reality, tho we ought to have been be keeping good policy regardless yes.

browniebroke commented 7 months ago

Should this fix be considered a potential breaking change? e.g if some folks relied on the current behaviour to rotate an existing token. They would clear the key field and call save() on an existing token.

CharString commented 7 months ago

Should this fix be considered a potential breaking change? e.g if some folks relied on the current behaviour to rotate an existing token. They would clear the key field and call save() on an existing token.

Would that even work? I'll test that when I prepare the PR.