CWebUser->changeIdentity does not remove previous session #384

Closed
felix-sigac opened this Issue Feb 22, 2012 · 10 comments

5 participants

@felix-sigac
Contributor

By default, the class CWebUser doesn't clean previous session if the changeIdentity method is called:

protected function changeIdentity($id,$name,$states)
{
    Yii::app()->getSession()->regenerateID();
    $this->setId($id);
    $this->setName($name);
    $this->loadIdentityStates($states);
}

This makes a serious security flaw, and a performance issue, because a session file remains in the server but is not more used. This could be fixed adding a config variable to CWebUser, or by default, removing the previous session:

protected function changeIdentity($id,$name,$states)
{
    Yii::app()->getSession()->regenerateID($deletePreviouSession);
    // or Yii::app()->getSession()->regenerateID(true);
    $this->setId($id);
    $this->setName($name);
    $this->loadIdentityStates($states);
}
@samdark
Member
samdark commented Feb 22, 2012

Not sure about performance issue but regenerating session looks OK.

@samdark samdark was assigned Feb 22, 2012
@samdark
Member
samdark commented Feb 22, 2012

@qiangxue, @mdomba What's your opinion about this change?

@mdomba
Member
mdomba commented Feb 22, 2012

I'm not sure about security flaw here... session data is on the server.. not accessible by users so what is the possible flaw ?

The deleting of the data can have unexpected problems if the regenerate_id does not finish as expected... note this comment on php.net - http://hr.php.net/manual/en/function.session-regenerate-id.php#84242

@felix-sigac
Contributor

Ok. For me is a security is cause I'm also using the session file to enable, or not, access to a folder through .htacess. Due that the method is called in login and restoreFromCookie, also can be a solution to pass the parameter 'true' only in the login call, and not in the other.

@mdomba
Member
mdomba commented Feb 22, 2012

That would be a good compromise, so to "know" if changeIdentity is called from login() we would then need to add a 4th parameter...

that can be "..., $deleteOldSession = false", so to keep BC... and then from login set it to true.

@samdark
Member
samdark commented Feb 22, 2012

Well, you can call regenerateID after changeIdentity so no 4th parameter needed. The issue is about making it mandatory or not.

@mdomba
Member
mdomba commented Feb 22, 2012

@samdark sorry, I did not get your point.

the issue is if to remove the old session data or not... as commented on php.net if deleted it can give unexpected results... but as felix pointed it if not deleted can have problems too.

So the idea is to delete the previous session only when changeIdentity() is called from login()

webuser::login() calls changeIdentity() that in turn calls regenerateID(), so how would you apply your idea?

@samdark
Member
samdark commented Feb 22, 2012

Ah, I see. Yes, you're right.

I don't think we should change default behavior because of the issue you've linked so either 4th parameter of additional public property for CWebUser will be good in order for developer to be able to customize this.

Let's wait for @qiangxue opinion on this.

@felix-sigac
Contributor

3 months waiting :P Will it be fixed for 1.1.11?

@cebe
Member
cebe commented May 30, 2012

@felix-sigac you could start a pull request to make this go faster.
See https://github.com/yiisoft/yii/wiki/Git-workflow-for-Yii-contributors for details.

@qiangxue qiangxue closed this in 30b1d6e Jul 29, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment