classification
Title: Adding salt and Modular Crypt Format to crypt library.
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: SilentGhost, brett.cannon, christian.heimes, dmalcolm, ezio.melotti, jafo, nicdumz, pitrou, python-dev, rhettinger
Priority: normal Keywords: buildbot, easy, patch

Created on 2011-01-17 07:06 by jafo, last changed 2012-06-27 14:02 by christian.heimes. This issue is now closed.

Files
File name Uploaded Description Edit
python-underscore_crypt-7.patch jafo, 2011-01-19 05:08 Removed duplication, docs fix, modules() now a list.
crypt.py.diff SilentGhost, 2011-02-22 15:44
Messages (25)
msg126393 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2011-01-17 07:06
Over the years I've written the same code over and over to create a random salt string of 2 characters.  Worse, the Modular Crypt Format is difficult to find documentation on, so creating stronger hashed passwords is difficult to get right.

To that end, I'm proposing the addition of a "mksalt()" method which will generate a salt, and several METHOD_* values to select which hashing method to use.

I also figure there will need to be a "methods()" call that figures out what methods are available in the library crypt() and return a list of the available ones.

If we have a way to generate a salt, then I figure we could drop the salt argument of crypt.crypt(), and if not specified to generate one.  So to hash a password you could do: "crypt.crypt('password')".

I figure that the best way to accomplish this is to implement this all in Python and move the existing C crypt module to _crypt.

A patch accomplishing this is attached.  Please review.

Attached is a patch to accomplish this.
msg126423 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-01-17 18:14
You forgot to add the new files to your patch.
msg126425 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2011-01-17 18:40
Oops, thanks.  It's in there now, though for some reason I can't get this patch to apply to trunk, but I'll have to look at that later this afternoon.  I wanted to get this new version up in the interim since it definitely does include the Lib/crypt.py file, the heart of the changes.
msg126437 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2011-01-17 21:29
I've made a new .patch file using "diff -c" rather than "svn diff".  This is the same code, but applies without manual intervention.
msg126442 - (view) Author: Nicolas Dumazet (nicdumz) Date: 2011-01-18 00:36
Hello,

1) Can you please avoid putting several statements in the same line?

2) wouldnt it be better to compute only once the contents of methods()? I'm not sure that module-initialization time is okay for CPython, but at the very least you can lazily fill a module-level variable, and return it directly from methods()?

3) what happens when a user uses one of the Crypt methods that are referenced from the Module, if this method is not available? Arguably, if I know what I'm doing, I will call mksalt(METHOD_SHA512) without checking that METHOD_SHA512 was in methods(). That's not very intuitive, and it seems that mksalt could break.

4) saltchars should probably be string.ascii_letters+string.digits instead of the hardcoded value

5) you should mention in the documentation that if not salt parameter is given, a different salt will be used for each crypt() call

6) is _MethodClass an old-style class?

7) it seems that the patch duplicates twice the diff of crypt.py, not sure of what happened there?
msg126453 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2011-01-18 05:06
Thanks for the review.  Attached is a new version of the patch.

1) Done.

2) Good point, I didn't think of that.  I've changed it into a class that stores the methods list, and made the module "methods" point to that method on an instance of that class.

3) This entirely depends on the underlying C library implementation of crypt.  It won't cause mksalt() to blow up, it's just that the crypt(3) call won't know how to deal with it.  On my Linux system using glibc, it simply uses the first two characters as the salt, which isn't entirely surprising except that "$" is not a valid salt character according to the standards.

4) I was being lazy and not looking up the locale implications of doing that.  They look fine, so I've changed it to use that.  Good suggestion.

5) I almost did that, but I figured that, generating a random salt, it was obvious that the return value would be different for the same result.  However, since you mentioned it as well, I've added a note.

6) I don't know, I thought everything in Python 3 was a new style class?

7) I don't see that.  Perhaps you mis-read "Lib/test/test_crypt.py" as being another copy of "Lib/crypt.py"?  In any case, I don't see it in v3 or v4 (the one addressing your questions).
msg126465 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-01-18 14:10
Can you use "diff -u" (or simply "svn diff") when generating a patch?

> 6) I don't know, I thought everything in Python 3 was a new style
> class?

It is indeed.
msg126491 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2011-01-18 20:16
Sure thing, here's an "svn diff".  I had switched to the diff because I couldn't get it to patch into a fresh trunk, but the format looked fine; not sure why it couldn't find the files.  Anyway, here's a new version.
msg126492 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-01-18 20:28
> Sure thing, here's an "svn diff".  I had switched to the diff because
> I couldn't get it to patch into a fresh trunk, but the format looked
> fine; not sure why it couldn't find the files.  Anyway, here's a new
> version.

You also have to "svn add" the relevant files :)
msg126495 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2011-01-18 21:35
Not sure if that was meant to be a suggestion for why my local patching wasn't working from the "svn diff" output, but obviously -5 was messed up.  Here's a new version that I can apply to my fresh trunk and passes "make test".

If the suggestion was how to fix my patching from "svn diff", the problem I ran into was that it had the files in it, say crypt.py, but it was trying to apply them as if I had specified "patch -p1", even though the "svn diff" contained the paths and I hadn't done "-p1".

Anyway, this diff is "diff -urN".  I just can't win, I usually use "diff -u", but in the distant past Guido asked me for "diff -c" instead.  :-)
msg126497 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-01-18 21:46
> Not sure if that was meant to be a suggestion for why my local
> patching wasn't working from the "svn diff" output, but obviously -5
> was messed up.  Here's a new version that I can apply to my fresh
> trunk and passes "make test".

Thank you! The important is that we now have a workable patch in unified
diff format :)

> If the suggestion was how to fix my patching from "svn diff", the
> problem I ran into was that it had the files in it, say crypt.py, but
> it was trying to apply them as if I had specified "patch -p1", even
> though the "svn diff" contained the paths and I hadn't done "-p1".

For the record, when using "svn diff", you have to use "patch -p0" to
apply the resulting patch. Not -p1.

> Anyway, this diff is "diff -urN".  I just can't win, I usually use
> "diff -u", but in the distant past Guido asked me for "diff -c"
> instead.  :-)

I would bet even Guido changed his habits :) Rietveld computes and
displays unified diffs as far as I remember.
msg126509 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2011-01-19 00:28
Thanks for the pointer about "patch -p0".  I *HAD* tried that, but it didn't seem to work either.  I'll double check that though...  "svn diff" is what I'd prefer, because then I can "svn commit" it when it's ready.

Any other review feedback?  I'll probably let this sit until 3.2 goes to maintenance and then check it into trunk, so there's some time yet...
msg126510 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-01-19 00:43
> Thanks for the pointer about "patch -p0".  I *HAD* tried that, but it
> didn't seem to work either.  I'll double check that though...  "svn
> diff" is what I'd prefer, because then I can "svn commit" it when it's
> ready.

Ok, it seems the code inside crypt.py is duplicated in your patch.
Also, when you commit, it'll be better if you use "svn rename" for the C
file, so that history isn't broken.

> Any other review feedback?  I'll probably let this sit until 3.2 goes
> to maintenance and then check it into trunk, so there's some time
> yet...

Looks good mostly. Why do you need _MethodListClass()? Executing code at
module startup sounds fine to me. Or, at worse, use a global variable.

+   *salt* (either a random 2 or 16 character string, possibly prefixed with
+   ``$digit$`` to indicate the method) which will be used to perturb the
+   encryption algorithm.  The characters in *salt* must be in the set
+   ``[./a-zA-Z0-9]``, with the exception of Modular Crypt Format which
+   prefixes a ``$digit$``.

That paragraph is a bit confusing. Also, other uses of *salt* are
described separately two paragraphs above.
msg126518 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2011-01-19 05:08
Affirmative on the "svn mv" for the C module.

The duplicated code, thanks for pointing that out.  Someone else mentioned it, but I didn't understand what they were saying and they didn't reply to my request for clarification.  Fixed.

On the modules() list, how about if I just make it a list and build it at import time?  The class was the way I thought most straightforward to do it as a function, so maybe this is more reasonable?

Per the documentation, I pulled down the description from above, which I think captured the uses of *salt* and removed the duplication.
msg126849 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2011-01-22 18:41
At this point I'm going to consider this good to go, and will commit it after the 3.2 final release.  Thanks for the review everyone.  Of course, I'm open to further suggestions until then, just not expecting any...
msg126851 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-01-22 18:43
Actually, the "pending" stage is only for when things have been committed :)
See http://docs.python.org/devguide/triaging.html#triaging
msg126918 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2011-01-24 10:24
Thanks.  I had just read that a day or so ago, reviewing it for Brett's work.
msg129049 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2011-02-22 10:57
Committed in r88500.
msg129060 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-02-22 12:24
Some buildbots are failing after the commit.

Also in the crypt.py module I still see things that according to msg126453 should be fixed already:
 * more statements on the same line (e.g. "if salt == None: salt = mksalt()");
 * the hardcoded salt values instead of string.ascii_letters;

According to the PEP8 there shouldn't be any spaces after the '[' and before the ']' (e.g. "method_list = [ METHOD_SHA512, METHOD_SHA256, METHOD_MD5 ]" and in the listcomps) and around the = in the function/method declarations/calls (e.g. "def crypt(word, salt = None):").
msg129089 - (view) Author: SilentGhost (SilentGhost) Date: 2011-02-22 15:44
Here is the patch fixing pep-8 compatibility and test. It is against the latest commit.
msg129131 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-02-22 21:20
I will look at the patch.
msg129135 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-02-22 21:47
The patch didn't even import as-is or past the tests, but I tweaked it so it did (and made method() just an attribute on the module).
msg130172 - (view) Author: SilentGhost (SilentGhost) Date: 2011-03-06 14:05
Above-mentioned fix was committed in 0586c699d467 and 62994662676a
msg164058 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-06-26 08:53
I just found mksalt in the whatsnew section and got curious how you've implemented the function. IMHO it has one major security flaw.

