Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sessions with more security #12164

Closed
wants to merge 4 commits into from
Closed

Sessions with more security #12164

wants to merge 4 commits into from

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Oct 11, 2017

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.
  1. 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).
  1. 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.
  1. 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 diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Oct 11, 2017
@diosmosis
Copy link
Member Author

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

@mattab mattab added the c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. label Oct 11, 2017
@diosmosis
Copy link
Member Author

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

@mattab mattab added this to the 3.3.0 milestone Oct 11, 2017
@mattab mattab added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Oct 11, 2017
@diosmosis
Copy link
Member Author

Nevermind, not as good an idea as I thought.

@diosmosis
Copy link
Member Author

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

@diosmosis diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Oct 11, 2017
@diosmosis
Copy link
Member Author

Noticed an issue, putting back into WIP.

Instead of authenticating by token auth if it exists in the cookie, validate an existing session. If the session
has the user name stored as a session var, it has been authenticated. If the request has the same IP address and
user agent as the request that created the session, the request is from the user that created the session. If
both of these are true, then the session is valid, and we don't need a token auth to authenticate.

If the session is deleted before the Piwik auth cookie expires (due to garbage collection), we attempt to
re-authenticate using a secure hash of the token auth. We don't do this on every request since password_verify()
will, at BEST, add 3.5ms to every request.
Invalidation is accomplished w/o having to individually touch sessions by:

1. Using the password hash as the piwik_auth key secret, instead of the token auth. So when a password changes, existing piwik_auth keys are no longer valid. This affects session re-authentication.
2. Saving the session start time & the last time a user's password was modified, and checking that the session start time is always newer than the password modification time.
…a does not disappear, remove session re-auth functionality & tie cookie hash to password modified time instead of password hash to retain automatic session invalidation on password change.
@diosmosis
Copy link
Member Author

Closing in favor of #12208 which uses branch in piwik/piwik

@diosmosis diosmosis closed this Oct 18, 2017
@diosmosis diosmosis deleted the 6531-say-no-to-token-auths branch October 18, 2017 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants