@anonymous-piwik-user opened this issue on July 19th 2008

What I can see from code is that passwords are hashed with md5, but without a salt. That's not secure enough, take look at here: http://ilia.ws/archives/68-MD5-Dictionary-Attacks.html

See also the request to use different salts for different hashes: #3051

@robocoder commented on January 11th 2009

Might want to consider http://www.openwall.com/phpass (public domain password framework already adopted by other open source projects).

@anonymous-piwik-user commented on January 13th 2009

All you need to do is add a salt before hashing the password (the longer the better), as reported in the summary.

@mattab commented on December 28th 2010

We should have a salt for Password to avoid dictionnary attacks or prevent password leakages on other websites to impact a user login to Piwik.

@robocoder commented on December 28th 2010

Note: salting is a compat buster. We can't salt existing passwords because they're already hashed.

To migrate: - add a flag column to each user; if true, user must change their password when he/she next logins; regenerate token_auth at that time - move current Login auth to a migration plugin that hooks into the authenticator; this plugin would be deactivated after everyone has changed their passwords

For more robustness: - configureable hash() algorihtm (defaults to md5); increase md5password and token_auth size to 128 chars (hexits) - implement double salt in the new Login auth

@alexlehm commented on July 24th 2012

I would like to add two concerns about md5 unsalted passwords

a hash is resolvable by googling if the password is a plain word, you do not even need a dictionary, e.g. https://www.google.com/search?q=14c4b06b824ec593239362517f538b29

all hashes for passwords up to a given length can be resolved by a rainbow table attack, so a leaked password table can be processed without any brute force operation

(if you need additional arguments, take a look at a few recent pw leaks e.g. on linkedin)

@mattab commented on December 16th 2012