The function uses random.choice(). The choice() function generates random values with a Mersenne Twister. However MTs are not suited for any cryptographic purpose and must not be used to generate passwords, session keys or salts.

The random.SystemRandom class uses os.urandom() as source which is a wrapper around /dev/urandom or the Windows crypto API. The output is suitable for short living states and salts.

I'm going to chance the implementation to a global instance of random.SystemRandom() and _sr.samples() as soon as Georg has cut beta 1.

_sr = random.SystemRandom()

s += ''.join(_sr.samples(_saltchars, method.salt_chars))
msg164155 - (view) Author: Roundup Robot (python-dev) Date: 2012-06-27 13:37
New changeset 74a1110a3b50 by Christian Heimes in branch 'default':
Issue 10924: Fixed mksalt() to use a RNG that is suitable for cryptographic purpose
http://hg.python.org/cpython/rev/74a1110a3b50
History
Date User Action Args
2012-06-27 14:02:35christian.heimessetstatus: open -> closed
resolution: fixed
2012-06-27 13:37:56python-devsetnosy: + python-dev
messages: + msg164155
2012-06-26 08:53:52christian.heimessetstatus: closed -> open

type: enhancement -> security
assignee: brett.cannon -> christian.heimes

nosy: + christian.heimes
messages: + msg164058
resolution: fixed -> (no value)
2011-03-06 14:06:08SilentGhostsetnosy: brett.cannon, rhettinger, jafo, pitrou, ezio.melotti, nicdumz, SilentGhost, dmalcolm
messages: - msg130171
2011-03-06 14:05:41SilentGhostsetnosy: brett.cannon, rhettinger, jafo, pitrou, ezio.melotti, nicdumz, SilentGhost, dmalcolm
messages: + msg130172
2011-03-06 14:00:44SilentGhostsetnosy: brett.cannon, rhettinger, jafo, pitrou, ezio.melotti, nicdumz, SilentGhost, dmalcolm
messages: + msg130171
2011-03-03 22:14:45dmalcolmsetnosy: + dmalcolm
2011-02-22 21:47:07brett.cannonsetstatus: open -> closed
nosy: brett.cannon, rhettinger, jafo, pitrou, ezio.melotti, nicdumz, SilentGhost
messages: + msg129135

resolution: accepted -> fixed
stage: patch review -> resolved
2011-02-22 21:20:38brett.cannonsetnosy: + brett.cannon
messages: + msg129131

assignee: jafo -> brett.cannon
stage: resolved -> patch review
2011-02-22 15:44:53SilentGhostsetstatus: closed -> open
files: + crypt.py.diff

nosy: + SilentGhost
messages: + msg129089
2011-02-22 12:24:30ezio.melottisetkeywords: + buildbot
nosy: + ezio.melotti
messages: + msg129060

2011-02-22 10:57:12jafosetstatus: open -> closed

messages: + msg129049
stage: commit review -> resolved
2011-01-24 10:24:55jafosetmessages: + msg126918
2011-01-22 18:43:39pitrousetstatus: pending -> open

messages: + msg126851
resolution: later -> accepted
stage: patch review -> commit review
2011-01-22 18:41:18jafosetstatus: open -> pending
messages: + msg126849

assignee: jafo
keywords: - needs review
resolution: later
2011-01-19 05:10:30jafosetfiles: - python-underscore_crypt-6.patch
2011-01-19 05:10:24jafosetfiles: - python-underscore_crypt-5.patch
2011-01-19 05:10:15jafosetfiles: - python-underscore_crypt-4.patch
2011-01-19 05:08:23jafosetfiles: + python-underscore_crypt-7.patch

messages: + msg126518
2011-01-19 00:43:40pitrousetmessages: + msg126510
2011-01-19 00:28:34jafosetmessages: + msg126509
2011-01-18 21:46:19pitrousetmessages: + msg126497
2011-01-18 21:35:47jafosetfiles: + python-underscore_crypt-6.patch

messages: + msg126495
2011-01-18 20:28:02pitrousetmessages: + msg126492
2011-01-18 20:16:14jafosetfiles: + python-underscore_crypt-5.patch

messages: + msg126491
2011-01-18 14:10:19pitrousetmessages: + msg126465
2011-01-18 11:41:49jafosetfiles: - python-underscore_crypt-3.patch
2011-01-18 05:06:52jafosetfiles: + python-underscore_crypt-4.patch

messages: + msg126453
2011-01-18 00:36:13nicdumzsetnosy: + nicdumz
messages: + msg126442
2011-01-17 21:29:48jafosetfiles: - python-underscore_crypt-2.patch
2011-01-17 21:29:13jafosetfiles: + python-underscore_crypt-3.patch

messages: + msg126437
2011-01-17 18:40:30jafosetfiles: - python-underscore_crypt.patch
2011-01-17 18:40:14jafosetfiles: + python-underscore_crypt-2.patch

messages: + msg126425
2011-01-17 18:14:52pitrousetnosy: + pitrou
messages: + msg126423
2011-01-17 11:36:08rhettingersetnosy: + rhettinger
2011-01-17 07:06:33jafocreate