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

This PR can be reviewed commit by commit.

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 completely.

    Now, the piwik_auth cookie contains a secure hash of a randomly generated string (via password_hash($user['session_password']))) and the name of the user (required for "session reauthentication"). If the cookie is present in a request, Piwik core will use SessionAuth, and bypass plugin based authentication altogether. (Note: session_password is a new column in the user table.)

    The session now contains four new pieces of data: the logged in user's username, the IP address of the request that authenticated the session, the user agent of the request that authenticated the session and the time the session was authenticated. When SessionAuth is used, it first checks that the session is authenticated by checking that the username exists in the session.

    If it does, SessionAuth then checks that the IP address & user agent of the request and the value of the username cookie match the data stored in the session. If they don't, this is a session hijacking attempt. If they do match, then the current request is allowed to use the authenticated session, and no more work is done.

    If the piwik_auth cookie is sent, but the session does not exist, SessionAuth verifies that the secure hash is correct (via password_verify()) and recreates the authenticated session (referred to as "session reauthentication"; why this was done this way is explained below).

  • 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, the session start time & the use of the new "session_password" column results in sessions automatically becoming invalid after a password is changed.

    SessionAuth checks that the session start time (stored in the session) is newer than the last time the user's password was modified. So if the password was changed, the session can no longer be used.

    When performing "session reauthentication", if the password is changed, the user's session_password will be different. This will cause session reauthentication to fail, making the session invalid.

Existing Behaviors

There is one important existing behavior that influenced the implementation of this PR (specifically the need for "session reauthentication").

In Piwik the session lifetime is not managed server side, but client side through cookie expiration. When the piwik_auth cookie expires, the session has ended. This was likely done in order to circumvent PHP session garbage collection, which Piwik has no control over. If a session is deleted server side, authenticating using the token auth in the cookie allows Piwik to restart the session, avoiding random logouts.

This is why the "session reauthentication" mechanism was implemented in this PR. It maintains this behavior, restarting a session if it is deleted by PHP. This behavior can be abandoned, but then users will have to sync the [General] login_cookie_expire value with the session.gc_maxlifetime value (or if they're using PHP 7, do as PHP recommends and set it to 0 & use session_gc() via cron).

BC Breaks

There should be no BC breaks. Plugins do not have to call Login::initAuthenticationFromCookie() anymore, but they 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, user name & a secure hash of the randomly generated session password. Having the username is the only bit of information that we'd want to remove, but it is required to support session reauthentication.
    • Assuming the attacker has no limit to their resources, they would be able to get the password hash, then the password. I'm not sure how likely this is in the real world, though.
  2. Would an attacker be able to use this information 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 the random session password, the attacker would have to know the password beforehand. If they're able to break the hash and get the random session password, then yes. At least until the password is changed.

Possible attack vectors

This solution is more secure than what Piwik currently does (I think), but there are two potential attack vectors that aren't addressed. First is:

  1. Attacker gets session cookies.
  2. Attacker gains access to user's wifi or wifi that created the session (eg, public wifi).
  3. Attacker uses that wifi & session cookies to gain access to Piwik.

Second is:

  1. Attacker gets session cookies.
  2. Attacker breaks session hash (unlikely w/ bcrypt & a random password, but not impossible; see eg http://cynosureprime.blogspot.com/2015/09/how-we-cracked-millions-of-ashley.html) to get session password.
  3. Attacker creates a new auth hash w/ the session password & adds the cookie to a new piwik session.
  4. Attacker gains access to Piwik.

Performance Considerations

password_verify() adds a significant delay to individual requests (as it should). With the lowest bcrypt cost, it would add 3.5ms to each request. This is why it's only done during normal authentication & "session reauthentication" (which should not happen that often). When not doing reauthentication, SessionAuth will be very fast.

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/session_password columns will log everyone out.
  4. Using a randomly generated "session password" instead of the password hash means we can prevent a dictionary attack that would be possible if a user had a weak password. Using it instead of the token auth means the session is still tied to the password and will be invalidated on password change.

Todo

  • add extra salt to piwik_auth hash using randomly generated session secret Nevermind: can't do this. Since the session won't exist during reauthentication, there's nowhere to keep the session secret.

Even more security

Some ideas for making Piwik even more secure:

  • 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 / session passwords will 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 user 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 11th 2017 Member

Not sure if every test will pass, but this should be good to review.

@diosmosis commented on October 11th 2017 Member

FYI, going to add another commit, had a new idea.

@diosmosis commented on October 11th 2017 Member

Nevermind, not as good an idea as I thought.

@diosmosis commented on October 11th 2017 Member

Actually, nevermind to my nevermind, it might be a good idea...

@diosmosis commented on October 11th 2017 Member

Noticed an issue, putting back into WIP.

Powered by GitHub Issue Mirror