Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16285 closed Cleanup/optimization (fixed)

Remove a misleading comment from the signing code

Reported by: PaulM Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


lines 99 and 100 of the signing module indicate that the provided salt is used to prevent brute forcing. This is misleading, it's actually necessary for namespacing signed strings, so that users can't take a signed string from one part of Django and put it into a different part which will trust it since it was properly signed.

Attachments (1)

signing_comment.diff (809 bytes) - added by PaulM 5 years ago.

Download all attachments as: .zip

Change History (8)

Changed 5 years ago by PaulM

comment:1 Changed 5 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

I think you can use salt for both puposes:

  • increasing security — then you need to store the salt somewhere and re-inject in when you verify the hash.
  • preventing injection of data from one part of an application into another — then the salt can be static.

I accept the ticket because the second use-case sounds useful.

I set "patch needs improvement" because I'd like to make sure we don't dismiss the first one too quickly.

comment:2 Changed 5 years ago by PaulM

  • Patch needs improvement unset

In general, you're correct about different ways to use salt. In this case, that's not what's going on.

The "salt" here is going into a pre-seed for the salted-hmac module. That module already has other key material (the secret key) to prevent an attacker from forging the signature. Generally, salt is used to prevent pre-computed lookup table attacks, and is stored alongside the hashed material (and the plaintext is not stored at all). Salt is not meant to be kept more secret than the resulting hash.

In the signing module, we're using it differently. Since we expect the plaintext to be known to the attacker (this is why we're signing it, after all, to prove to ourselves that it hasn't been tampered with), using a salt to prevent rainbow table type attacks is pointless. We don't care that the attacker knows which plaintext hashes to which signature, since we provide both of those. What we do care about is that the secret key stay secret, since that's what prevents the attacker from generating their own signatures. Adding a salt doesn't change this property, since the attacker still has everything necessary to produce a signature except the secret key. Adding a salt produces a varying signature for the same input value, but this doesn't gain us any security, since all the previous salted signatures are also still valid, and the attacker knows the plaintext.

Using a random salt in this field makes your application less secure than using a single string for namespacing, since it allows a user to transfer a signature from one area of your program to another.

You might want to consult the original academic papers on HMAC, as well as look at the way salted_hmac is implemented in Django.

Last edited 5 years ago by PaulM (previous) (diff)

comment:3 Changed 5 years ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

Although I have a small background in cryptography, I wasn't familiar with HMAC. Thanks for the explanation, it was very clear.

Patch applies cleanly on SVN trunk.

comment:4 Changed 5 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

In [16458]:

Fixed #16285 -- Removed a misleading comment from the signing code, thanks PaulM.

comment:5 Changed 5 years ago by mitsuhiko

The documentation itself still mentions that the secret key can be used for preventing brute force attacks. Also I would recommend adding an example of why salting is a good thing to have. Feel free to steal from the one i have in itsdangerous:

comment:6 Changed 5 years ago by mitsuhiko

I added a separate ticket for that (#16369)

comment:7 Changed 5 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

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