Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use proper password hashing algorithm #514

Open
the-infinity opened this issue Mar 23, 2016 · 24 comments · May be fixed by #868
Open

Use proper password hashing algorithm #514

the-infinity opened this issue Mar 23, 2016 · 24 comments · May be fixed by #868

Comments

@the-infinity
Copy link

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
Copy link
Member

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

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

@evert
Copy link
Member

evert commented Mar 23, 2016

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
Copy link
Member

evert commented Mar 23, 2016

It actually has a chapter named A2

@the-infinity
Copy link
Author

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
Copy link
Member

evert commented Mar 23, 2016

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
Copy link
Member

staabm commented Mar 23, 2016

You should just use password_hash

@untitaker
Copy link
Member

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 [email protected] 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:
#514 (comment)

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

@evert
Copy link
Member

evert commented Mar 24, 2016

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
Copy link

citrin commented Sep 6, 2016

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
Copy link
Member

evert commented Sep 6, 2016

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
Copy link
Member

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.

@untitaker untitaker changed the title SHA256 / SHA512 instead of md5() Use proper password hashing algorithm Sep 10, 2016
@muelli
Copy link

muelli commented Jan 6, 2017

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
Copy link
Member

evert commented Jan 8, 2017

@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
Copy link

muelli commented Jan 9, 2017

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
Copy link
Member

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

@untitaker
Copy link
Member

Thats what i was suggesting above.

@K-os
Copy link

K-os commented Apr 24, 2019

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
Copy link
Member

staabm commented Apr 24, 2019

@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
Copy link

K-os commented Apr 24, 2019

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
Copy link
Member

staabm commented Apr 24, 2019

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

@evert
Copy link
Member

evert commented Apr 26, 2019

@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
Copy link

K-os commented Apr 26, 2019

@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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants