Code

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#9687 closed (wontfix)

Use more randomness in secret key generation

Reported by: paulway@… Owned by: nobody
Component: Core (Other) Version: 1.0
Severity: Keywords: SECRET_KEY random
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The generation of the SECRET_KEY setting for a new site uses an artificially low number of characters due to a design accident. As far as I can see, SECRET_KEY is not used in a way which would make it case-sensitive or require it to be read out, but instead is used in MD5 hashes where more randomness is preferred.

The supplied patch not only adds more characters to be selected from but also makes an unreadable one-liner into a readable two-liner.

Attachments (1)

more_randomness_in_secret_key.patch (851 bytes) - added by paulway@… 5 years ago.
Patch in svn diff format

Download all attachments as: .zip

Change History (4)

Changed 5 years ago by paulway@…

Patch in svn diff format

comment:1 Changed 5 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to wontfix
  • Status changed from new to closed

You don't explain what the "design accident" might be. If you are referring to the particular choice of characters, that is hardly an accident.

The question isn't whether the string is as random as possible (your alternative doesn't meet that requirement either), it's whether it's sufficiently random as to be effectively unguessable. The current code has 5050 different strings (around 1085) that are possible and the choices are statistically uniformly distributed over that space. Your are proposing to extend the range of characters, but that doesn't change much. Since the chance of guessing the secret key string for a particular site is already unlikely (a billion guesses per second will take 1068 years), the key weakness is the PRNG being used to gerneated the choices and that security isn't improved by increased the range of characters (if you can accurately determine the state at the start of the string, you can still determine the whole string, regardless of the number of characters used).

At some point here, we just have to make a choice. Why not use more characters than your range? Why not switch to 150 characters in the secret, etc?

If you want to use a longer secret key in your projects, that's certainly going to be possible, since it's just a string and you can create it however you like. The one that Django generates isn't inherently insecure, though.

comment:2 Changed 5 years ago by paulway@…

Whatever.

The issue shouldn't be whether it's "good enough", the issue should be whether it improves things. You've got someone else not only noticing a problem, but actually supplying you with source code, and it's not acceptable simply because the current system meets your own standards for 'fitness'.

I could point to http://code.djangoproject.com/ticket/1180 as an example of the "works for me" approach breaking, as well as being an example of Django developers not really getting basic cryptographic randomness issues, but then I'd get accused of being petty.

The question of whether your accepting reasonable patches or not is your own problem.

comment:3 Changed 5 years ago by mtredinnick

Perhaps you could leave the passive-aggressive attitude and ad hominem attachs at home next time. I'm grateful you looked at this and posted a patch, but that doesn't make it a slam dunk. Your assertion about our understanding isn't correct, primarily because we're not using MD5 hashing for cryptographic purposes (we're using the uniform hashing properties). For example, #1180 is an example of why PRNG handling has to be done carefully and it was a bug that was fixed carefully.

If you have an actual technical problem with this being closed, feel free to raise it in the appropriate way. I closed this because it doesn't significantly increase the security of the system. The current key length is already long enough to cover the space of generated MD5 digests twice over (and using a reduced alphabet makes the unlikely event of generating a false pre-image less likely). Changing code just because we can is never a goal in software.

I'm sorry you feel your patch was unfairly rejected, but it's not because of the reasons you mention and you're being unfair to myself and others.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.