Yii2: User stays authorized despite auth key is changed

[laendyl]

While renewAuthStatus() method of yii\web\user is getting user id from session variable, it's not checking auth key. If auth key changes, the user still will be authorized while his id is stored in session.

Steps to reproduce this issue with Advanced application template:

1. Add $user->generateAuthKey(); in resetPassword method of frontend\models\ResetPasswordForm directly after $user->setPassword($this->password);
2. Login as any user in browser
3. Open new inconito tab
4. Login as same user in incognito tab
5. Change password in incognito tab
6. Check that you are still authorized in all tabs

[klimov-paul]

Why should 'auth key' affect current user session?
Its definition is pretty clear:

key that can be used to check the validity of a given identity ID

This means you an authorize user using pair of 'identity ID' and 'auth key'. This happens when 'enabledAutoLogin' is set to true, but it does not related to the running session.
Currently authenticated user simply unable to provide 'auth key' value - he does not have a source for it.

[undestroyer]

Ok, let's forget about authKey. Problem is about keeping user logged in after changing password. In my opinion this is security issue. For example, if someone stole my password and login with my parameters, the only thing I can do as user is change my password. But it makes no effect.
The most popular scenario when account is probably stolen - change your password. It is the most popular advice to keep account under control (Google, Apple, etc.)
Today apps written on yii2 are vulnerable because changing password makes sense only with clearing server sessions data. So, everyone can steal password, login, do whatever he want while session is alive. This is a problem, isn't if?

[cebe]

This is not a problem of Yii directly but your implementation. If you want to invalidate active sessions you can use an alternative session storage, e.g. db storage which allows you to add extra fields like user id which you can use to delete session entries for a specific user.

[undestroyer]

I am still disaggree with @cebe. Today users are helpless in face of account compromising. Why it shouldn't be fixed "out of box"?

[undestroyer]

I changed yii\web\User to make it work correct.

protected function renewAuthStatus()
    {
     // begining without changes

        if ($this->enableAutoLogin) {
            if ($this->getIsGuest()) {
                $this->loginByCookie();
            } elseif ($this->autoRenewCookie) {
                $authKey = json_decode(Yii::$app->getRequest()->getCookies()->getValue($this->identityCookie['name']),TRUE)[1];
                if($identity->validateAuthKey($authKey)) {
                    $this->renewIdentityCookie();
                    Yii::info("User $id succeeded authKey validation");
                } else {
                    $this->logout();
                    Yii::info("User $id failed authKey validation");
                }
            }
        }
    }

Also ResetPasswordForm generates new authKey for user in function resetPassword().

[cebe]

enableAutoLogin has nothing to do with the session. please try to formulate the problem more specifically.

[undestroyer]

Ok, I'll try explain problem in example.
Mr. U is a mysite.com user
Mr. X made fishing website mys1te.com
U didn't mentioned typo in URL and send username and password to X
U keeps work with original mysite.com
X has U's username and pass, so he logged in like U
U noticed unusual activity in his account
The only thing U can do is changing password and he did it
U suppose his account is safe now
But it is not. X is still online and can do everything like U because Yii trust logged in users if enableAutoLogin = true.

I propose to check auth_key every time and logout user if it is invalid. Changing password generates new auth_key for user. It will help keep accouns safe.

I didn't find better place to validate auth_key.

[kidol]

Looking into this, I don't understand why authKey is not stored in session. If attacker has NO open session, but tries to login with a stolen identity cookie that has different authKey, he will fail as expected. However, for session there is no such check.

Both login methods do:

$identity = $class::findIdentity($id);

However, only cookie login does:

if ($identity->validateAuthKey($authKey)) {

I think session should store both, id and authKey. Then get identity by id as usual but also check whether authKey is the same.

[dynasource]

@undestroyer has a big point here.

Is it useful to invalidate cookies on different media when a password has changed?

IMHO, it definately is.

• Firstly, when a password has changed, it is for a reason.
• Secondly, the disadvantage is minor. Users will understand to relogin on other media after a password change. This is quite expectable and with the new password in short memory, this wouldnt have to be.
• Thirdly, safety above all (or at lease provide an config option in User)

[samdark]

@kidol, @dynasource any idea on how to both keep BC and invalidate cookie for these cases? @undestroyer approach looks like it's close to what I'd do...

[dynasource]

the solution of @undestroyer is in the right direction. Forcing a logout removes the cookie. Would that break BC?

[kidol]

It has nothing to do with cookie. In his code if enableAutoLogin or autoRenewCookie is false, his fix falls through because attacker already has a valid session.

See proposal fix and test it in basic app demo: Login, then change authKey in User model. Now reload and you are properly logged out.

[dynasource]

I've approached this issue only from cookie perspective.

[samdark]

That's why I've asked ;) @undestroyer what do you think about @kidol approach to the problem?

[kidol]

I still don't understand the reasoning for the current code.

getAuthKey()
This is required if [[User::enableAutoLogin]] is enabled.

validateAuthKey($authKey)
This is required if [[User::enableAutoLogin]] is enabled.

