Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16285 closed Cleanup/optimization (fixed)

Remove a misleading comment from the signing code

Reported by: Paul McMillan 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 Paul McMillan 7 years ago.

Download all attachments as: .zip

Change History (8)

Changed 7 years ago by Paul McMillan

Attachment: signing_comment.diff added

comment:1 Changed 7 years ago by Aymeric Augustin

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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 7 years ago by Paul McMillan

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 7 years ago by Paul McMillan (previous) (diff)

comment:3 Changed 7 years ago by Aymeric Augustin

Triage Stage: AcceptedReady 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 7 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [16458]:

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

comment:5 Changed 7 years ago by Armin Ronacher

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 7 years ago by Armin Ronacher

I added a separate ticket for that (#16369)

comment:7 Changed 7 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

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