search

sabre-io / Baikal

Baïkal is a Calendar+Contacts server
https://sabre.io/baikal/
GNU General Public License v3.0
2.43k stars 281 forks source link

Use proper password hashing algorithm #514

Open the-infinity opened 8 years ago

the-infinity commented 8 years ago

Would be great to have a better password crypting method. Is there a description or something which provides more information about the authentication mechanism? Patching A1 is no problem, but I would like to understand A2, too (without digging TOO deep in code), to provide a patch for this. Thanks! :)

untitaker commented 8 years ago

None of {md5,sha256,sha512} are remotely secure options!

https://paragonie.com/blog/2016/02/how-safely-store-password-in-2016

evert commented 8 years ago

The main issue here is that if we hash the password in a different way, we can no longer use Digest authentication. Basic is an option, but it's even worse for those that are not on HTTPS yet (for whatever reason).

If you want to know more about the current hashing approach, look no further than rfc7616.

evert commented 8 years ago

It actually has a chapter named A2

the-infinity commented 8 years ago

Yep, after digging a little deeper into the topic and reading the digest RFCs I decided to develop an alternative auth method which does not provide digest auth, but HMAC SHA512 encrypted passwords in database. Of course this will just make sense when you use TLS encryption. You will get a pull request in a few days. :)

evert commented 8 years ago

I think if we would do anything new, it would be bcrypt based. Seems to be the best choice these days ;)

If you're interested in using that as well, I think the best place for a PR would be in the sabre/dav project. Then eventually it can make its way back into Baikal.

staabm commented 8 years ago

You should just use password_hash

evert commented 8 years ago

http://security.stackexchange.com/questions/16809/is-a-hmac-ed-password-is-more-secure-than-a-bcrypt-ed-or-scrypt-ed-password

untitaker commented 8 years ago

I don't see the purpose of Digest authentication given that HTTPS is supposed to be used everywhere anyway?

While I don't know anything about PHP, password_hash seems the way to go.

On 23 March 2016 18:27:34 CET, Evert Pot notifications@github.com wrote:

The main issue here is that if we hash the password in a different way, we can no longer use Digest authentication. Basic is an option, but it's even worse for those that are not on HTTPS yet (for whatever reason).

If you want to know more about the current hashing approach, look no further than rfc7616.

You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/fruux/Baikal/issues/514#issuecomment-200452931

Sent from my Android device with K-9 Mail. Please excuse my brevity.

evert commented 8 years ago

Well, HTTPS is not really everywhere yet. It would be nice to provide some protection for the people that don't have it. Although I can definitely see a future where we ditch Digest altogether. It also makes the server slow!

citrin commented 7 years ago

If digest still need, just add configuration option: secure password hash (bcrypt or scrypt) without digest support or hash for digest support. But I doubt that DAV without https is popular in 2016. Currently security of all installations is compromised because of small number of digest users.

evert commented 7 years ago

I agree, lets make it official:

  1. Ditch Digest completely. It has other issues anyway.
  2. Make Basic the default
  3. Only use bcrypt for password hashes. No point in using SHA256/512.
untitaker commented 7 years ago

Sensible plan although I would specifically use password_hash, which is not married to bcrypt and can automatically upgrade to stronger algorithms as they become available.

muelli commented 7 years ago

A small thought: If you let the Web server do the authentication, then you can use all of, say, Apache's authentication methods, including TLS-SRP or client certificates.

evert commented 7 years ago

@muelli that's not a great solution for the average baikal user. It means that they need to understand apache configuration even more than they do already. It might be nice to have that as an option though. Definitely open a feature request if you want it.

muelli commented 7 years ago

Yes. Deployment is less straight forward, because you cannot work with each and every Web server and their configuration. Especially because deploying PHP applications in a rather generic way seems to be an unsolved problem as of 2017. The benefit is, though, that the relatively complicated matter of authenticating users is someone else's responsibility. Not only has the "other party" probably more expertise in doing authentication (correctly), but you'd also get rid of code that does weird things like using MD5.

FWIW: Apache typically allows configuration per directory. Baikal is deployed in a directory, so it could ship it's Apache configuration.

untitaker commented 7 years ago

General deployment may be an unsolved problem but your change isn't going to make it any easier.

untitaker commented 7 years ago

Thats what i was suggesting above.

K-os commented 5 years ago

So, it's 2019 and you are really still using plain md5? Really? Not even salted? I know I shouldn't judge software by the programming language it is written in, but things like this make it really hard ditch my prejudices and not to be sceptical about PHP-software in general.

Also, not using a proper password hashing algorithm to support plaintext HTTP is a really lame excuse post Let's Encrypt.

To be a little more constructive: A very low hanging fruit would be to at least generate a truly random authentication realm on first setup. This would at least improve the situation for new installations insofar, as an attacker then needs a different rainbow-table for each installation.

staabm commented 5 years ago

@K-os feel free to provide a pull request to fix this low hanging fruits.

This project was not maintained for a long time and recently a new maintainer has stepped up. The problem this project is using plain passwords is there because noone spents time working on it.

Most of us are are aware that plain passwords are a bad idea. You will also find 5 year old not maintained projects in other languages using plain passwords.. no reason to push against php..

K-os commented 5 years ago

Ok, I'm sorry. I didn't know this project was unmaintained. I just saw the really recent release and didn't realize the previous release was more than two years old.

staabm commented 5 years ago

@K-os no worries just provide a PR to fix this glitch and everyone is happy 😘

evert commented 5 years ago

@K-os a different realm per user is not an option, because we have to send the realm before we know what the user id might be.

Anyway, I do feel this warrants a follow-up, since it's a been a while since I replied here and I've since changed my mind.

I now believe that the right more forward is to:

  1. Switch to Basic authentication.
  2. Use bcrypt/password_hash to hash the password.

For a long while the trade-off was that we could either have (somewhat) safe passwords sent over plain text, or we could have good hashing and when I originally started sabre/dav in 2006, SSL certificates were not as available as they are right now.

So yea, I no longer stand by what I said earlier. But the issue remains that for this to change, somebody has to pick up the work. Ideally, I think this work is done in the sabre-io/dav repo as a new, more secure authentication backend before added to Baikal.

K-os commented 5 years ago

@K-os a different realm per user is not an option, because we have to send the realm before we know what the user id might be.

Yes, I know, but a different realm per installation would be possible. The currently used default constant is used in three different places in the code. I had a look into the code and tried to implement that, but I am not familiar with the used frameworks, so it was not a quick fix I could implement in my lunch break.

Using bcrypt with plaintext over https sounds like a good solution.

derStephan commented 1 year ago

Is this still an issue with the current version of baikal or is it fixed?