@mnapoli opened this issue on August 20th 2015

Fixes #8548

Refactored the way password login is done: instead of computing the token for the given login/password and then checking the token, login/password is now directly checked against the database. Since DB string comparison is case insensitive, it allows logging in with case-insensitive login (when logging in through the login/password form).

Added more tests to cover this new behavior + existing "log in with password" behavior.

Please review extensively. Also I have no idea why the login was working the way it was working before, maybe I just missed a use case…

(tip for reviewing: the first commit is a small refactoring, review them separately it's easier)

@diosmosis commented on August 20th 2015

Looks like Piwik always used the token auth and not the password: https://github.com/piwik/piwik/blob/0da8fa6a7f4f77412eb0f8cda966914416fad977/plugins/Login/Auth.php

Not sure if it confers any security benefits, going to investigate further...

@diosmosis commented on August 20th 2015

Found initial use of token_auth for logging in: https://github.com/piwik/piwik/commit/62ff9b52a06d97bd7edecd9d81bb95d060763800

Before this, it used the 'password' as the credential column (for Zend's auth adapter).

My guess is that it was put in in preparation for API authentication. Since that uses only token auth, it may have been easier to have one code path instead of two.

I cannot think of any security benefits of using only the token auth. Someone could potentially brute force the password just as easily as when authenticating against the hashed password.

@diosmosis commented on August 20th 2015

I can see a couple issues w/ merging this as is: 1. Can you make sure users cannot be added if there exists a user w/ the same name but different capitalization? Can you also make sure there is a test covering this? (Before posting this I checked for the answer to the first question, and it appears you cannot add a user w/ a similar name. I did not check for a test though.) 2. If there are users in current installs that are different based on case sensitivity, one of them will end up being ignored. To solve this, can you make Model::getUser() favor users w/ the exact same name as what is given? Ie, if the user's name is an exact match, use that user, otherwise use the first user that matches w/ case-insensitive compare. I'm not sure if it's currently possible for this to be the case, though, unless the user was added in the past or was added manually to the DB (or added through 3rd party code). We can change this behavior for 3.0, but I think for the LTS 2.15 version, we should try to maintain BC.

@diosmosis commented on August 20th 2015

One more thing: can you make the case insensitivity explicit in Model::getUser()? Either by adding a comment or changing the code. It would be helpful to developers to see that case insensitivity is the expected behaviour when messing w/ the code. If you think a test is enough though, feel free to ignore this.

@mnapoli commented on August 20th 2015

Forgot to mention it but for the record another motivation for this solution (i.e. login using the password, not always going through the auth token) is to help a tiny bit getting rid of md5() in the future. That's one less behavior coupled to md5 (because generating the auth_token from the login/password was using md5).

can you make the case insensitivity explicit in Model::getUser()? Either by adding a comment

Done. Comment + test should do it.

  1. Can you make sure users cannot be added if there exists a user w/ the same name but different capitalization? Can you also make sure there is a test covering this?

Yep a test for that will be good. Will do it.

  1. If there are users in current installs that are different based on case sensitivity, one of them will end up being ignored.

I hope that currently it's not possible to register with the same login and different case. Else this will be a mess. We'll see tomorrow ;)

@mnapoli commented on August 21st 2015

@diosmosis It should be good with the last 2 commits. Check also b4c52c72245219e7ddaddad98882ebae75e9db17, it's quite ugly but if you think that it's possible that some users could exist somewhere with same login/different case then…

@tsteur commented on August 21st 2015

As the initial request was re email address (which in another plugin may be used as login), does it maybe make sense to update methods like getUserByEmail(), userEmailExists() as well? Think they are already case insensitive though so please ignore it :)

Also wondering if there was a possible attack to create a new account with an existing username but different case, eg first letter uppercase, and then when logging in, I'm getting a different users identity or cannot log in anymore?

Eg there is a user with username test.tester. Now someone else is signing up with username Test.Tester. Now someone is trying to log in via Test.tester... it would probably always return the first user test.tester: https://github.com/piwik/piwik/pull/8610/files#diff-3a5ed5014067501a6c9488796d449cd5R161 so for the user Test.Tester it wouldn't be possible to log in case insensitive. It's an edge case I know.

Guess all I'm saying is can we prevent the creation of new users with the same username but different case? I think it's already the case and it is not even possible to have same username twice with different case so please ignore it all if someone can confirm this. userExists() looks like it's case insensitive

@mnapoli commented on August 21st 2015

Yes this is what we discussed above, see the latest commits (e.g. 9244504fc6bd8e32ee88b74f6d5510a4a751e833, b4c52c72245219e7ddaddad98882ebae75e9db17).

This issue was closed on August 21st 2015
Powered by GitHub Issue Mirror