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)

comment:1 by Luke Plant, 3 years ago

Meant to say: this is probably controversial enough that it warrants some discussion on django-devs or somewhere.

in reply to:  1 comment:2 by Mariusz Felisiak, 3 years ago

Resolution: wontfix
Status: newclosed
Type: BugCleanup/optimization
Version: 4.0dev

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!

Note: See TracTickets for help on using tickets.
Back to Top