672129 - Implement hmac + bcrypt password storage in Bugzilla (change PASSWORD_DIGEST_ALGORITHM)

Status: ASSIGNED

(Bugzilla :: User Accounts, enhancement, P3)

Description

[Reed Loden [:reed]]

As part of a larger push to better password storage across all Mozilla sites, we should convert Bugzilla to use bcrypt + hmac for password storage.

For more details, see:
https://wiki.mozilla.org/WebAppSec/Secure_Coding_Guidelines#Password_Storage
http://blog.mozilla.com/webappsec/2011/05/10/sha-512-w-per-user-salts-is-not-enough/
http://blog.mozilla.com/webappsec/2011/06/01/sha-512-follow-up-and-thank-you/

Comment 1

[Max Kanat-Alexander]

  Interesting. Note that a key part of the blog is "All the attacker needs to do now is figure out the password entered into the hash formula and the order in which it was used." He does still point out that given a dictionary, it's rather fast to crack salted hashes if the passwords match that dictionary, but then again, would that really even be significantly better with hmac + bcrypt? 24 hours to get a password vs. 4 seconds to get a password would certainly be different, but not ultimately all *that* different for many security situations. 

  I would only consider a solution really good if it meant that the time to crack the password would be longer than the password could reasonably possibly be in use--for example, 100 years.

  In any case, FWIW, Bugzilla currently allows pluggable password-crypting systems, you just have to change one constant in Bugzilla::Constants, if you want to do this for higher-security installations.

Comment 2

[Michael Coates [:mcoates] (acct no longer active)]

DKL

Should we open a second bug to get this into our BMO instance via the pluggable password-crypting system?  

Does that support a conversion system where passwords are rehashed into the new format after a successful login of an existing user? In other words, we would want to upgrade users hashing at their next login instead of erasing hashes and forcing a password reset.

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> Does that support a conversion system where passwords are rehashed into the
> new format after a successful login of an existing user?

  Yep. Bugzilla automatically does that for you.

Comment 4

[Michael Coates [:mcoates] (acct no longer active)]

Great. Filed a separate bug (672827) to make this transition for bugzilla.mozilla.org per the pluggable password-crypting systems mentioned in comment 1

Comment 5

[David Lawrence [:dkl]]

Moving back to this bug and changing product as this will not be going upstream most likely.

Comment 7

[Reed Loden [:reed]]

