Adding salt and Modular Crypt Format to crypt library.

[jafo]

BPO	10924
Nosy	@brettcannon, @rhettinger, @pitrou, @tiran, @ezio-melotti, @davidmalcolm
Files	python-underscore_crypt-7.patch: Removed duplication, docs fix, modules() now a list. crypt.py.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/tiran'
closed_at = <Date 2012-06-27.14:02:35.043>
created_at = <Date 2011-01-17.07:06:33.214>
labels = ['type-security', 'easy', 'library']
title = 'Adding salt and Modular Crypt Format to crypt library.'
updated_at = <Date 2012-06-27.14:02:35.042>
user = 'https://bugs.python.org/jafo'

bugs.python.org fields:

activity = <Date 2012-06-27.14:02:35.042>
actor = 'christian.heimes'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2012-06-27.14:02:35.043>
closer = 'christian.heimes'
components = ['Library (Lib)']
creation = <Date 2011-01-17.07:06:33.214>
creator = 'jafo'
dependencies = []
files = ['20449', '20840']
hgrepos = []
issue_num = 10924
keywords = ['patch', 'easy', 'buildbot']
message_count = 25.0
messages = ['126393', '126423', '126425', '126437', '126442', '126453', '126465', '126491', '126492', '126495', '126497', '126509', '126510', '126518', '126849', '126851', '126918', '129049', '129060', '129089', '129131', '129135', '130172', '164058', '164155']
nosy_count = 10.0
nosy_names = ['brett.cannon', 'rhettinger', 'jafo', 'pitrou', 'christian.heimes', 'ezio.melotti', 'nicdumz', 'SilentGhost', 'dmalcolm', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue10924'
versions = ['Python 3.3']

[jafo]

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.

[pitrou]

You forgot to add the new files to your patch.

[jafo]

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.

[jafo]

I've made a new .patch file using "diff -c" rather than "svn diff". This is the same code, but applies without manual intervention.

[nicdumz]

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?

[jafo]

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).

[pitrou]

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.

[jafo]

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.

[pitrou]

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 :)

[jafo]

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. :-)

[pitrou]

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.

[jafo]

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...

[pitrou]

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.

[jafo]

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.

[jafo]

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...

[pitrou]

Actually, the "pending" stage is only for when things have been committed :)
See http://docs.python.org/devguide/triaging.html#triaging

[jafo]

Thanks. I had just read that a day or so ago, reviewing it for Brett's work.

[jafo]

Committed in r88500.

[ezio-melotti]

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 PEP-8 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):").

[SilentGhost]

Here is the patch fixing pep-8 compatibility and test. It is against the latest commit.

[brettcannon]

I will look at the patch.

[brettcannon]

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).

[SilentGhost]

Above-mentioned fix was committed in 0586c699d467 and 62994662676a

[tiran]

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))

[python-dev]

New changeset 74a1110a3b50 by Christian Heimes in branch 'default':
bpo-10924: Fixed mksalt() to use a RNG that is suitable for cryptographic purpose
http://hg.python.org/cpython/rev/74a1110a3b50