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 #12208

Merged
merged 30 commits into from Jul 26, 2018
Merged

Sessions with more security #12208

merged 30 commits into from Jul 26, 2018

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Oct 18, 2017

TODO

  • 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.
  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 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. Needs Review PRs that need a code review labels Oct 18, 2017
@diosmosis diosmosis added this to the 3.3.0 milestone Oct 18, 2017
{
$_SESSION[self::USER_NAME_SESSION_VAR_NAME] = $userName;
$_SESSION[self::SESSION_INFO_SESSION_VAR_NAME] = [
'sec' => $this->generateSessionSecret(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not quite sure but why do we need a session secret?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the whole auth hash thing can be done more simply w/ a session start time stored in the session. I think I was still thinking I couldn't rely on the session existing at this point. I'll change this.

$sessionFingerprint = new SessionFingerprint();

$cookie = $sessionCookieFactory->getCookie(false);
$cookie->set('id', $sessionFingerprint->getHash($passwordModifiedTime));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for not letting PHP set the session cookie? and generate the ID?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing code uses Cookie & overrides some parts of Zend_Session. Didn't want to just get rid of all of that in case there were reasons for its existence, but I can if that's desired.

$cookieName = Config::getInstance()->General['login_cookie_name'];
$_COOKIE[$cookieName] = $cookie->generateContentString();

$_SERVER['REMOTE_ADDR'] = $ip;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we checked IP's before for that? On mobile phones IP might often change...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Associating sessions to a single IP/user agent/whatever is new. Its purpose is to prevent session hijacking by making it harder for stolen session cookies to be used by attackers. If there's some other information that can be used to identify the device/client/user that can't be faked, could use that. Otherwise I can just remove this from the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I know the reason it is there for and user agent should be definitely used. I just know that some people had problems in the past using IPs for this when they change regularly. Not sure what else would be save to use (have limited internet access)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User Agent is good to have (now I think I know why I get logged out of some services when upgrading browser version 😄 )

IP address probably shouldn't be used as it could lead to frequent logouts in case people have rotating IPs every day or so.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here’s an idea: we can include a nonce in the cookie and change it on each request. If it doesn’t match what’s in the session, then the session is destroyed. If an attacker intercepts an in progress session, they will only be able to use it until the legit user sends another request. Not sure how well this will work with concurrent requests (though I think it might) and it doesn’t protect sessions that are idle for a long time (because of a large remember me value). What do you think?

@diosmosis
Copy link
Member Author

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.

// authenticate the request.
$sessionAuthCookieFactory = StaticContainer::get(SessionAuthCookieFactory::class);
if ($sessionAuthCookieFactory->isCookieInRequest()) {
Session::start();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This here will start a session for API requests but there should never be any session for API requests see eg https://github.com/piwik/piwik/blob/3.2.0/core/FrontController.php#L404-L409 . There is also a check to start the session only if Piwik is installed which may be good here as well.

Also is it actually needed to start the session twice (here and in dispatch see link above?)

Also: Ideally we close the session directly again if needed as it increases performance because PHP can then execute more requests for the same user in parallel and doesn't have to wait for the other request to finish.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be useful to allow session auth for API methods. We could, for example, remove the token auth from the JS code. (Note: we could also use something like a JWT, but since there’s no server side component, they’d be less secure than a session. Ie, a stolen JWT would be usable by an attacker.)

What about not starting the session, if there’s a token auth? So API requests will start a session ONLY if the session cookie is present.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sessions should not be used at all when using the API. Also not for authentication. I know we are not doing REST but being stateless is like one of the core concepts there and should be done here as well. It is like a best practice for APIs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re right, I was optimizing for the wrong metric.


public function isMatchingCurrentRequest()
{
$requestIp = IP::getIpFromHeader();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should be still removing the IP as the IP changes often, especially on mobile but also in general.

* the browser is closed or not.
* @return Cookie
*/
protected function getAuthCookie($rememberMe)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is in theory API and now removed. Can we somehow restore that method and make it work as before? Otherwise we would need to add this to the developer changelog as breaking change.

}

$tsPasswordModified = $user['ts_password_modified'];
if ($this->isSessionStartedBeforePasswordChange($sessionId, $tsPasswordModified)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that the user will be logged out afterwards. We would need to mention this in the UI maybe. Or would there be a way to not log the user out of the current session but all others? Or maybe this is already implemented and I did not notice it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be done on password change in the usersmanager API. The session changes there. All other sessions do not change, so are automatically invalidated.

}

$user = $userModel->getUser($userForSession);
if (empty($user)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it is a bit paranoid but lets maybe replace this if by

if (empty($user) || $user['login'] !== $userForSession) {

As the authentication is basically based on the getUser method... if there ever was a bug somehow in there, it could potentially log you in as someone else.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All sanity checks come from legitimate paranoia ;)

if ($this->shouldDestroySession) {
session_regenerate_id();
}
$sessionId->clear();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if the order is any important, only noticed in isSessionStartedBeforePasswordChange it calls ->clear first. Just checking, no clue if there is any difference.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll see if there's a difference or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a reason for this, added a comment in the code to clarify.

return true;
}

return $sessionStartTime < Date::factory($tsPasswordModified)->getTimestampUTC();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random thought, I wonder if it would be any smarter, or any difference if we stored in the userInfo in ts the time the password was last changed instead of the current timestamp. Then we could do something like if $userInfo['ts] !== $user['ts_password_modified']. Actually I think it wouldn't make any difference.

However, in SessionFingerprint::initialize() could we use 'ts' => Date::now()->getTimestampUTC() instead of 'ts' => time()? I know it is very much the same I think, but thinking if there is ever a bug in Date, or getTimestampUTC is changed, or something, it could cause random problems and thinking it may be better to re-use same logic to generate the time when comparing them later.

@@ -76,6 +76,7 @@ private function _checkUserHasNotChanged($user, $newPassword, $newEmail = null,
}
$userAfter = $this->api->getUser($user["login"]);
unset($userAfter['date_registered']);
unset($userAfter['ts_password_modified']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to hide this from general API output? I have a feeling like it better should be but not 100% sure or can't explain it. @mattab @diosmosis , eg in UsersManager\API::enrichUser?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be useful to show the user later (eg, your password hasn’t been updated in N days!). Doesn’t HAVE to be in API output for this to work though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would actually leave the ts_password_modified in the API output as some people may find value in this @diosmosis

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe only for super users then? I am personally finding this information quite private and wouldn't want an admin to see that kind of information.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will display it in API output if the user requested is the current user or the super user. Let me know if there are any issues w/ this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that seems to be the current case: https://github.com/piwik/piwik/blob/3.x-dev/plugins/UsersManager/API.php#L362 ? @mattab @tsteur am I wrong? lf there's something to change here, let me know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what needs changed is probably remove this line unset($userAfter['ts_password_modified']); from the test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that value is non-deterministic and would cause test failures if it's not removed during tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just check the value is there and don't test the actual value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i'll add that check, and I won't make any modifications to UsersManager/API since it already restricts user info to either the superuser or the currently logged in user.

@tsteur
Copy link
Member

tsteur commented Nov 2, 2017

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.

return true;
}

return $sessionStartTime < Date::factory($tsPasswordModified)->getTimestampUTC();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I currently got this error because $tsPasswordModified is null. In my case it did not execute the DB update because of something else and now I cannot access the UI anymore. May be good to ignore this check if forever reason that password modified is not set.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update probably has an old version, need to change it before it gets merged.

@diosmosis
Copy link
Member Author

@tsteur Applied feedback and rebased.


private function destroyCurrentSession(SessionFingerprint $sessionFingerprint)
{
// Note: can't use Session::destroy() since Zend prohibits starting a new session
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just wondering: why are we starting a new session after it was destroyed? I think this was not needed before this PR. Ideally destroying session as early as possible be great as it can increase performance quite a bit. (haven't really looked too deep into this, just wondering)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure why anymore, but Zend kept throwing an exception when I called Session::destroy(). It might not be necessary, but I'll have to switch it back and check to figure out where the second session start happened. (This is what you're asking about, right?)

Also note (and you might be aware of this, just mentioning in case it wasn't clear), the session is destroyed in the same place as it was before in FrontController. All of this session stuff happens before that, so it shouldn't have an effect on when it gets closed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a check and when I use Session::destroy(), this code results in an exception being thrown. The message I see is: The session was explicitly destroyed during this request, attempting to re-start is not allowed.

And the stacktrace is:

#0 /usr/src/piwik/libs/Zend/Session/Namespace.php(143): Zend_Session::start(true)
#1 /usr/src/piwik/core/Session/SessionNamespace.php(34): Zend_Session_Namespace->__construct('Login.login', false)
#2 /usr/src/piwik/core/Nonce.php(39): Piwik\Session\SessionNamespace->__construct('Login.login')
#3 /usr/src/piwik/plugins/Login/Controller.php(133): Piwik\Nonce::getNonce('Login.login')
#4 /usr/src/piwik/plugins/Login/Controller.php(115): Piwik\Plugins\Login\Controller->configureView(Object(Piwik\View))
#5 [internal function]: Piwik\Plugins\Login\Controller->login('You can't acces...')
#6 /usr/src/piwik/core/FrontController.php(517): call_user_func_array(Array, Array)
#7 /usr/src/piwik/core/FrontController.php(140): Piwik\FrontController->doDispatch('Login', 'login', Array)
#8 /usr/src/piwik/plugins/Login/Login.php(63): Piwik\FrontController->dispatch('Login', 'login', Array)
#9 [internal function]: Piwik\Plugins\Login\Login->noAccess(Object(Piwik\NoAccessException))
#10 /usr/src/piwik/core/EventDispatcher.php(141): call_user_func_array(Array, Array)
#11 /usr/src/piwik/core/Piwik.php(701): Piwik\EventDispatcher->postEvent('User.isNotAutho...', Array, true, NULL)
#12 /usr/src/piwik/core/FrontController.php(153): Piwik\Piwik::postEvent('User.isNotAutho...', Array, true)
#13 /usr/src/piwik/core/dispatch.php(34): Piwik\FrontController->dispatch()
#14 /usr/src/piwik/index.php(27): require_once('/usr/src/piwik/...')
#15 {main}

Looks like it's because of the nonce that's used on the Login form. Zend_Session will throw if the session was destroyed through Zend: https://github.com/piwik/piwik/blob/3.x-dev/libs/Zend/Session.php#L420

I'm not sure if there's a better way to do this than to regenerate the ID, but I can't think of a (quick) better idea right now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the comment w/ regard to the nonce issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what you're asking about, right?

Yes I was basically wondering why this was not a problem before, and why it is now as there seems to be a difference (unless we never noticed the problem before or so)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(missed your last comment) Before I don't think we were ever destroying the session (we also weren't really using it since it could disappear at any time). It's only a problem now that I wanted to destroy invalid sessions.

*/
public function __construct($usersManagerAPI = null, $authCookieName = null, $authCookieValidTime = null,
$authCookiePath = null)
public function __construct($usersManagerAPI = null)
Copy link
Member

@tsteur tsteur Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW: We are using a custom auth cookie name in another plugin. Is it still possible to use sessionInitializer with a custom authCookieName and the regular, configured authCookieName at the same time?

like in another plugin the user is authenticated using the regular cookie as normal. There may be a second authentication on top to access certain features which is currently solved by using like new SessionInitializer(null, 'myCustomAuth', '60')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the class is API, removing these parameters be like basically a breaking change. BTW we also set a custom $authCookieValidTime value there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The auth cookie is no longer used, so this would be a problem. I can restore the old SessionInitializer, deprecate it and create an entirely new one for the new logic. Is that acceptable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should work 👍 as long as everything still works

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.
@mattab mattab modified the milestones: 3.6.0, 3.5.0 Apr 9, 2018
@diosmosis
Copy link
Member Author

@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).

diosmosis added a commit to matomo-org/plugin-LoginHttpAuth that referenced this pull request Jun 14, 2018
diosmosis added a commit to matomo-org/plugin-LoginHttpAuth that referenced this pull request Jun 14, 2018
@@ -362,6 +360,9 @@ public function resetPasswordSuccess()
*/
public static function clearSession()
{
$sessionFingerprint = new Session\SessionFingerprint();
$sessionFingerprint->clear();

$authCookieName = Config::getInstance()->General['login_cookie_name'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still needed considering the cookie has been deprecated and is not set anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing cookie will still be there (ie, it won't automatically be unset for all users once this PR is merged), so we have to explicitly remove it somewhere (since it will have the token auth in it). I thought it was safer to just keep this. I can change it if you have a better idea, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping the original cookie will eventually expire?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're right, it should just expire. I'll remove this code.

public function beforeSessionStart()
{
// if this is a login request & form_rememberme was set, change the session cookie expire time before starting the session
$rememberMe = Common::getRequestVar('form_rememberme', false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this because all it takes be a URL parameter ?form_rememberme at any point and it could potentially be misused. Can we set it as part of the regular form like it used to happen where we also check an nonce?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Session::rememberMe has to be done before the session is started, or it will have no effect, and nonces are stored in the session so to read one we'd have to start a session. I'm not too sure how to make this better, but I'll think about it. If you have an idea would be interested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be always done before the session is started when setting the URL parameter in this case right? Ideally, it would be only set after a successful log in. Is there any way to move it kind of back to the original logic? I haven't thought of how it could work otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has to be set before the session is created, and before a login is attempted there is an active session (since the login form uses a nonce I think). It's super annoying, because Zend won't allow you to destroy & restart a session. Maybe there's some way to only execute this code if we're attempting a login, but I haven't thought of one yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "before session start" limitation is documented in the PHP function: https://secure.php.net/manual/en/function.session-set-cookie-params.php

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can check if the module = Login as well. If someone adds that to the URL, then they'll be taken to the login page. And I guess maybe check that if a session is already set the form_rememberme=1 param wouldn't change the expiry time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing it only when module=Login sounds good although it would maybe still have the affect that the session be active in the end for a longer time? Is it maybe possible to also only do it for $_POST requests? Would that help?

It has to be set before the session is created
from what I saw it is needed to be called before session is started, not created but I may misunderstand it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we check for module=Login, action=login, and only in POST. If someone adds form_rememberme to this request, it's basically just ticking the checkbox.

from what I saw it is needed to be called before session is started, not created but I may misunderstand it.

I don't think Zend differentiates between starting & creating a session. If there's no SESSIONID, it's created, otherwise just started. And there's always a session active even before login since the form uses a nonce (and Zend has a hardcoded exception that disallows creating a session after one is destroyed).

@@ -102,7 +102,7 @@ private function authenticateWithTokenOrHashToken($token, $login)

if (!empty($user['token_auth'])
// authenticate either with the token or the "hash token"
&& ((SessionInitializer::getHashTokenAuth($login, $user['token_auth']) === $token)
&& ((\Piwik\Session\SessionInitializer::getHashTokenAuth($login, $user['token_auth']) === $token)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure I understand it. I would have expected to only see the check $user['token_auth'] === $token here and no longer the old check? Is this to keep users still logged in who were logged in at time of update? If so I would be keen to log them out on update and remove this line now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I changed this code (phpstorm probably did). I can remove that check, shouldn't be an issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be good to remove it as shouldn't be needed anymore I would say and improves security.

public function initialize($userName, $time = null, $userAgent = null)
{
$_SESSION[self::USER_NAME_SESSION_VAR_NAME] = $userName;
$_SESSION[self::SESSION_INFO_SESSION_VAR_NAME] = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very random thought and very likely not an issue... AFAIK PHP uses some custom serialize format for sessions. Not sure if there are any known issues re security and whether users could potentially set any weird username (eg when disable_checks_usernames_attributes enabled) or user agent to somehow escape and make the result of the session a different username. Only trying to think a bit out of the box... I really don't think it is an issue but wanted to just mention it.

Was thinking to maybe sanitize username/useragent maybe but likely not needed and could maybe even have other security issues when doing that...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could maybe add an automated test w/ every symbol & some unicode characters (maybe w/ a null character in it) and see if the session user info is written/read correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could but not sure it helps much as even the combination of it may be relevant. Only wanted to mention it but not much of a concern AFAIK

// the session must be started before using the session authenticator,
// so we do it here, if this is not an API request.
if (SettingsPiwik::isPiwikInstalled()
&& ($module !== 'API' || ($action && $action !== 'index'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit unrelated but maybe could also directly exclude to start a session for the opt out iframe see #12540 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, the opt-out form uses a nonce & the nonce is stored in a session, so I think it's required for the moment (at least changing that would require a bit more work).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@tsteur
Copy link
Member

tsteur commented Jul 24, 2018

@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
Copy link
Member

mattab commented Aug 28, 2018

It's awesome to have this available now 🎉
@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 #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 #13325

Add a password strength requirement that encourages large passwords.

covered in #13070

InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
* Modifying "cookie authentication" to be more secure.

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.

* Invalidate existing sessions after user password change.

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.

* Set session.gc_maxlifetime to login_cookie_expire time so session data 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.

* In SessionInitializer, clear other cookie values so previously stored token auths will be removed.

* Make sure anonymous user is still default user whan authenticating.

* fixing test failures

* Remove hash checking in piwik_auth cookie.

piwik_auth cookie still required since it's presence indicates we should use SessionAuth instead of the normal authentication mechanism. Since there's always a session, even if you're not logged in, PIWIK_SESSID can't be used by itself to determine this.

* Make sure session auth doesnt break in edge case where ts_password_modified column does not exist.

* Clarify session destruction/invalidation logic in SessionAuth.

* Make UsersManagerTest slightly more comprehensive.

* Use Date::now()->getTimestampUTC() instead of time() in SessionFingerprint::initialize().

* Check getUser returns correct user info in SessionAuth for sanity.

* Add SessionInitializer::getAuthCookie() back since it is @api.

* Remove IP address from session auth info + check.

* Refactor session start changes so it is started in one place only.

* Remove SessionAuthCookieFactory & deprecate auth cookie INI config vars (still needed for SessionInitializer deprectaed method).

* Make sure user can still login if ts_password_modified column is not present in database.

* Rename ts_password_modified Update class.

* Update comment in SessionAuth to include why Piwik tries to create another session.

* Restore 3.x-dev SessionInitializer for BC (deprecated), move new SessionInitializer to core, add tests for both SessionInitializers.

* Change update to 3.5 version.

* Make sure normal auth implementation is used if sessionauth fails so anonymous user can be logged in.

* On logout clear session fingerprint so same session cannot be used to login.

* Change update name + bump version, and make sure Session::rememberMe() is called before session is started (otherwise it has no effect).

* Fixing tests.

* apply review fixes

* remove test
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. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants