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
Sessions with more security #12208
Conversation
core/Session/SessionFingerprint.php
Outdated
{ | ||
$_SESSION[self::USER_NAME_SESSION_VAR_NAME] = $userName; | ||
$_SESSION[self::SESSION_INFO_SESSION_VAR_NAME] = [ | ||
'sec' => $this->generateSessionSecret(), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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. |
79a5098
to
98954ca
Compare
core/FrontController.php
Outdated
// authenticate the request. | ||
$sessionAuthCookieFactory = StaticContainer::get(SessionAuthCookieFactory::class); | ||
if ($sessionAuthCookieFactory->isCookieInRequest()) { | ||
Session::start(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
core/Session/SessionFingerprint.php
Outdated
|
||
public function isMatchingCurrentRequest() | ||
{ | ||
$requestIp = IP::getIpFromHeader(); |
There was a problem hiding this comment.
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.
plugins/Login/SessionInitializer.php
Outdated
* the browser is closed or not. | ||
* @return Cookie | ||
*/ | ||
protected function getAuthCookie($rememberMe) |
There was a problem hiding this comment.
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.
core/Session/SessionAuth.php
Outdated
} | ||
|
||
$tsPasswordModified = $user['ts_password_modified']; | ||
if ($this->isSessionStartedBeforePasswordChange($sessionId, $tsPasswordModified)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
core/Session/SessionAuth.php
Outdated
} | ||
|
||
$user = $userModel->getUser($userForSession); | ||
if (empty($user)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;)
core/Session/SessionAuth.php
Outdated
if ($this->shouldDestroySession) { | ||
session_regenerate_id(); | ||
} | ||
$sessionId->clear(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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']); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
98954ca
to
8eedf27
Compare
@tsteur Applied feedback and rebased. |
core/Session/SessionAuth.php
Outdated
|
||
private function destroyCurrentSession(SessionFingerprint $sessionFingerprint) | ||
{ | ||
// Note: can't use Session::destroy() since Zend prohibits starting a new session |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
plugins/Login/SessionInitializer.php
Outdated
*/ | ||
public function __construct($usersManagerAPI = null, $authCookieName = null, $authCookieValidTime = null, | ||
$authCookiePath = null) | ||
public function __construct($usersManagerAPI = null) |
There was a problem hiding this comment.
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')
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
… token auths will be removed.
…) is called before session is started (otherwise it has no effect).
@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). |
plugins/Login/Controller.php
Outdated
@@ -362,6 +360,9 @@ public function resetPasswordSuccess() | |||
*/ | |||
public static function clearSession() | |||
{ | |||
$sessionFingerprint = new Session\SessionFingerprint(); | |||
$sessionFingerprint->clear(); | |||
|
|||
$authCookieName = Config::getInstance()->General['login_cookie_name']; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
plugins/Login/Login.php
Outdated
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
plugins/Login/Auth.php
Outdated
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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] = [ |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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')) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@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 👍 |
It's awesome to have this available now 🎉
Created #13357
could you please create an issue to keep track of this security improvement?
covered in #13325
covered in #13070 |
* 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
TODO
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