Standardize password hash function usage #9480

Joey3000 · 2016-01-07T16:25:27Z

As preparation for introduction of a stronger password hash function in #5728, this standardizes the usage of the current password hash function across the Login and UsersManager plugins. Specifically:

Use UsersManager::getPasswordHash() throughout, instead of previous direct use of md5() in some places.
Concentrate the hash length sanity check in the newly created UsersManager::checkPasswordHash(). Previously, UsersManager\API and Login\PasswordResetter classes did it separately. Both checks were moved into the newly created function in the UsersManager class, as that is also where other public static password check functions reside.
Replace the "md5" string with "hash" in affected variable names and comments.

No functional change is intended.

~~One little caveat is that I can't test at the moment. It would be great if someone could take this on a test ride.~~

To summarize the hash function uses by the Login and UsersManager plugins:

The password hash function is now centralized in (https://github.com/piwik/piwik/blob/c54fc9e4e6072e7bffbb1a02b1c11645c50038e4/plugins/UsersManager/UsersManager.php#L153):

public static function getPasswordHash($password)
{
    // if change here, should also edit the installation process
    // to change how the root pwd is saved in the config file
    return md5($password);
}

Following hash function uses were not modified:

token_auth hash function is controlled by (https://github.com/piwik/piwik/blob/c54fc9e4e6072e7bffbb1a02b1c11645c50038e4/plugins/UsersManager/API.php#L789) (apparently for DB storage - this could be something for Hashed and salted storage of token_auth #9457):
```
public function getTokenAuth($userLogin, $passwordHash)
{
    UsersManager::checkPasswordHash($passwordHash, Piwik::translate('UsersManager_ExceptionPasswordMD5HashExpected'))

    return md5($userLogin . $passwordHash);
}
```
As well as by (https://github.com/Joey3000/piwik/blob/c54fc9e4e6072e7bffbb1a02b1c11645c50038e4/plugins/Login/SessionInitializer.php#L235) (apparently for cookie setting - this could be changed, e.g, at the same time as Hashed and salted storage of token_auth #9457 gets implemented, to have token_auth only change once):
```
public static function getHashTokenAuth($login, $token_auth)
{
    return md5($login . $token_auth);
}
```
Password reset token (per e-mail) is controlled by (https://github.com/piwik/piwik/blob/c54fc9e4e6072e7bffbb1a02b1c11645c50038e4/plugins/Login/PasswordResetter.php#L320):
```
protected function hashData($data)
{
    return Common::hash($data);
}
```

As preparation for introduction of a stronger hash function in matomo-org#5728, this standardizes the usage of the current **password** hash function across the Login and UsersManager plugins. Specifically: * Use UsersManager::getPasswordHash() throughout, instead of previous direct use of md5() in some places * Concentrate the hash length sanity check in the newly created UsersManager::checkPasswordHash(). Previously, UsersManager\Auth and Login\PasswordResetter classes did it separately. Both checks were moved into the newly created UsersManager class function, as that is also where other public static password check functions reside. * Replaced the "md5" string with "hash" in affected variable names and comments No functional change is intended.

As preparation for Common::hash() changing its hash function some time in future

For performance reasons

Joey3000 · 2016-01-08T01:51:57Z

Does anyone know why the test fails?

I've tested the patch with piwik-2.16.0-b1.zip (on a remote test server, updated from piwik-2.15.0-b14 via a complete delete and re-upload via FTP, keeping the DB tables) and don't get any errors with PHP 5.5. Tested:

As super user: Login plugin: login, logout; UsersManager plugin: create a "view" user, change own and "view" user's password
As the new "view" user: Login plugin: login, logout
Not logged in: password change (was accepted - failed correctly: server has e-mailing disabled)

I don't get any passwordHash related errors.

The only error I see does not seem to be related: UsersManager: When trying to change own password as the "view" user (not tested as super user), I get following error:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'use_12_hour_clock' in 'field list'

(Without trying to change the clock format. And similarly for 'use_24_hour_clock' if I try to change it to 24h-format.)

But that error could be due to the update from the 2.15.0-b14 beta. I'd need to try updating from a released version some time.

Joey3000 · 2016-01-08T02:13:55Z

I found it, thanks to https://builds-artifacts.piwik.org/piwik/piwik/master/17449/ (missing semicolon in Login\PasswordResetter::checkPasswordHash(), which I didn't test due to the disabled e-mailing on the server).

The remaining passwordHash-related issues are due to the function argument renaming in https://github.com/piwik/piwik/pull/9480/files#diff-243038712dace7532e3b1856dd63274eR789. (Including the API list screenshot - scroll down to the "Module UsersManager". So, it's expected. But I can revert the renaming, if desired.

Please let me know if it'd be better to do this in the 3.0 branch, which #5728 is for (or not at all).

tsteur · 2016-01-10T22:52:24Z

plugins/UsersManager/API.php

     * @return string
     */
-    public function getTokenAuth($userLogin, $md5Password)
+    public function getTokenAuth($userLogin, $passwordHash)


That's why tests fail as it breaks the API. Unfortunately we cannot rename this one although it would be good :(

Users call it eg with index.php?module=API&method=API.getTokenAuth&userLogin=foo&md5Password=bar.... This would break all applications, libraries etc that use this method (eg the Piwik Mobile App but also many other libraries and apps).

Unfortunately we do not have a versioning in our API to handle such cases. The only thing that I could imagine would be making $md5Password optional, add $passwordHash optional and then checking in the method that at least one of them is set and if not throw an exception.

We'd need to document this in the developer changelog and deprecate $md5Password parameter. We'd probably never actually remove that parameter instead deprecate the whole API and replace it with a REST API in a few years

tsteur · 2016-01-10T22:52:53Z

PR looks good! Thx for that. Only one issue with the failing test. Explained it in a code comment

To prevent API breakage - see matomo-org#9480 (diff)

See https://builds-artifacts.piwik.org/piwik/piwik/master/17471/processed/Login_formless_login.png

Joey3000 · 2016-01-11T08:12:40Z

Thanks for the feedback! I reverted the renaming (adding a comment in https://github.com/piwik/piwik/pull/9480/files#diff-243038712dace7532e3b1856dd63274eR786 instead) and fixed another issue left over.

I don't know what the remaining test failures are about, though. E.g.:

Piwik\Tests\System\PeriodIsRangeDateIsLastNMetadataAndNormalAPITest: Differences with expected in '/home/travis/build/piwik/piwik/tests/PHPUnit/Framework/TestCase/../../System/processed/test_periodIsRange_dateIsLastN_MetadataAndNormalAPI__Live.getVisitorProfile.xml'

Failed asserting that two DOM documents are equal.

--- Expected

+++ Actual

@@ @@

 <?xml version="1.0"?>

 <result>

    <totalVisits>2</totalVisits>

    <totalVisitDuration>361</totalVisitDuration>

    <totalActions>2</totalActions>

+   <totalEvents>0</totalEvents>

+   <totalOutlinks>1</totalOutlinks>

+   <totalDownloads>0</totalDownloads>

    <totalSearches>0</totalSearches>

+   <totalPageViews>1</totalPageViews>

[...]

As well as the moved text in the screenshots in https://builds-artifacts.piwik.org/piwik/piwik/master/17472/.

I would appreciate any hints.

tsteur · 2016-01-11T19:39:50Z

Thx for the quick change! The failing test is not due to your change. I merged a couple of PRs yesterday but haven't fixed the build yet.

Standardize password hash function usage

Joey3000 · 2016-01-31T09:21:56Z

The only error I see does not seem to be related: UsersManager: When trying to change own password as the "view" user (not tested as super user), I get following error:

  SQLSTATE[42S22]: Column not found: 1054 Unknown column 'use_12_hour_clock' in 'field list'

Reported in #9666.

Joey3000 added 5 commits January 7, 2016 12:17

Comment update only

76a29a6

As preparation for Common::hash() changing its hash function some time in future

Revert hash calculation on comparison

c54fc9e

For performance reasons

Bugfix (syntax)

0b2c333

Syntax error fix

ca39cfe

tsteur reviewed Jan 10, 2016
View reviewed changes

Joey3000 added 2 commits January 11, 2016 05:48

Revert renaming of getTokenAuth API parameter

61f2a1c

To prevent API breakage - see matomo-org#9480 (diff)

Fix missing use Piwik\Piwik

664f61c

See https://builds-artifacts.piwik.org/piwik/piwik/master/17471/processed/Login_formless_login.png

tsteur added a commit that referenced this pull request Jan 11, 2016

Merge pull request #9480 from Joey3000/standard_hash

fa5dcae

Standardize password hash function usage

tsteur merged commit fa5dcae into matomo-org:master Jan 11, 2016

tsteur added the Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. label Jan 11, 2016

tsteur added this to the 2.16.0 milestone Jan 11, 2016

Joey3000 deleted the standard_hash branch January 12, 2016 02:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize password hash function usage #9480

Standardize password hash function usage #9480

Joey3000 commented Jan 7, 2016

Joey3000 commented Jan 8, 2016

Joey3000 commented Jan 8, 2016

tsteur Jan 10, 2016

tsteur commented Jan 10, 2016

Joey3000 commented Jan 11, 2016

tsteur commented Jan 11, 2016

Joey3000 commented Jan 31, 2016

Standardize password hash function usage #9480

Standardize password hash function usage #9480

Conversation

Joey3000 commented Jan 7, 2016

Joey3000 commented Jan 8, 2016

Joey3000 commented Jan 8, 2016

tsteur Jan 10, 2016

Choose a reason for hiding this comment

tsteur commented Jan 10, 2016

Joey3000 commented Jan 11, 2016

tsteur commented Jan 11, 2016

Joey3000 commented Jan 31, 2016