Opened 3 years ago
Closed 3 years ago
#33806 closed Cleanup/optimization (wontfix)
The `salt` argument for signing functions should not be optional.
Reported by: | Luke Plant | Owned by: | nobody |
---|---|---|---|
Component: | Utilities | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The docs currently have a reasonable explanation of why the salt
argument is needed: https://docs.djangoproject.com/en/4.0/topics/signing/#using-the-salt-argument
The problem is that because salt is an optional argument, no-one reads those docs. Our example code also doesn't give a good introduction, which it can get away with because salt is optional.
By now, I've seen far too many instances of people not supplying salt and introducing security vulnerabilities. There is basically never a case when you shouldn't supply it, the possibility of insecure usage is too high, and often it is impossible to audit. If you are using several third party libraries that use the signing functions without salt, they can easily be introducing vulnerabilities into each other's functionality and this will be invisible. Enforcing salt, and setting good examples of how to use it (e.g. long, fully qualified dotted names as a default minimum) would go a long way to fixing this.
There are also plenty of cases I've seen where functionality can be used to generate signatures of data with a high degree of control by the attacker. For example, a service that signs the PK of an object and returns that to the user (e.g. as an access token), and also allows the user to create many objects, which will have sequentially increasing PKs, allowing the user to get signatures for a large number of integers. These can then be re-used anywhere a sig for an integer is expected. This makes the probability of exploitation higher.
Also, the sooner we can introduce people to salt in our docs, the better. You shouldn't be using signing if you don't have an understanding of the fact that you need to carefully design the scope of a signature so that the signature cannot be re-used inappropriately.
This issue affects Signer
, TimestampSigner
and the utilities dumps
and loads
.
The immediate issue it brings is that as soon as you start supplying unique salt, existing signatures will break, so people will cheat and do salt="django.core.signing"
leaving us back where we were. This is also an issue for rotating your SECRET_KEY - it's too hard to do it without breaking everything, so people don't even when they should.
So my proposal is that we also add utilities to help with this situation. In particular, we should add a FallbackSigner
which will only generate signatures using the current secret/salt combo, but will also have a list of fallbacks that it will use for unsigning only.
A related issue is whether dumps
and loads
are actually useful. If you need to configure signing with a salt argument every time, it is less repetition to specify it once in the Signer and then use that instance's sign and unsign methods. I propose deprecating these utilities.
Change History (2)
follow-up: 2 comment:1 by , 3 years ago
comment:2 by , 3 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Type: | Bug → Cleanup/optimization |
Version: | 4.0 → dev |
Replying to Luke Plant:
Meant to say: this is probably controversial enough that it warrants some discussion on django-devs or somewhere.
Yes, this is a breaking change so we need to have a proper discussion and reach a consensus on the DevelopersMailingList. The backward compatible deprecation path can be tricky.
Marked as "wontfix" pending discussion. Thanks!
Meant to say: this is probably controversial enough that it warrants some discussion on django-devs or somewhere.