Add advice on best practices for hashing secrets

[tiran]

BPO	17006
Nosy	@rhettinger, @pitrou, @vstinner, @giampaolo, @tiran, @ezio-melotti, @alex, @bitdancer, @jab, @asvetlov, @hynek
Files	hashlib.patch: Need more explanation in 'Security Considerations' section. hashlib.patch

^{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: ```python assignee = None closed_at = None created_at = labels = ['type-feature', 'docs'] title = 'Add advice on best practices for hashing secrets' updated_at = user = 'https://github.com/tiran' ``` bugs.python.org fields: ```python activity = actor = 'Ramchandra Apte' assignee = 'docs@python' closed = False closed_date = None closer = None components = ['Documentation'] creation = creator = 'christian.heimes' dependencies = [] files = ['34357', '34406'] hgrepos = [] issue_num = 17006 keywords = ['patch'] message_count = 19.0 messages = ['180330', '180331', '180341', '200764', '203161', '203207', '203225', '213171', '213478', '213480', '213533', '213541', '213545', '213548', '213558', '213563', '213575', '257267', '257468'] nosy_count = 14.0 nosy_names = ['rhettinger', 'pitrou', 'vstinner', 'giampaolo.rodola', 'christian.heimes', 'ezio.melotti', 'alex', 'r.david.murray', 'jab', 'asvetlov', 'docs@python', 'Ramchandra Apte', 'hynek', 'yating.huang'] pr_nums = [] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue17006' versions = ['Python 2.7', 'Python 3.4', 'Python 3.5'] ```

[tiran]

Lot's of people still think that something like sha512(secret + message), sha1(password + salt) or even sha1(password) is secure. Except it isn't. Most crypto hash functions like md5, sha1, sha2 family (sha256, sha384, sha512) use a Merkle–Damgård construction [1]. The construction is vulnerable to several attack vectors like length extension attacks. Passwords needs special care, too.

I propose we add a warning to the documentation of hashlib. It's not the right place to teach cryptographics but it's a good place to raise attention. The warning should explain that you shouldn't solely hash secrets or messages containing a secret. For messages a MAC algorithm like HMAC should be used. For passwords a key stretching and key derivation function like PBKDF2, bcrypt or scrypt is much more secure.

[1] http://en.wikipedia.org/wiki/Merkle%E2%80%93Damg%C3%A5rd_construction

[hynek]

I think since we ship cryptographic functions, we should take responsibility and warn against the most common mistakes people do.

[918f67d7-4fec-4a8d-93e3-6530aeb1e57e]

+1 "Better to be safe than sorry"

[tiran]

Who likes to step in? Alex?

[tiran]

ping :)

[ezio-melotti]

This might be useful to whoever attempts to write a patch: http://docs.python.org/devguide/documenting.html#security-considerations-and-other-concerns

[vstinner]

For passwords a key stretching and key derivation function like PBKDF2, bcrypt or scrypt is much more secure.

It looks like Python 3.4 now provides something for pbkdf2, so it may be interested to mention it on the top of the hashlib in your warning.

http://docs.python.org/dev/library/hashlib.html#hashlib.pbkdf2_hmac

[bitdancer]

Priya: I see that there is already a 'warning' box at the top of the hashib page. If Christian concurs, I suggest we add your note to that warning, with the link to the security considerations section as you have it, and removing the reference to the 'see also' section. Then we move the security related 'see also' link into the new security section, with a sentence that says that it explains the weaknesses.

A note about the patch: you should wrap all lines to less than 80 characters.

[ab2a1301-2bc3-4d96-adea-26269fb263c3]

Hi. this is my first patch. I tried to follow the instruction by David to add Christian's notes into a new security section.

[bitdancer]

Please note what I said about wrapping lines to less than 80 characters.