So according to docs the authKey is only used for auto-login cookie. That being the case, the identity which holds this authKey, is loaded once the session is accessed. So current code is only logical if validateAuthKey is supposed to take additional time and is thus limited to autologin cookie.

Also @klimov-paul said:

Currently authenticated user simply unable to provide 'auth key' value - he does not have a source for it.

But identity which provides the authKey is always loaded??

I don't get it.

[dynasource]

Anti hack measures require:

• closing sessions on changed authkey
• autorenewal cookies reset

This requires continiously checking the DB for a changed auth key, going at cost of performance.
Therefore this would be something to support as an option or to support with a caching + flushing mechanism.

[kidol]

Everytime you access session, the identity (user record) is already loaded from db, isn't it? So with proposed fix the 2 things you mention are not needed because with wrong authKey it's not possible to initiate session.

[dynasource]

I recall a similar discussion on calling the DB for checking on isGuest, see https://github.com/yiisoft/yii2/issues/4464
. Havent had time to look a optimization & security on sessions yet, but calling the DB for every session is also not something I would prefer.

[kidol]

I don't do either. Just realized today that yii2 does this (never really used this feature).
Not sure if cache/invalidation is in scope of this auth stuff. After all it's easy to also delete some cache entry when manually creating new authKey after password change etc.

[undestroyer]

Why don't we put authKey in session data? It will help to check authKey when enableAutoLogin or autoRenewCookie is false.

My apologies, I didn't see @kidol commits. I think it is good fix to keep data in session, but I haven't tested it yet.

[dynasource]

the solution provided by @kidol looks good and is important for making Yii more robust against hacking of accounts. This usecase and investigation of \yii\webUser does show that this class has many opportunities for further improvement.

[laendyl]

That's exactly what I've meant when I created this issue:

1. Store authKey in session in switchIdentity method of yii\webUser as well as user id is storing
2. In renewAuthStatus method of yii\webUser, after getting identity ($identity = $class::findIdentity($id);) check authKey by $identity->validateAuthKey($authKey), where $authkey is stored in session authKey

In my project I extended yii\webUser and overrided these methods that way.

[DritonA]

@undestroyer your solution provided above will not work correctly. Try to uncheck "remember me" checkbox and try to log in. You are not able to log in without checking "remember me" button.

[Emtschikay]

Any news on this?

I have problems understanding the proposed solution from @kidol . It only works when renewAuthStatus() is called. To log out a cookie-theft on his next request, this only works when renewAuthStatus() is called on every request. Am I missing something or is this the case?

[WadeShuler]

A user should not be logged out after changing their password. This is a pain on the user experience side. The authKey should be regenerated, and a new cookie/session should be generated.

For both situations, either session or cookie, the authKey should be validated on every page load.

Every time you go to a new page, it pulls the user info from the database. So in the example someone mentioned above, regarding a phishing site and both victim and attacker are online at the same time, once the victim changes his password, and then attacker visits another page, he wouldn't get anywhere because the authKey has been changed.

I think the authKey should be generated on every login. This would be a single user account. There should be an option to disable the regeneration on login, for shared account uses.

How that would play out is like this:

Attacker logs into account (which generates new authKey). If victim had a valid session before, the attacker logging in would destroy his. Victim logs back in, thus killing the attackers session/cookie. It would be a battle back in forth. However, this wouldn't work in situations where multiple people legitimately share the same account. Thus the need for an option in the config to turn account sharing.

It would allow the victim to log back in, change his password, and the attacker would be completely locked out. This wouldn't even give the attacker the chance to change the password while he is in, because the next page load would lock him out!

This should be a feature in the config under the user component as enableAccountSharing with default as false.

In this situation, the developer could specifically enable account sharing if it was really necessary. In a shared account setup, you can't really prevent one of the logged in users from changing the password. In a shared account setup, the authKey shouldn't be regenerated on login, but only when the password has been changed (including after they complete the reset password form).

[undestroyer]

@WadeShuler interesting idea, but I'm confused. You call auto logout as

pain on the user experience side

I am partially agree with you. But some later you advice generate authKey on every login.

It would be a battle back in forth.

Don't you think that it is UX pain too? User log in, user click link, session is invalid and user logged out. Is there any easy way to tell user: "Hi, you are logged out because of some other guy who use your account. If you are good (valid) guy, please recover your password".

I would present upgraded idea how it should work:

1. User changes password
2. Backend logges out all user's connection
3. Backend logges in current user's connection

We avoid first UX pain, but I have no idea how implement this

[WadeShuler]

@undestroyer It is annoying for anyone on any site to have to re-login when they change their password.

Most users are not sharing their account with anyone else, so this would be a rare instance. When you generate a new authkey on login, you ensure any old logins are invalidated. The current yii design is not secure. It generates an auth key on signup, then the auth key never ever changes. If it was compromised, it would always be compromised. This with is a link between a secret key and the user identity. You want this to change frequently for security. So the best way would be to change it on login. So if the user is remembered, he can use the site for weeks and not have to login.. However, when he does have to login again, we should ensure any and all old sessions are destroyed. This is the perfect time to change the auth key!

