[Django] #33806: The `salt` argument for signing functions should not be optional.
Django
--------------------------------------+------------------------
Reporter: Luke Plant | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 4.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33806>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Luke Plant | Owner: nobody
Type: Bug | Status: new
Component: Utilities | Version: 4.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Luke Plant):
Meant to say: this is probably controversial enough that it warrants some
discussion on django-devs or somewhere.
--
Ticket URL: <https://code.djangoproject.com/ticket/33806#comment:1>
Django
Reporter: Luke Plant | Owner: nobody
Cleanup/optimization |
Component: Utilities | Version: dev
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* type: Bug => Cleanup/optimization
* version: 4.0 => dev
* resolution: => wontfix
Comment:
Replying to [comment:1 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/33806#comment:2>