Also, my thought was to move the 'see also' entry that is referenced in the existing warning text (the wikipedia link) into the 'security considerations' section, probably as a separate paragraph that consists of the current text that follows that reference, with 'wikipedia article' turned into a link to the actual article. (See http://docutils.sourceforge.net/docs/user/rst/quickref.html#hyperlink-targets for information on how to make links to external urls in .rst files).

[rhettinger]

If something gets added, please follow the dev-guide and word it affirmatively (here the recommended practices) instead of continuing to fill the docs with warnings and danger signs.

[bitdancer]

Good point. There is an existing warning for hash weaknesses...the whole thing could be rephrased as "Please see the security considerations section for important information on the considerations involved in using the various hashing algorithms, and notes on best practices for message and password hashing."

The whole thing could then be turned into a note...except that there is a reason that it is a warning, because some of the hashes have known weaknesses yet still must be used for certain things. So probably it should stay a warning in this particular case.

[rhettinger]

So probably it should stay a warning in this particular case.

Please don't. Python's docs have become cluttered with warning and danger signs. This stands in marked contrast with the docs for other languages which are much cleaner. Our docs have also started to become "preachy", telling people how we think they should write programs rather than documenting what the language does.

[bitdancer]

Raymond: I'm not talking about *adding* a warning.

Is it your opinion that the existing warning should be removed?

[tiran]

Raymond makes a good point. We mustn't clutter the docs with warnings. People are going to skip warning boxes if they occur too often. The documentation of the hashlib module contains three "note" boxes and one "warning box". That's far too many.

The first "note" box could be moved to "see also". The other two "note" could be removed and their content added to the documentation of update(). The warning box should follow the example of the ssl module and all further security considerations should be moved into a new section.

The Python stdlib documentation is the wrong place to teach users about crypto and security stuff. But in my opinion good documentation should point out that something is dangerous or may lure a user into false sense of security.

Perhaps I should start a howto with common security-related issues in Python software for 3.5.

[akuchling]

+1 to reducing the number of notes, and to a security HOWTO. (Christian: if you need writing help, please let me know; I'd be happy to help.)

[pitrou]

Note boxes have nothing to do with warnings, we should discuss them separately if needed. (I see nothing wrong with multiple notes, given that a note is generally something ancillary and optional)

[ezio-melotti]

People are going to skip warning boxes if they occur too often.

I'm not sure I agree. This would be true if they were abused for trivial things ("Warnings: using .pop() on a empty list will return an IndexError!"), but I don't think they are.

I think warnings are ignored only by people that are already familiar with the module and its limitation/issues, and that know what they are doing. If the warning is not evident, people are going to miss it [0].

If warnings are used correctly, people will spot them easily and read them (or ignore them if they already know what they are warning against).

[0]: I know I missed it in e.g. https://api.jquery.com/die/ -- the function is deprecated, but (currently) this is only written in the top right corner and in small in the category at the top -- two places that are easily overlooked. https://api.jquery.com/toggle-event/ on the other hand has a clearly visible yellow box at the top that immediately says that the method is deprecated.

[918f67d7-4fec-4a8d-93e3-6530aeb1e57e]

It is good to add warnings; if they are ignored it is little worse than the status quo.

On 1 January 2016 at 08:54, Ezio Melotti \report@bugs.python.org\ wrote:

Ezio Melotti added the comment:

> People are going to skip warning boxes if they occur too often.

I'm not sure I agree. This would be true if they were abused for trivial things ("Warnings: using .pop() on a empty list will return an IndexError!"), but I don't think they are.

I think warnings are ignored only by people that are already familiar with the module and its limitation/issues, and that know what they are doing. If the warning is not evident, people are going to miss it [0].

If warnings are used correctly, people will spot them easily and read them (or ignore them if they already know what they are warning against).

[0]: I know I missed it in e.g. https://api.jquery.com/die/ -- the function is deprecated, but (currently) this is only written in the top right corner and in small in the category at the top -- two places that are easily overlooked. https://api.jquery.com/toggle-event/ on the other hand has a clearly visible yellow box at the top that immediately says that the method is deprecated.

----------

---

Python tracker \report@bugs.python.org\ \http://bugs.python.org/issue17006\

---