Use more secure hashes for passwords

[flaix]

Gitblit currently uses outdated hash functions for passwords stored in the user file. It should be updated to use modern hashes considered secure.

[jblaufuss]

Yes! I just came here to report this same issue. Looking at users.conf, passwords are hashed using MD5, which is easily cracked. A modern system should use something like scrypt, which is specifically designed for hashing passwords and making dictionary-based cracking difficult.

[martinspielmann]

I opened a pull request to address this issue. It's adding PBKDF2WithHmacSHA256 hash algorithm while being fully backward compatible.

A general topic which should be discussed in my opinion is the following:
When the realm.passwordStorage property is updated, the stored password hashes will only be updated if the user changes his password which might take a very long time.
Would it be better to check if the property was changed on every login?

Best regards,
Martin

[jblaufuss]

I have never actually built and authentication system like this myself, but have have heard it discussed. IIRC, the "right way" to do a hash algorithm migration is this:

• The password validation function has two cases:

1. Hash the password with the new algorithm and verify the output against the stored hash.

2. If the above fails, then hash the password with the old algorithm, then hash the output with the new algorithm, then verify the chained output against the stored hash.

• On upgrade, automatically hash all the hashes in users.conf with the new algorithm. They'll be able to be verified by case 2.

• If a user changes his password, it's only hashed with the new algo, and is verified by case 1.

The benefit of this is that you don't have hashes created with a weak algorithm hanging around (potentially indefinitely) on your system, waiting to be cracked.

I suppose it could be simpler to only have the case 2 code, because it would provide the extra security and no one can make any guarantees that everyone will eventually update their passwords so it can eventually be eliminated.

Also, IIRC, simple MD5 hashes can be cracked so effectively at this point that they're really just good for obfuscation, not security. I don't think it even makes sense to provide the option to use simple MD5 hashes at all.

[martinspielmann]

Ah, I wasn't aware of this migration process, but it sounds reasonable 👍. I'll update the PR asap.
I share the opinion that MD5 for password hashing. I'm a little afraid of dropping support for MD5 though. Who knows if someone used it for some integration or something... and there's also the option to use plaintext... don't know what's a good solution here :/

[flaix]

The password hash to be used is configurable. That means that it could have changed over time and that multiple different schemes are in use in the users.conf file. There is no way to know what the "old" algorithm was, I think. I believe the suggested process works for applications with a fixed hashing algorithm that was changed, but not in this case.

[martinspielmann]

@fzs you are right. I tried to implement the above suggested migration process and kept the information about previous algorithms in an additional prefix. But it turned out to get really complex and not very maintainable :/

[flaix]

I don't think it is much of a problem and needs to be solved automatically. A Gitblit administrator can decide if he is fine with leaving old hashed values as they are or to what degree she wants to bug her users to update their passwords so that they are hashed with a new algorithm.
Another possibility would be to upgrade the hash in user.conf when a user logs in, not only when he changes his password.

[martinspielmann]

OK, Yes that was my first idea too. I,ll add that tothe PR asap.

EDIT: added automatic hash update when a user logs in.

[martinspielmann]

Is there any question or something I can do to help? Would be great to have a new release soon :)

[flaix]

This has been implemented by the PR and subsequent commit building upon it.

[darmbrust]

So, I'm not sure how this change ended up being implemented. But the end result seems to be, that it corrupted all of my credentials in one of my docker containers, and I can no longer log in.

Looking at the conf file, some of my user accounts were rewritten as PBKDF2 hashes, while others were not, and the ones that were upgraded, well, they are now unusable.

I also can't find any doc updates that tell us how to properly encode a password into the conf file for bulk creating users, such as in a docker deployment.

http://gitblit.com/administration.html

Previously, I could simply do echo -n yoursuperpassword | md5sum
and I could put the results in the conf file when building the image.

[flaix]

The implementation sets PBKDF as the new default method. So if no other method is/was explicitly specified, this is used.
When a user logs in successfully, the stored password hash is overwritten with a PBKDF version, if it was a weaker method before.

If some user accounts now have PBKDF hashes and others don't, then this is because these users have logged in while others haven't yet.

So far everything works as expected. But in no way should this make accounts unusable and lock you out. All password methods still work, i.e. existing hashed/plaintext passwords stored in users.conf can be read and used for login. So lets concentrate on what keeps you from logging in and what makes you say that accounts became unusable.

If you had MD5 hashed passwords before, the update apparently uses the PBKDF method and rehashed the password when you logged in. With the next login, this method is used. It is less likely that the PBKDF method doesn't work, so I suspect something else to be amiss. Maybe you could give a bit more detail and context about your update, so that we may find the problem and solve it.
What will not work, of course, is when you use the same password file one two instances where only one is the latest version that supports PBKDF and the other doesn't.

As for provisioning accounts, that has never had special support by Gitblit. Nevertheless, you could still use the way you described above, since Gitblit would be able to use the MD5 hash that you provisioned for the first login, and then change it to a safe password hash. Or you choose to simply not store your passwords securely and set a different method in the configuration.

[darmbrust]

I can try to reproduce it again locally, I'm not sure why/how it got into a state where my passwords were no longer valid. The passwords were pre-encrypted as MD5, and I didn't specify the password type, so on the upgrade, it changed over to the new PBKDF. It still worked correctly, and let me log in after the upgrade, and apparently, changed the passwords to PBKDF at this time. And some point later, I redeployed the docker image again (but the passwords are on a volume that remains) and I could no longer log in. All user accounts with a PBKDF password now refused the (correct) password. Seems to be some sequence where the upgrade doesn't hash the right text when it upgrades the passwords.

[darmbrust]

I should probably move this to a new bug, but can confirm. Gitblit 1.9.0 - start it up with existing MD5 passwords. Leave the config on the new default for PBKDF.
1st Login: Successful.
Logout.
Can never log in again.

I tried another pattern - 1st login - successful.
Changed my password (to the same password)
Logged out.
Can successfully log back in.

There is something broken with the password upgrade process upon login.
As an aside, if the existing passwords are bad / should they really be upgraded to the new encryption unless the user actually changes the password?

[darmbrust]

Oh, and something I should put in a feature request, it would be nice if a main was added to the SecurePasswordHashUtils that let us hash a password correctly for use cases like mine, where I want to pre-configure a docker image via script. I know, I know, submit a pull request :)

[darmbrust]

The bug and the root cause is here:
#1335

[flaix]

As an aside, if the existing passwords are bad / should they really be upgraded to the new encryption unless the user actually changes the password?

Gitblit knows nothing about whether the existing passwords are good or bad. It has no code to judge password quality.
Gitblit only knows whether the stored password format is a secure hash or an insecure one. So it can only decide to at least store the password in a secure way, be the password itself of good or bad quality.

[darmbrust]

I just meant, that if the existing passwords are assumed to be compromised / compromiseable, then from a strict security standpoint, simply rehashing them securely doesn't actually improve the security, until the password is changed. I don't think it really matters that much for an application like gitblit anyway, was just a thought.

[flaix]

The difference is between compromised and compromiseable. If they had to be considered compromised, then all passwords need to be reset and changed. But that is a different scenario, not what was addressed in this change.

Here, an insecure hash makes stolen passwords easier to brute force. One could say, they are easier compromiseable, in a way. Rehashing them with a more secure hash does improve security in this case. This is more protection for the future case that a password db is stolen.
Consider the case that in five years the PBKDF parameters are considered not secure enough anymore or some other algorithm is to be used. An update (hopefully earlier then, than this time around) would then work like now, where the password is stored with improved security once a user logs in. He doesn't have to change the password for that. Passwords would not be automatically be considered compromised.

If all this matters for Gitblit is debatable. But then again, I have no idea where and how Gitblit is used. So I think it should provide the best security it can for the scenarios it could be used in.