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
diosmosis
wants to merge
4
commits into
matomo-org:3.x-dev
from
diosmosis:6531-say-no-to-token-auths
Closed
Sessions with more security #12164
diosmosis
wants to merge
4
commits into
matomo-org:3.x-dev
from
diosmosis:6531-say-no-to-token-auths
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
Not sure if every test will pass, but this should be good to review. |
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
FYI, going to add another commit, had a new idea. |
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
Nevermind, not as good an idea as I thought. |
Actually, nevermind to my nevermind, it might be a good idea... |
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
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.
… token auths will be removed.
Closing in favor of #12208 which uses branch in piwik/piwik |
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.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Possible attack vectors
The only way I can think of for an attacker to gain access to a session, is for the attacker to:
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
Even more security
Some ideas for making Piwik even more secure (not necessarily related to this PR):
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.Fixes #6531