(In reply to comment #5)
> Moving back to this bug and changing product as this will not be going
> upstream most likely.

Why is that exactly? I have yet to see any viable reason why this shouldn't go upstream.

Comment 8

[Michael Coates [:mcoates] (acct no longer active)]

I filed this separate bug so the upstream communication could happen in that bug while we just implement away for BMO here.

Comment 9

[David Lawrence [:dkl]]

I am fine with backing up and keeping this as two separate bugs. I merely used my judgement from previous comments that due to the fact that current versions of Bugzilla allow for pluggable password-crypting systems, and that installations would need to make a simple change to Bugzilla::Constants, that upstream would not need to accept this as a built in crypting method. Therefore we would just keep this as a BMO specific change. But if I am premature and upstream would in fact include a working alternative crypting method in future versions of Bugzilla, then we can use this bug to track that part.

dkl

Comment 10

[Max Kanat-Alexander]

(In reply to comment #7)
> Why is that exactly? I have yet to see any viable reason why this shouldn't
> go upstream.

  Yeah, to be clear, I did not reject this bug with my comment. I just mentioned that I'd like to see some data that proves that this is a truly significant security improvement for our userbase that's worth the dependencies, effort, maintenance, and complexity.

Comment 11

[Reed Loden [:reed]]

Attachment: patch - v1

This patch was originally developed for bug 672827 by dkl and modified a bunch by me. It has been successfully tested in a variety of methods, but I do welcome more testing.

BMO will need some slight modifications to this to support hardened hashes, but I don't think Bugzilla proper needs to support those, as it's pretty hackish.

Comment 12

[Reed Loden [:reed]]

Attachment: patch - v1.1

Oops, I forgot to ./runtests.pl... Tests completely pass with this version.

Comment 13

[Frédéric Buclin]

I said yesterday it was time to stabilize the code for 4.4, so this is a bit too invasive for 4.4.

Comment 14

[Frédéric Buclin]

Comment on attachment 655400 [details] [diff] [review]
patch - v1.1

At first glance, the code looks pretty complex. I'm not a fan of this. Also, what's the point to have several keys? Will a new one be created everytime you run checksetup.pl?? That's definitely something I would object.

Also, I see you write |use DateTime| inside a subroutine; this doesn't make sense. localtime() is much cheaper. And we already talked about hmac_sha512_base64 which requires 64bit operations. I see no reason to use it instead of e.g. hmac_sha256_base64, which we already use in Token.pm. I prefer consistency than using different methods here and there.

Comment 15

[Reed Loden [:reed]]

(In reply to Frédéric Buclin from comment #14)
> At first glance, the code looks pretty complex. I'm not a fan of this.

I don't see how it's that complex... If we supported actual split-out password hash modules, I agree it would be nicer, but with what Bugzilla supports, it's really not super-crazy. The code is pretty fairly contained to just a few places.

> Also, what's the point to have several keys? Will a new one be created everytime
> you run checksetup.pl?? That's definitely something I would object.

Nope. It's meant to be regularly changed (at some period of time... probably at least once a year, maybe more) or when you feel that something has been compromised. Maybe it would be nice to eventually add a option to ./checksetup.pl to add a new key to make it easier for admins, but it's fine for now (I'll follow-up with that in another bug, as I think there should be a ./checksetup.pl option to regenerate the site_wide_secret easily without removing it, too).

To give a better understanding about how Bcrypt and HMAC work together, check out this blog post by one of Mozilla's web developers:
http://fredericiana.com/2012/06/08/lets-talk-about-password-storage/

> Also, I see you write |use DateTime| inside a subroutine; this doesn't make
> sense. localtime() is much cheaper.

It's a one-time operation on initial parameter adding. "cheaper" is not really a factor here. However, I'm happy to change to use localtime() if it lets me get the same thing out of it. Seems I can just use Time::localtime and localtime->year + 1900, localtime->mon + 1, and localtime->mday, so that seems easy enough.

> And we already talked about
> hmac_sha512_base64 which requires 64bit operations. I see no reason to use
> it instead of e.g. hmac_sha256_base64, which we already use in Token.pm. I
> prefer consistency than using different methods here and there.

I don't really think the slowness of SHA-512 on a 32-bit system is really much of a factor, especially when dealing with passwords, as you *want* password hashing to be slow (that's kinda the whole point of bcrypt). However, there's not that big enough of a difference between the two hash functions to warrant arguing over it, so I'll change it. We may change it for BMO, though, depending on what the Security Assurance team wants.

Comment 16

[Reed Loden [:reed]]

(In reply to Frédéric Buclin from comment #13)
> I said yesterday it was time to stabilize the code for 4.4, so this is a bit
> too invasive for 4.4.

It's not worth waiting another year for this, especially with all the password database dumps lately. With today's password crackers, just using a SHA-2-based digest + salt is not good enough. The time is now, especially since this has already waited a year. Moore's Law is at play here, and time is not on our side. Mozilla's Security Assurance team has been requesting this for over a year, and I'm sure both dkl and glob are happy to do whatever QA necessary to ensure this doesn't break things.

http://codahale.com/how-to-safely-store-a-password/
http://yorickpeterse.com/articles/use-bcrypt-fool
http://phpmaster.com/why-you-should-use-bcrypt-to-hash-stored-passwords/

Also, to be fair, there are plenty of people that recommend doing even more (such as PBKDF2 or scrypt -- http://www.unlimitednovelty.com/2012/03/dont-use-bcrypt.html), but since we're not using bcrypt alone (this is bcrypt + HMAC), it's more than enough for the immediate future, and it's consistent with the rest of Mozilla's websites.

Comment 17

[Reed Loden [:reed]]

Attachment: patch - v2

* Swaps from DateTime to Time::localtime
* Uses HMAC-SHA-256 instead of HMAC-SHA-512

All tests pass.

Comment 18

[Reed Loden [:reed]]

Comment on attachment 655438 [details] [diff] [review]
patch - v2

Review of attachment 655438 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Constants.pm
@@ +567,5 @@
> +# Perl's "Digest" module. One exception is BCRYPT_HMAC which is implemented
> +# differently since it uses a combination of HMAC-SHA-256 and Bcrypt.
> +# Note that if you change this, it won't take effect until a user changes
> +# his password.
> +use constant PASSWORD_DIGEST_ALGORITHM => 'BCRYPT_HMAC';

So, I see one issue I missed. For this constant, I really should be doing something like:

use constant PASSWORD_DIGEST_ALGORITHM => Bugzilla->feature('bcrypt_hmac') ? 'BCRYPT_HMAC' : 'SHA-256';

However, I can't access Bugzilla->feature() from Bugzilla::Constants. Thoughts on how best to handle this? I could made a specific exception in bz_crypt() that will use SHA-256 if PASSWORD_DIGEST_ALGORITHM eq 'BCRYPT_HMAC', yet it is not available or something, but that seems hackish.

Comment 19

[Reed Loden [:reed]]

Oh, you can test this code at https://landfill.bugzilla.org/bz672827/.

Comment 20

[Frédéric Buclin]

(In reply to Reed Loden [:reed] from comment #17)
> * Swaps from DateTime to Time::localtime

localtime() is a built-in function, no need to load any external module. In array context, it returns all the data you need.

Comment 21

[Frédéric Buclin]

(In reply to Reed Loden [:reed] from comment #16)
> It's not worth waiting another year for this

You had a whole development cycle to upload a patch and have it reviewed carefully. You jump in a few days *after* I announced that we were going to branch and stabilize the branch. You know the rules well enough to not make me change my mind on this. It's targetted 5.0.

Comment 22

[Dave Miller [:justdave]]

4.4 isn't RC yet, and the weakness of SHA-256 is well-documented at this point.  If we can still get this into 4.4 before it goes RC I don't see a reason not to commit this on the branch.

Comment 23

[Reed Loden [:reed]]

Attachment: patch - v2.1

Thanks to glob, changed PASSWORD_DIGEST_ALGORITHM to a sub, which takes care of my Bugzilla->feature() problem.

Update localtime code, as per LpSolit's comment.

Comment 24

[Dave Miller [:justdave]]

FWIW, I intentionally hadn't retargeted yet (oh, to be able to lock target milestone down to approvers ;) ), intending to state my opinion and spur more discussion on the topic in here before doing so.  There's apparently some compelling reasons not to take this on the 4.4 branch, so I'm trying to figure out what all the effects of this are still.

Comment 25

[Frédéric Buclin]

Comment on attachment 655501 [details] [diff] [review]
patch - v2.1

>=== modified file 'Bugzilla/Auth/Verify/DB.pm'

>+    my $current_algorithm = PASSWORD_DIGEST_ALGORITHM;
>+    my ($algorithm) = $real_password_crypted =~ /{([^}]+)}$/;
>+    my $update_password = 0;
>+
>     # If their old password was using crypt() or some different hash
>     # than we're using now, convert the stored password to using
>     # whatever hashing system we're using now.
>-    my $current_algorithm = PASSWORD_DIGEST_ALGORITHM;
>-    if ($real_password_crypted !~ /{\Q$current_algorithm\E}$/) {
>+    if ($algorithm ne $current_algorithm) {
>+        $update_password = 1;
>+    }

These changes are already in bug 785283. So your patch will conflict. (first part of my r-)


>+            if ($hmac_key_id != get_latest_hmac_key_id()) {

I don't see the point to have several hmac keys. There should be only one. If the older ones are compromised, then they should be invalidated (replaced by a new one). If they are fine, then I don't see the point to generate a new key. (2nd part of my r-)

Comment 26

[Reed Loden [:reed]]

(In reply to Frédéric Buclin from comment #25)
> These changes are already in bug 785283. So your patch will conflict. (first
> part of my r-)

I wrote this patch long before the one for bug 785283. Once bug 785283 lands, I can update this patch to support the code it added/changed.

> >+            if ($hmac_key_id != get_latest_hmac_key_id()) {
> 
> I don't see the point to have several hmac keys. There should be only one.
> If the older ones are compromised, then they should be invalidated (replaced
> by a new one). If they are fine, then I don't see the point to generate a
> new key. (2nd part of my r-)

Only one HMAC key is used at a time, but old ones still have to be kept forever in the config. Otherwise, people's password hashes that were created using older HMAC keys (and who haven't logged back in since such change) wouldn't work, and they would have to manually reset their password. As users authenticate to Bugzilla, if they aren't using the latest HMAC key, their hash will be regenerated.

Comment 27

[Frédéric Buclin]

(In reply to Reed Loden [:reed] from comment #26)
> Only one HMAC key is used at a time, but old ones still have to be kept
> forever in the config. Otherwise, people's password hashes that were created
> using older HMAC keys (and who haven't logged back in since such change)
> wouldn't work, and they would have to manually reset their password.

I think that's what we want. There is no reason to create another HMAC key unless it has been compromised, in which case we don't want old password hashes to still work.

Comment 28

[Reed Loden [:reed]]

(In reply to Frédéric Buclin from comment #27)
> (In reply to Reed Loden [:reed] from comment #26)
> > Only one HMAC key is used at a time, but old ones still have to be kept
> > forever in the config. Otherwise, people's password hashes that were created
> > using older HMAC keys (and who haven't logged back in since such change)
> > wouldn't work, and they would have to manually reset their password.
> 
> I think that's what we want. There is no reason to create another HMAC key
> unless it has been compromised, in which case we don't want old password
> hashes to still work.

No, it's not what we want. For one, you don't *only* change HMAC keys for compromise. You also change it when a sysadmin moves on (as in to another job) but you still trust him/her, so you wouldn't inconvenience every user you have just to make any possible database dump invalid. Also, changing it regularly is good security posture as well, as sometimes you just don't know if you've been compromised. This ability to have multiple keys with only one in use at a time and have users who log in with older keys get upgraded to the newest key is a core part to how this algorithm is used, so it's not going to change.

Comment 29

[Dave Miller [:justdave]]

It's also how we've done hash changes in the past (changing from unix crypt to md5, for example).  After some period of time, the crypt password stops working and everyone has to reset their password to get in.  If that's a suitable time period then that'd be a good thing anyway.  I'd even suggest letting the old keys age out after a configurable amount of time instead of keeping them forever (default to a year or 2 years or something)

Comment 30

[Reed Loden [:reed]]

(In reply to Dave Miller [:justdave] from comment #29)
> It's also how we've done hash changes in the past (changing from unix crypt
> to md5, for example).  After some period of time, the crypt password stops
> working and everyone has to reset their password to get in.  If that's a
> suitable time period then that'd be a good thing anyway.  I'd even suggest
> letting the old keys age out after a configurable amount of time instead of
> keeping them forever (default to a year or 2 years or something)

That's up to the Bugzilla administrator as to if he/she wants to edit localconfig and remove older keys after some point in time, but I don't think it's something we should add directly in to auto-remove keys.

Comment 31

[Reed Loden [:reed]]

Attachment: patch - v2.2 (wip)

still wip... not ready for review

Comment 32

[Frédéric Buclin]

Comment on attachment 697995 [details] [diff] [review]
patch - v2.2 (wip)

>=== modified file 'Bugzilla/Util.pm'

>+        if (Bugzilla->feature('bcrypt_hmac') && $algorithm eq 'BCRYPT_HMAC') {

It would be cleaner to have all the code in this IF block in a separate private subroutine, instead of this large IF block.

Comment 34

[Gervase Markham [:gerv]]

Would a patch using Digest::Bcrypt <https://github.com/LoonyPandora/Digest-Bcrypt> be any simpler? It seems to use the Digest interface which we already support, with the addition of the restriction on salt length and the need to provide a cost.

Gerv

Comment 35

[Frédéric Buclin]

reed: any progress? You already missed 4.4 (comment 13). Do not wait for 5.0 to enter its RC stage to submit a new patch.