wiki2 authentication bug fix and improvement against timing attack

[stevepiercy]

ping @mmerickel @bertjwregeer for review and comment (thanks to @ztane @Themanwithoutaplan and @wichert for IRC help)

• Bytes type does not have encode method. The expected_hash retrieved from the database is a bytes object.

• Use hmac.compare_digest() instead of == to avoid timing attacks as a recommended security best practice. See https://www.python.org/dev/peps/pep-0466/ https://bugs.python.org/issue21306 and https://codahale.com/a-lesson-in-timing-attacks/ for details.

  Note, however, hmac.compare_digest() was not backported to py2.6. For a tutorial, I am OK with stating this will not work on Python 2.6 with a clear warning note at the start of the tutorial and on the authentication step.

[stevepiercy]

Oh, and if approved, I will do the necessary work on the docs:

• add references to avoiding timing attack best practices in authentication
• carry forward changes in user.py to later tutorial steps
• possible test updates? I haven't worked through that far in the tutorial yet.

[Themanwithoutaplan]

FWIW Python 2.6 probably shouldn't be in use on any servers anyway because it's not getting security updates.

[mmerickel]

@bertjwregeer can back me up on this but there is no evidence that timing attacks are possible against password hashing algorithms (at least bcrypt) because the string comparison is on the resulting hash itself. Any change to the password results in a completely new hash which would take longer or shorter to compare thus it's infeasible to use timing as an attacker to progressively guess more and more characters of the string. [1]

This also breaks py26 and some versions of py27 (not gonna happen). If @bertjwregeer believes we should use a constant-time compare here anyway we need to come up with a solution that will work on all our supported versions of python (we aren't dropping support for 2.7.0 - 2.7.7). One option would be to expose a public api in pyramid for constant time compare (I wouldn't expose strings_differ directly because I hate that api :-).

As far as the bytes vs unicode issue - I was previously testing on py27 so didn't notice this. Just spun up a virtualenv on py35 and reproduced the issue. If you want to submit that fix in a separate PR I will merge it.

[1] http://security.stackexchange.com/questions/46212/does-bcrypt-compare-the-hashes-in-length-constant-time

[stevepiercy]

@mmerickel @bertjwregeer I could be wrong, but my understanding is that bcrypt provides a constant-length, but when comparing the two hashes, we still need to use hmac or XOR to avoid the timing attack. See the last two sections "Why does the hashing code on this page compare the hashes in 'length-constant' time?" and "How does the SlowEquals code work?"
https://crackstation.net/hashing-security.htm

While we're discussing the topic, how come we don't use a salt? I could add that, too, and make the necessary updates to the db init script, etc.

Finally, advocating best security practices is not consistent with supporting versions of Python that don't receive security updates. I'm not sure what we should do in this regard, but at least mentioning implications would be the responsible thing to do.

[mmerickel]

I could be wrong, but my understanding is that bcrypt provides a constant-length, but when comparing the two hashes, we still need to use hmac or XOR to avoid the timing attack.

There's a lot of crud floating around about storing passwords given "best practices" are basically re-learned by every developer ever who chooses to blog about it. I'm not saying your link is wrong, and there are benefits to constant-time comparisons in many situations however bcrypt makes these attacks practically quite unfeasible. See the comments in pyca/bcrypt#8 and pyca/bcrypt#60 for examples. If the bcrypt folks in PyCA say it's unfeasible I'm liable to believe them. If they decide to add a comparison function to the library as one of those issues discusses, then we should update our example to use it.

While we're discussing the topic, how come we don't use a salt?

We are using a salt. Note the bcrypt.gensalt() used in the set_password method. You can use your own application-level salts and such but they provided only limited benefit over per-password salts.

Finally, advocating best security practices is not consistent with supporting versions of Python that don't receive security updates.

We've already started the discussion to deprecate Python 2.6 in #2368, however we cannot stop supporting any versions of Python 2.7 regardless of their state as they are actively being bundled by OSs and are not aggressively updated to patch releases.

[stevepiercy]

If the bcrypt folks in PyCA say it's unfeasible I'm liable to believe them.

OK, I can go along with that discussion.

We are using a salt. Note the bcrypt.gensalt() used in the set_password method. You can use your own application-level salts and such but they provided only limited benefit over per-password salts.

OK, I was looking at the db schema only, not the code, and wondered why there weren't per-password salts. I've done that in my apps outside of Python.

Given the foregoing, my final point is moot and I'll pull it from the table.

I'll revert the hmac stuff, and roll the source forward in later steps. Thanks!

[mmerickel]

OK, I was looking at the db schema only, not the code, and wondered why there weren't per-password salts.

The bcrypt result we store as password_hash is actually an ascii string containing several of the input parameters along with the resulting hash. That's why bcrypt.hashpw(pw.encode('utf8'), expected_hash) works. It is borrowing the input parameters (salt and work factor) stored in expected_hash and using them to hash the new pw.

[digitalresistor]

You do not need to have a constant time compare for password hashes.

[digitalresistor]

Oh $DEITY, that site https://crackstation.net/hashing-security.htm is terrible.

[digitalresistor]

@stevepiercy

Constant time compare is needed when an attacker can manipulate the hash being compared. For example, signed cookies. The cookie is actually <hash (aka signature)>. It's secure so long as the attacker can't modify and generate a valid . However is only X amount of bytes long. The attacker can modify the byte by byte, and if it isn't using a constant time compare, use timing information to derive a valid for their chosen .

In the case of passwords, the hash is stored server side, and the client sends up plaintext information. The attacker can't modify any hashes. Server side we then take the salt, and bcrypt and out comes a hash. We can then compare that hash against the one we had in our database and see if they match. Let's say they don't, all an attacker gleans is that the password is invalid, as soon as they change a byte in the plaintext input, the avalanche property of a good hash function will cause the output hash to change so significantly that an attacker won't glean anything and has to bruteforce the full password space.

Avalanche in action:
Password1 == 2ac9cb7dc02b3c0083eb70898e549b63
password1 == 7c6a180b36896a0a8c02787eeafb0e4c

[stevepiercy]

@bertjwregeer thanks, that makes sense.

@mmerickel I made the updates, and merged the PR. I'll resume work on feature/alchemy-scaffold-update branch, including the last two pages of docs, the feature to redirect when adding a page that already exists feature, and circle back to spruce up the affected docs.

[Themanwithoutaplan]

@bertjwregeer am I correct in thinking that bcrypt.hashpw(…) works in constant time? Seems to me this would be the relevant call for a timing attack. Comparing two strings of the same length should have minimal variance as the stackexchange article indicates.

[digitalresistor]

I have no idea if that is constant time or not, and even if it isn't constant time, nothing is lost.

There is no timing attack in comparison of password hashes.

[ztane]

@stevepiercy @Themanwithoutaplan notice also that the salt is not revealed to the attacker, so the attacker cannot even know the hashes of their chosen passwords. And these password encryptions are so damn slow that it is almost guaranteed that a context switch either in kernel or in python occurs for them.