When we implement per user salt, we should also add in - when user change the password, also invalidate all other currently logged in sessions for this users (ie. change the user's salt?)

@halfdan commented on February 4th 2013

We should use the new Password API from PHP 5.5 for this! At the end of the article there is a link to a compatibility implementation for earlier PHP versions.

https://gist.github.com/3707231

If we agree on something I am keen on implementing this one.

@mattab commented on February 5th 2013

This new API looks great! But it will need to work of course for 5.1.x to 5.4.x as well, also we need to plan if "crypt" extension is not available (prior to 5.3 it seems crypt might not be available always)

@alexlehm commented on February 1st 2014

I cannot believe that this is still buried as a low priority ticket and has been ignored for 6 years.

md5 password storage is completely insufficient, this poses a significant security risk.

md5 passwords with salt are also insufficient since md5 cannot be considered secure at all anymore.

To achieve an acceptable level of security, it is necessary to use a newer hash method like bcrypt, scrypt or another key derivation function, most previous pw methods are too weak and have been for some time.

Sacrificing this with the argument of backward compatibility is criminally stupid, sorry.

@robocoder commented on February 1st 2014

@alexlehm In future, make your comments constructive. The challenge is migrating existing users.

@alexlehm commented on February 1st 2014

I'm sorry if that is non-constructive, however the current pw scheme is such a major issue that this must be addressed sooner rather than later.

Serendipity used a rather smart method to migrate user pws to a new scheme a while back where they defined a cut-off date 180 days after the migration was activated (based on the installation, not on the release of the software) so that all active users migrated themselves without even knowing and inactive users were locked out after the grace period.

E.g. migrating the admin user would work like, since the user will log in anyway. I am not sure how the auth mechanisms work internally, but for anything that is authenticating on a client API, an API hash would be better then using a real password.

@mattab commented on February 1st 2014

Increasing priority... Yes it's an important ticket, but as you can see, there are a lot of important tickets to work on, the challlenge is that we have a very small team... Are you familiar with PHP? If you are interested and familiar with php, please consider joining the project or helping with this!

@alexlehm commented on February 1st 2014

Actually I could do that, have to read up on the piwik source code first, even though I am using this for quite a while I have never looked at anything except the config file.

@halfdan commented on February 1st 2014

@alexlehm It would be nice if you'd join up for sure :)

@mattab We discussed this a few days ago - I'd very much like to do this with the new PHP 5.5 password API (+ the 5.3 fallback).

The way this could work is that for all installations with PHP >= 5.3.7 we automatically update the passwords to bcrypt once they log in (the 5.3 fallback only works with >= 5.3.7 since there were issues in earlier PHP versions). This would leave quite a number of people out of the loop, but if they cared about security they wouldn't be running 5.3.x in the first place :)

Cheers!

@mattab commented on February 3rd 2014

Another challenge: maintain backward compatibility for token_auth - Since token = md5($userLogin . $md5Password) (see UsersManager\API) then changing password will change the token. this may cause some problem and break some tools/apps using the previous token... - we have to clarify all places where md5password is used, this will break some usersManager APIs. Can we keep BC in these APIs by keeping the md5Password parameter?

@alexlehm commented on February 4th 2014

using the hashed password to create the token without any additional data is not so good, this makes it impossible to change the token without changing the password as well, it should be possible to invalidate the api tokens by changing the password to the same value (or to create a new token by clicking a button without affecting the user account at all).

Have to look into this a bit further, it should be possible to keep both but disable the old functionality when the user chooses so.

@halfdan commented on February 4th 2014

I agree, the token should be decoupled from the password completely. We could simply generate a unique string with some random data.

It would be really cool to invalidate the token without having to change the password, so I'll be looking into that.

@mattab commented on March 7th 2014

Things to think about: - logme mechanism http://piwik.org/faq/how-to/faq_30/ - should not use md5 anymore but the more ecure hashing (via API?) - how do we keep BC for logme - how do keep backward compatibility in general - update tests - Log analytics http://piwik.org/log-analytics/ import_logs.py https://github.com/piwik/piwik/blob/master/misc/log-analytics/import_logs.py#L574 - Other modules, see: https://github.com/piwik/piwik/search?l=php&q=md5*password&utf8=%E2%9C%93

@etjossem commented on September 11th 2015

Please try to auto migrate users to bcrypt, etc. Unsalted MD5 is kind of a scary vulnerability.

See: https://github.com/piwik/piwik/issues/8753

@ascorbic commented on December 8th 2015

It's perfectly possible to generate stateless tokens (i.e. without the need for sessions) based on a password, but which are secure and can have expiry dates. I did a simple implementation a few years ago: https://github.com/ascorbic/php-stateless-cookies The "cookie" generated by that would work fine as an API token.

@paragonie-scott commented on December 8th 2015

https://www.reddit.com/r/PHP/comments/3lwxlw/hash_and_verify_passwords_in_php_the_right_way/cva6y6p

This is a solved problem.

@ircmaxell commented on December 8th 2015

@ascorbic @ascorbic there are issues with that which render it less secure (the password should be derived using a KDF before it's used as an HMAC key). I raised an issue on that project to communicate that.

@ascorbic commented on December 8th 2015

@ircmaxell Thanks. It's a very long time since I wrote that and I do need to have a better look at it. I linked it more of an illustration of the concept of stateless tokens than as a suggestion it were actually used here, but I should certainly fix that.

@mattab commented on December 18th 2015

We've moved this issue to 3.0.0 milestone. If anyone reading is up for an interesting challenge, we welcome Pull requests and happy to collaborate on this security improvement!

@Joey3000 commented on January 2nd 2016

https://www.reddit.com/r/PHP/comments/3lwxlw/hash_and_verify_passwords_in_php_the_right_way/cva6y6p

This is a solved problem.

:+1: It's implemented similarly here: https://github.com/joomla/joomla-cms/blob/3.4.8/libraries/joomla/user/helper.php#L326.

Note: The PHP password_*() functions need PHP 5 >= 5.5.0, which should be fine with Piwik 3.0 - see https://github.com/piwik/piwik/issues/8156. Otherwise, there is this: https://github.com/ircmaxell/password_compat.

@jimlei commented on February 18th 2016

We've moved this issue to 3.0.0 milestone. If anyone reading is up for an interesting challenge, we welcome Pull requests and happy to collaborate on this security improvement! - mattab commented on Dec 18, 2015

Which version of PHP does Piwik 3 aim to require? Atm it's set up to require PHP >=5.3.3 in 3.0.0 composer.json, which is a bit low.

password_hash requires PHP >= 5.5 (ircmaxell/password_compat lowers this to PHP >= 5.3.7)

I'm thinking this would be a very good (necessary) improvement - perhaps in combination with changing the MD5 token to a JWT, or similar

@mattab commented on March 31st 2016

@jimlei Piwik 3.0.0 will require PHP 5.5, see: #8156

@mneudert commented on September 10th 2016

Hi all,

after a chat with @sgiehl some time ago I took a look at what could/should/might be done to replace the current md5 password hashing with the new password_* functions as well as some ideas to revamp the API authentication.

I hope I have not missed or misunderstood any already existing feature.

## Bcrypt hashing

Just switching bcrypt as the hashing algorithm is probably not that much of a problem (with an optionally configurable hashing cost param). Keeping a safe and user friendly upgrade path is the one thing that needs some attention.

### Upgrading the user performing the upgrade

The easy one. We could just ask the administrator to enter his password and rehash it according to the algorithm changes. After all the update has to be done by someone and is not "headless" by default. Only works for one user...

### Other accounts

Every account excepting the one performing an upgrade are a bit more tricky.

If the passwords are not reset during the upgrade it would be necessary to have a backwards compatible "upgrade password on next login" feature. And having administrators skip versions when upgrading it might need to be available for some time.

Resetting the passwords and sending all users an email with a temporary one-time password should be user-friendly enough to be a viable solution.

Afaik right now there is no such feature available. A different way I found was setting a new password and then confirm it using a reset token. But that is of course not usable during an upgrade process as there is no password we could store in the reset_password_info option.

We would need to have a reset token sent to the user and THEN ask for the new password. This change in behaviour here could replace the old workflow.

Having one-time password and reset token emails could also remove the need for manual password editing in the user administration. Just have some "force reset on next login" and "assign temporary password" buttons should do the trick.

## API Tokens

The current token_auth could be replaced with a more detailed control over API token creation.

I think about having each user request multiple API tokens and name them after what they use it for. Each token could then be individually be revoked if necessary while not breaking every tool accessing the API.

Generating tokens could be as easy as hashing some junk bytes from /dev/urandom (or just a password_hash). That should be random enough to not provide a realistic chance of guessing the tokens.

By default we could generate a token for each super user to use with tools like the python log importer. Imho a user should not be allowed to use his real password with any API but changing that behaviour might break more stuff than worth it.

### API only users

Having a user not being allowed to login to the web interface while granting access to the API might be some nice thing for tools to have. The user should be completely blocked from the web interface, not just have a password "no one knows".

## "Remember Me"

(based on the already existing plugin for that feature)

Using the tools already available in Piwik (geolocation, device detector, ...) a list of active sessions could be integrated to the user backend. The information could be checked upon login/session use to avoid having a login session moved between devices (read "stolen"). Each long-time cookie could be deactivated at will in the backend.

To have these session secured from hijacking they should not be allowed to do administrative actions without re-entering the accounts password from time to time. Think about GitHub asking for your password once a day when managing your account. The information about that state should of course be tracked individually for each login cookie.

## 2FA

Well, the title of the topic says enough. Always a nice thing to have. With the right plugins it shouldn't be a huge problem to integrate it if SMS tokens are no requirement.

The biggest problem before doing anything of the above is:

Where is breaking backwards compatibility and require users to adjust to new workflows acceptable and where not?

@paragonie-scott commented on September 10th 2016
@tsteur commented on September 11th 2016

Upgrading the user performing the upgrade

I was thinking of the following: - User logs in, we check if password string is 32 char. If so, we hash original password and save the newly hashed password in the DB in the same column (hashing using password_hash()).

I don't think it's needed to email users and so far I think we have not done this. This would be basically up to each Piwik Administrator whether they want to email their users and ask them to log in again so their password is updated.

API Tokens

Sounds like https://github.com/piwik/piwik/issues/6559

Here we basically have the raw token in the DB and could simply run password_hash on them. Problem is when a user wants to authenticate using a token we can't simply do a query like select * from users where api_token = password_hash($passedApiToken). The problem is, in API calls we do not have a username where we can get the hashed password and then use password_verify($passedApiToken). We would need to compare it individually against each user (where there could be > 30k users).

It may work to generate the original hash using something like https://github.com/ircmaxell/password_compat/blob/v1.0.4/lib/password.php#L235 and then we are able to query for that user in the DB but not sure if it works. Also it might be a problem as on some installations crypt might not be available. So we would not be able to rely on the crypt extension and would need an alternative there.

In the UI we would not be able to show the original token anymore on the API page which is good. Instead users would be able to add / revoke tokens etc.

API only users

That would be really great to have if not too much effort

@mneudert commented on September 12th 2016

So the basic target is "a silent upgrade with full compatibility". Sounds fine :+1:

Silent password update

Comparing the hash length and then silently switching over is probably the most suitable way without having to add separate database field, option keys, ....

Probably also the most common way to deal with that issue. And as already mentioned a solved problem since long ago.

That would keep everything working for several releases until support is completely dropped from the core. (Or potentially moved to a separate plugin for large installations that cannot notify users even if they haven't used their password for "a long time" and therefore skipped the upgrade.)

API tokens

I might miss something but it should not be a big problem at all.

While upgrading the release all existing users could get one token auto generated using the old method. Whether new users get a default token or not is a different thing. As the token_auth database field already has a unique key we could just take that over to the new storage without running into problems with duplicates. That makes a frictionless upgrade path leaving it the administrator's choice to force users to generate new API tokens or not.

For generating/validating the tokens I think the part about password_hash was a bit confusing. The hashing was only intended to generate a random string. It was not my intention to say any incoming request for token authentication should hash the token and then compare it.

Tokens should be no problem to be stored in plaintext as they are already. For a usable token management interface they have to be available in a readable form anyway. Cannot think of a way to manage a token someone cannot read :D.

And as we have to store them that way it sounds suitable to also compare them that way.

password_compat seems to be a perfect resource for the parts to use/"copy". The source range #L104-L138 shows a really portable way to generate new tokens. And at line #L241 is the timing attack resisting comparison.

With some special care it might also be possible to keep the upgrade process smooth for tools like python log importer. Like saying "please do not revoke your default key until you have updated your tools"...

API only users

Have not looked at the implications that idea would have. But having disconnected tokens and credentials should make it at least a candidate for a 3.1.0 release if not earlier.

@tsteur commented on September 12th 2016

For generating/validating the tokens I think the part about password_hash was a bit confusing. The hashing was only intended to generate a random string. It was not my intention to say any incoming request for token authentication should hash the token and then compare it.

For improved security it would be really good to have the API tokens hashed. Just in case for some reasons someone gets access to DB etc. Of course they could then also just overwrite the token with a new one but it would be still good security to have it hashed. There are probably solutions for this but I haven't checked yet. Usually authentications consist of a key and a secret which makes it easier as only having the secret.

Tokens should be no problem to be stored in plaintext as they are already. For a usable token management interface they have to be available in a readable form anyway. Cannot think of a way to manage a token someone cannot read :D.

When creating a token eg on Google they show it once when the token is generated. They ask you to "save" it eg in a password manage or to use it in your mail client etc. There is no way to see that token again. If you "lose" it, you need to generate a new one (and ideally delete/revoke the old one).

If we'd still keep the token in plain text in the DB there is not really a big reason to change anything (for now). It would be still a benefit to generate the token more randomly though. So if we decide to keep the token in plain text in the DB until we have a REST API we'd only need to improve the token generation IMO.

I just noticed if we decide to hash the API token we would need to also fix eg https://github.com/piwik/piwik/issues/10185 as the token is currently used for the export feature and for other things. However, we would not be able to get the "original" token anymore because it is hashed.

@ircmaxell commented on September 12th 2016

For improved security it would be really good to have the API tokens hashed. Just in case for some reasons someone gets access to DB etc. Of course they could then also just overwrite the token with a new one but it would be still good security to have it hashed. There are probably solutions for this but I haven't checked yet. Usually authentications consist of a key and a secret which makes it easier as only having the secret.

For this use-case, a simple SHA-256 on DB-insert should suffice.

@tsteur commented on September 12th 2016

For this use-case, a simple SHA-256 on DB-insert should suffice.

That's a good point :+1:

@J0WI commented on September 12th 2016

It would be great to have the plain md5 hashes mitigated during the update even if the users will not log in for a while.

We could hash the current md5 hashes a second time with a stronger algorithm and with salt and pepper. These hashes can be flagged (so the login script knows that it need to hash them twice to verify) and updated during the next successful log in. If the hashes become public for any reason, we have no plain md5 hashes in DB.

The md5 hashes are the main issue here. So if the above solution won't be implemented, I would go with forcing a password reset for every user. History showed that plain md5 password hashes are always evil, even for inactive users or hosts.

Edit: If something similar is done with the API tokens, it might be possible to be backwards compatible. But for security reasons this should be behind a pref and removed completely in future updates.

@mneudert commented on September 12th 2016

Is there any feature that could not be modified to use the API proxy defined in ee3bc9c? Or at least not short-term modified?

A search for token_auth usage find results like the DocumentationGenerator (example URLs for RSS feeds if I get it right?) or UserCountryMapController.

Hashing stuff is always good but if a feature breaks it might be a deal breaker.

If these are "user facing" features for "URLs without login" we could go with hashing and update the messages/documentation to point at the token management. Users without any tokens available would get displayed a nice message to create a token. And going back to a string length check we could have the legacy token only available for existing URLs or "people who know" while not including it in the check for potentially available tokens.

@tsteur commented on September 13th 2016

We cannot use the solution in https://github.com/piwik/piwik/commit/ee3bc9cc1abc566464cb6428376a542e757eef3a as it is missing an nonce to prevent CSRF attacks. We would need to pass along an nonce token or a token/access key that is only valid for limited time but ideally an nonce. We also currently use the token_auth in API requests from JavaScript. This is pretty much the same case and here we would need to use something like nonce as well.

I think the most critical things for now are: 1) Migrate password via password_hash on login 2) Generate token auth randomly and not based on login / password

This will already be a great improvement.

Next step that will be needed eventually is to hash the token_auth which we will have to do for sure. If not in 3.0, then in 3.1 as it shouldn't break anything (I reckon).

It would be great to focus for now on the first two things. Once this is done we could do another evaluation on the steps.

@tsteur commented on September 25th 2016

@mneudert or @sgiehl will you work on this?

@sgiehl commented on September 25th 2016

@mneudert is working on that. See https://github.com/piwik/piwik/compare/3.x-dev...mneudert:password_hash

@J0WI commented on September 26th 2016

How do you think about my proposal in https://github.com/piwik/piwik/issues/5728#issuecomment-246502440 to migrate plain md5 hashes during the update procedure and not only on login?

@tsteur commented on September 26th 2016

be worth having a look into it for sure. Would probably only need to check how to flag it.

@mneudert commented on September 26th 2016

Currently I plan to rehash every password with SHA256 during the upgrade combined with setting a "_legacy_password"-Option stored for each user. After a first valid login this option will get deleted and a proper rehashing will update the password.

Having all passwords rehashed also nicely shows where the password/token_auth is in use with all the tests breaking :D

@HybridAU commented on September 27th 2016

I might be misunderstanding the process you are using here, and for what it's worth I really appreciate you doing this work so I don't want this to come across as just complaints from the peanut gallery.

But I think if your going to go through all the effort of changing the password storage method you should move to something like PBKDF2 or bcrypt. They are designed for password storage and have things like a salt and configurable iterations built in.

Rather than moving to SHA256 which while better than MD5 still shares many of it's faults (e.g. With out a salt two passwords which are the same will have the same hash and so rainbow tables can be made. Also there is no easy way to increase the number of required iterations and so the time to create a hash which is already quite low, will only ever get lower.)

@tsteur commented on September 27th 2016

I'm not quite sure but I think the SHA256 is only for auth tokens and not for end user passwords see discussion further above. end user passwords should be using http://us2.php.net/manual/en/function.password-hash.php (bcrypt)

@mneudert commented on September 27th 2016

Yeah, that's right. I think however that needs some deeper refactorings for example regarding the tests matching password hashes in their XML fixtures. Plan is to change SHA256 with bcrypt for old password once new password are hashed that way. Then everything should be in place to handle that switch a lot easier than right of the bat.

@mattab commented on October 6th 2016

Hi @mneudert @sgiehl - what is the status for this security improvement? if possible, we would love to include it in the Piwik 3.0.0-beta2 or beta3 in coming days. We've just released the first public beta of Piwik 3.0.0: https://piwik.org/changelog/piwik-3-0-0-beta1/

@mneudert commented on October 9th 2016

Currently I have switched the password hashing to bcrypt (re-hashing of old passwords yet to be added) and worked on the token separation/extraction. If it helps I could open a WIP pull request for that part (with a list of caveats attached as some things might seem somewhat "quirky" otherwise).

There are still some things to sort out like a proper handling of md5 hashed passwords used in API requests or what should happen if a user has no API token (last one deleted or none ever created).

@tsteur commented on October 9th 2016

PR with caveats sounds good :+1:

For now we wouldn't need the feature to create multiple tokens etc and be good to only migrate existing tokens (each user has one token) but if you're keen on implementing this as well with multi tokens :+1: . Maybe it would be possible to split it into 2 PRs as just making passwords and token more secure by hashing them could be much more simple compared to managing multiple tokens maybe. Ideally the UI would not rely on using API tokens and only use nonce or generated short-term tokens that are always different per session or so for better security. The UI should eventually not use an API token and therefore, when a user deletes all tokens the API would be basically not usable which is a good feature.

Main feature would be: On login bcrypt password if not done yet, on update hash all tokens with SHA256 and generate the tokens in the future more randomly. Those three things would be main improvement for now. Everything else, like app-specific tokens is great to have :)

