@diosmosis opened this Pull Request on October 18th 2017 Member

TODO

  • [x] change the update version to the right version (don't think 3.2.0... will work)

Changes

Note: security & performance considerations are discussed below.

  • Changed concept of "cookie authentication". Currently, the token auth is stored insecurely in the piwik_auth cookie. When this cookie is in a request to Piwik, this token auth is used to authenticate the request. The authentication is also delegated to the Auth implementation. This PR changes the strategy.

    Now, the session stores information related to who the session is for (including IP address, user agent and a randomly generated secret). The piwik_auth cookie now contains a md5 hash of this randomly generated string + the last time a user's password was changed.

    If the cookie is present in a request, Piwik core will use SessionAuth, and bypass plugin based authentication altogether. SessionAuth will check that the session has been authenticated already, check that the request using the session is from the same place that initiated the session, and that the password hasn't changed since.

  • A new column was added to the user table, ts_password_modified. This column holds the last time the user's password was changed. This column is used to automatically invalidate sessions after a user's password is changed (so we don't have to iterate over sessions or anything).

BC Breaks

There should be no BC breaks. Plugins do not have to call Login::initAuthenticationFromCookie() anymore, but existing implementations don't have to be modified.

Security Review

  1. What info is compromised if the piwik_auth cookie is compromised?

    • The attacker would have access to the session ID and a md5 hash of the session secret. None of this information gives the attacker direct access to Piwik.
    • The attacker should also not be able to guess new session secrets, even if the system uses a weak random number generator, since secrets are only created after a user logs in.
  2. Would an attacker be able to use the piwik_auth cookie to gain access to Piwik?

    • The token auth is no longer in the auth cookie, so they would not be able to use that.
    • The attacker could try to send the session cookies from another computer, but unless both the IP address and user agent are the same, they will not be able to use the existing session.
    • If an attacker has access to a user's machine or wifi, they would be able to use the session (provided they have the correct browser or the user agent string).
  3. What would an attacker gain if the server side session variables were somehow compromised?

    • The attacker would have an IP address, user agent & user name, but would not have the token auth or password, since nothing of the sort is stored in the session.
  4. Can an attacker generate their own valid piwik_auth hash?
    • Since the hash uses a secret that is unique to each session, they would have to know the secret of an existing session. Which means the hashes they generate would only be good for that one session.

Possible attack vectors

The only way I can think of for an attacker to gain access to a session, is for the attacker to:

  1. steal session cookies from an already authenticated session
  2. use the session cookies from the same network as the user

Performance Considerations

Since we're using a weak hash in SessionAuth, there's no real overhead to this solution.

Since sessions are not invalidated manually (eg, by iterating over every session), there is no overhead added to the change password workflow, either.

Keeping SessionAuth in core also allows plugin authentication to be more costly if required. Once a session is established, plugin based authentication won't happen again.

Other Notes

  1. This PR should not change how tracker requests are authenticated.
  2. If cookies are sent w/ every AJAX request, then we don't need to pass the token auth to the client. (So we could potentially remove the token_auth from https://github.com/piwik/piwik/blob/9243b9a7b6fae8237596f76cda1fe8b6816463af/plugins/Morpheus/templates/_jsGlobalVariables.twig . Not sure if that would be a BC break, though.
  3. The update for the ts_password_modified columns will log everyone out.

Even more security

Some ideas for making Piwik even more secure (not necessarily related to this PR):

  • Replace Common::generateUniqId()'s use of md5 & uniqid w/ random_bytes() (there's are polyfills for PHP 5.*, eg, https://github.com/symfony/polyfill). Would prevent attackers from being able to guess what new token auths would be.
  • Add "recognized user agents" feature (akin to what facebook does). If a user logs in on an unknown device, it must be saved by an existing session before you can log in.
  • Two factor auth (using virtual device preferably).
  • Saving more information about the current device using Piwik (though I'm not sure if this info is available client side).
  • Add a password strength requirement that encourages large passwords.

Fixes #6531

@diosmosis commented on October 21st 2017 Member

I removed the session secret, so now the piwik_auth cookie is empty. It is, however, required (at the moment anyway). In FrontController we check if this cookie is sent and if so, we use SessionAuth instead of the normal Auth implementation.

We can't use the PIWIK_SESSID cookie (which is the PHP session ID) since in the UI there is always a session, even when you're logged out. I believe this is for form nonces (like the one used in the login form), which are stored in sessions.

I thought about checking if the session is authenticated (ie, the 'user.name' variable exists in the session), but that would mean always doing session_start(), and to my understanding, we don't want to do that for, eg, every API request, just for requests from a browser.

@tsteur commented on November 2nd 2017 Member

Haven't done an actual test if it works but looked at the code and left a few comments. Overall looks good but need to make sure everything still works as before.

@diosmosis commented on November 23rd 2017 Member

@tsteur Applied feedback and rebased.

@diosmosis commented on March 5th 2018 Member

Rebased and retested this PR. Fixed two issues in the last two commits:

  • anonymous user couldn't be logged in anymore. fixed by running normal auth if session auth fails. (probably caused by removing session auth cookie)
  • clear session fingerprint on logout so old session can't be manually entered into the browser to login.
@diosmosis commented on March 5th 2018 Member

For the record here are the manual tests I ran (in addition to the automated tests for SessionAuth):

  • normal login correct password
  • normal login, wrong password
  • normal login in one browser, normal login in another browser (two simultaneous sessions)
  • password change in one browser logs out other browser
  • normal login & logout, re-enter session cookies manually in browser, does not auto login
  • login in one browser, copy session cookie in another browser, does not login in other browser
  • anonymous user auto-login
  • session stays alive according to login_cookie_expire, even if maxlifetime is small
  • login w/ remember me
  • api request w/ token auth, no logged in session

These tests involve FrontController & the login page, so they're a bit harder to automate (in a performant manner anyway).

@diosmosis commented on June 13th 2018 Member

@matomo-org/core-team can this get reviewed/merged now? The last commit needs to be reviewed (noticed the "remember me" login functionality was broken since the call has no effect after a session is started).

@tsteur commented on July 24th 2018 Member

@diosmosis left a few comments as you noticed otherwise looks fine to me. Main concern be now only to keep BC... but I reckon this should be 👍

@mattab commented on August 28th 2018 Member

It's awesome to have this available now :tada:
@diosmosis Well done. Following up to your notes "Even more security":

Replace Common::generateUniqId()'s use of md5 & uniqid w/ random_bytes() (there's are polyfills for PHP 5.*, eg, https://github.com/symfony/polyfill). Would prevent attackers from being able to guess what new token auths would be.

Created https://github.com/matomo-org/matomo/issues/13357

Add "recognized user agents" feature (akin to what facebook does). If a user logs in on an unknown device, it must be saved by an existing session before you can log in.

could you please create an issue to keep track of this security improvement?

Two factor auth (using virtual device preferably).

covered in https://github.com/matomo-org/matomo/issues/13325

Add a password strength requirement that encourages large passwords.

covered in https://github.com/matomo-org/matomo/issues/13070

This Pull Request was closed on July 26th 2018
Powered by GitHub Issue Mirror