There isn't really a reliable way to say "you have been logged out because someone else logged into your account", without freaking them out. You would have to log the IP address linked to the with key. If their IP changes, they would get freaked out and think they were hacked.

The best compromise would be to keep a log of past 10+ logins with date/time. This way if they ever thought something was up, they could look at the login history.

[rob006]

Most users are not sharing their account with anyone else, so this would be a rare instance.

Most of users uses multiple devices, so this will be incredible annoying - you could not have active session in your PC and phone at the same - each switch of device will force you to login again. This is definitively not rare situation.

[hi-rad]

Most of users uses multiple devices, so this will be incredible annoying - you could not have active session in your PC and phone at the same - each switch of device will force you to login again. This is definitively not rare situation.

A property can be set in config so that if that property is set to true, authKey checking will be done every time. This way if this is a rare situation in an application, you can set this property to be false and the user will not be logged out when authKey changes, and if security is important (and by security I mean, the problem mentioned above) this property can be set to true and authKey will be checked every time and the user will be logged out if authKey has been changed.

[rob006]

@hi-rad It does not change anything if authKey will be changed on every login. This is UX killer, and 99% of applications does not need this level of security. I already use changing password event for logout all active sessions and it works pretty well - user can still easily logout all sessions, but he doesn't need relogin every time when he switches device.

[hi-rad]

@rob006 I am not saying on every login. what I meant was, authKey changes on password change only.

I already use changing password event for logout all active sessions and it works pretty well - user can still easily logout all sessions, but he doesn't need relogin every time when he switches device.

Did you use Dbsession for this?

[rob006]

Did you use Dbsession for this?

No, I'm saving token in session and identity cookie, and compare them with current token from database on every page refresh.

[hi-rad]

@rob006 can you please guide me through your approach?

[rob006]

I have only code for Yii 1.1, but implementing this in Yii 2 should be similar.
In WebUser:

/**
 * {@inheritdoc}
 */
public function init() {
    parent::init();

    if ($this->getIsGuest()) {
        return;
    }

    // logout all active sessions after password change
    if ($this->getState('token') !== $this->getModel()->token) {
        $this->logout();
    }

    // immediately logout banned users
    if ($this->getModel()->status !== User::STATUS_ACTIVE) {
        $this->logout();
    }
}

/**
 * {@inheritdoc}
 */
protected function beforeLogin($id, $states, $fromCookie) {
    // Detect if password was changed during last login - if true, prevent login from outdated cookie
    if ($fromCookie && !$this->validateCookieToken($states)) {
        return false;
    }

    return parent::beforeLogin($id, $states, $fromCookie);
}

/**
 * @param array $states
 * @return bool
 * @throws CException
 */
private function validateCookieToken($states) {
    if (!isset($states['token']) || !isset($states['id'])) {
        return false;
    }

    $user = User::model()->findByPk($states['id']);
    if ($user === null) {
        return false;
    }

    return $states['token'] === $user->token;
}

In UserIdentity:

$this->setState('token', $user->token);
$this->setState('id', $user->id);

[hi-rad]

@rob006 thanks

[yurii-github]

not sure if I'm facing same issue.. but it is very strange. my use case is REST API via postman

let's make controller with optional auth by token, like

class SiteController extends Controller
{
    /**
     * @inheritdoc
     */
    public function behaviors()
    {
        return [
            'contentNegotiator' => [
                'class' => ContentNegotiator::className(),
                'formats' => [
                    'application/json' => Response::FORMAT_JSON,
                ],
            ],
            'authenticator' => [
                'class' => CompositeAuth::className(),
                'optional' => ['index'],
                'authMethods' => [
                    HttpBasicAuth::className(),
                ],
            ],
        ];
    }

    public function actionIndex()
    {
        $user = Yii::$app->user->identity;
        return $user;
    }

1. we do request with good token to 'actionIndex'. \Yii::$app->user->identity gets user X. Good
2. now, we do request with bad/unknown token. \Yii::$app->user->identity gets user X. NOT good.

very weird, any ideas?

regards

[yurii-github]

p.s.
for my case solution was to disable session

        'user' => [
            'identityClass' => 'app\models\User',
            'enableAutoLogin' => true,
            'enableSession' => false
        ],

your logic is next: (schema is a bit wrong, because after $identity=null you throw exception and it stays false)

[samdark]

Related to https://github.com/yiisoft/yii2/issues/8537

[borysenko]

Currently authenticated user simply unable to provide 'auth key' value - he does not have a source for it.

Написал компонент https://github.com/borysenko/yii2-user-component который сразу разлогиневывает юзера при смени authKey.
Запуск валидации authKey проходит в методе init() - это нам позволяет сделать валидацию authKey при каждом обновлении страницы.

class User extends \yii\web\User
{
    public function init()
    {
        $this->enableAutoLogin = true;
        $this->getIdentityAndDurationFromCookie();
        parent::init();
    }

 }

а в метод getIdentityAndDurationFromCookie() добавил удалении сессии

elseif (!$identity->validateAuthKey($authKey)) {
                    if(Yii::$app->session->has($this->idParam)) {
                        Yii::$app->session->remove($this->idParam);
                        $this->removeIdentityCookie();
                    }
                }