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

[CharString]

django-rest-framework/rest_framework/authtoken/models.py

Lines 9 to 37 in 5ad467a

	class Token(models.Model):
	"""
	The default authorization token model.
	"""
	key = models.CharField(_("Key"), max_length=40, primary_key=True)
	user = models.OneToOneField(
	settings.AUTH_USER_MODEL, related_name='auth_token',
	on_delete=models.CASCADE, verbose_name=_("User")
	)
	created = models.DateTimeField(_("Created"), auto_now_add=True)
	class Meta:
	# Work around for a bug in Django:
	# https://code.djangoproject.com/ticket/19422
	#
	# Also see corresponding ticket:
	# https://github.com/encode/django-rest-framework/issues/705
	abstract = 'rest_framework.authtoken' not in settings.INSTALLED_APPS
	verbose_name = _("Token")
	verbose_name_plural = _("Tokens")
	def save(self, *args, **kwargs):
	if not self.key:
	self.key = self.generate_key()
	return super().save(*args, **kwargs)
	@classmethod
	def generate_key(cls):
	return binascii.hexlify(os.urandom(20)).decode()

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) 

[lovelydinosaur]

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

[lovelydinosaur]

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]

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.

[lovelydinosaur]

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

[browniebroke]

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]

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.

[mahdirahimi1999]

Hi everyone,

I'd like to work on this issue. My plan is to implement the solution discussed (using force_insert for new model instances to prevent overwrites) and also modernize the generate_key method to use secrets.token_hex.

I'll submit a pull request with the code changes and a complete test suite shortly.

Thanks!