@mattab commented on December 2nd 2016

Awesome work @mneudert @tsteur - this important security improvement merged in #10926 :tada:

@mlissner commented on December 7th 2016

I've only skimmed this thread, but I don't see anywhere that we're keeping track of the bcrypt configuration parameters. Ideally when we upgrade people's existing passwords, we will save all of the following:

  • Algorithm
  • Iterations
  • Salt
  • Hashed password

For example, this is the value that Django stores for every user. It's a $ delimited field in the database:

pbkdf2_sha256$20000$random-salt-here$hashed-password-here

This says:

The password was hashed using 20000 rotations of the pdkdf2 sha256 algorithm using the salt random-salt-here and resulted in the hash hashed-password-here.

Apologies if this is all being taken care of already (I didn't check the code), but we need to store ALL of this so that we can later upgrade the algorithm, the number of rotations, etc. For example, Django regularly upgrades the number of rotations as computer hardware and software become faster. If we don't do this now, we'll have another headache down the road when we need to upgrade things again.

Here's Django's documentation. It's very good.

@tsteur commented on December 7th 2016

The algorithm, cost and salt etc is stored in the password see http://us2.php.net/manual/en/function.password-hash.php

@mlissner commented on December 7th 2016

Fantastic! I'll go on my now. Apologies for the drive by. I'm very happy to see this improvement.

This issue was closed on December 2nd 2016
Powered by GitHub Issue Mirror