[RFC][Security] Deprecate UserInterface implementations not implementing EquatableInterface too

[ajgarlag]

The security system offers an interesting feature: when a request is authenticated from a token stored in session, the user instance is refreshed and compared to check if it has changed to prevent authentication with stale user data. The problem is how this comparison is made when the user object does not implements EquatableInterface.

In Symfony 3.4, current code compares the output of getUsername, getPassword and getSalt. If the user class implements AdvancedUserInterface, it compares the output of isAccountNonExpired, isAccountNonLocked, isAccountNonLocked and isEnabled too.

In Symfony 4.4 a new feature was added to check if the user roles have changed (#31177), but the implementation has a bug (#35941) that compares the refreshed user roles with token roles instead of comparing them with the stored user instance. This introduced a BC break fixed in #35944 and reverted in #37008. Now we have Symfony 4.4 with a known (and undocumented) BC break that unintentionally prevents to add to the token object any role not present in user object.

Usually, symfony apps use custom UserInterface implementations, so comparing two concrete object instances could be a tricky bussiness. To prevent this my proposition is:

Deprecate UserInterface implementations not implementing EquatableInterface too in Symfony 5.2, and make UserInterface to extend EquatableInterface in Symfony 6 discarding the code that compares UserInterface instances that do not implement EquatableInterface.

But this will not solve the problem with the BC break introduced in Symfony 4.4, so I propose to deprecate this in Symfony 3.4 too, but maybe with a different message pointing to the known BC break unintentionally introduced in Symfony 4.4.

Although Symfony 4.3 is only supported for security fixes, I think this deprecation should be included too to prevent problems when upgrading to Symfony 4.4.

Finally, if this is deprecated in Symfony 3.4 and Symfony 5.2, should this deprecation affects to Symfony 4.4, 5.0 and 5.1 too?

[stof]

We cannot add new deprecations in 3.4 or 4.4

[wouterj]

Usually, symfony apps use custom UserInterface implementations, so comparing two concrete object instances could be a tricky bussiness.

I think this is a very valid point and thus good to consider (for 5.2 - as pointed out by @stof).

Interestingly, this used to be the case in 2.0. It was changed in 2.1, see #2669 and #2927. If I understand correctly, the main arguments where:

• Having the equals() method appeared to be confusing for developers, as most where doing $user === $this, which is not the correct solution.
• Requiring all developers to implement this check can result in quite a bit more boilerplate code

[ajgarlag]

We cannot add new deprecations in 3.4 or 4.4

I know, but BC breaks should not be added in minor releases and 4.4 added one.

Adding this deprecation would not hurt any codebase, but upgrading from 3.4 or 4.3 to 4.4 could result in an broken app without any previous alert.

This is the easiest way I found to prevent this, but maybe we can find a better one.

[ajgarlag]

Interestingly, this used to be the case in 2.0. It was changed in 2.1, see #2669 and #2927. If I understand correctly, the main arguments where:

Ok, I see. I'm still asking me two things:

• Why original implementation does not compare getRoles?
• Why most UserInterface implementations do not serialize user roles?

Surely the answer to the second question is the first one.

* Having the `equals()` method appeared to be confusing for developers, as most where doing `$user === $this`, which is not the correct solution.

Maybe we can rename equals to hasUserChanged in 6.0

* Requiring all developers to implement this check can result in quite a bit more boilerplate code

[Clorr]

While I agree on the problem, I don't agree on the solution proposed. There is another problem to be solved ASAP and that remains since a long time (see: #24731 , or here , or here) that is that the FULL User object is serialized in the session! In most of the case, it is not a problem since the User is just an alone entity, but as soon as you add relations to it, the linked objects may be serialized along with the User Entity !

Yesterday, my SF 5.1.3 was trying to store 100Kb of serialized data in a blob limited to 64Kb (as I use PdoSessionHandler) and I was getting the infamous Warning: session_start(): Failed to decode session object. Session has been destroyed error. I circumvented it by implemeting the Serializable interface specified here (It's for version 4.0, but the doc of version 5.x is even less specific about this problem) but I think it's an ugly solution because I may want to serialize my users for other uses outside of the session.

I don't like the solution here of relying on EquatableInterface because it still relies on serializing blindly the full User object by default. And many sites are certainly storing big session files with potentially sensitive information without knowing. Imagine this:

class Customer implements UserInterface
{
...
    private $credit_cards;
...
    /**
     * @return Collection|CreditCards[]
     */
    public function getCreditCards(): Collection
    {
        return $this->credit_cards;
    }
...
}

You would end up with all credit cards data stored in session files! This would be a huge security breach!!! I hope credit card handling is not this simple, but this is to be considered for any kind of sensitive data...

My opinion for a solution to that problem would be to add a method that returns a unique identifier of the User based on freely choosable fields. Ex:

public function getUserSessionIdentifier() : string
{
    return $this->id . $this->email . $this->name; // Concatenation certainly not the best solution but I just make a quick example
}

This way, instead of storing the user in session, we just store this identifier (we also have to store the id separately to retrieve user in DB) and on reload, we just compare the stored identifier to the fresh loaded user from DB.

This also would enable developers to choose which fields they want to use to invalidate the session. If I don't include the name, modifying the name won't invalidate the session which is maybe not so important but solves also another problem I had today when trying to solve my session problem.

Actually, I had also modified my getUsername() method, and on some users, I wasn't able to connect just because the serialized version of my User was not returning the same value as my DB User, so it made some more white hairs there... The getUsername() method is really confusing there, because you think you can use it to format the displayed name of the user, but actually, it is used internally! Plus, it is named as if it was just a getter while it is not (it returns login, or email, depending on the parameters you gave to make:user)

So, again, I agree there is something to do here, but please, do consider all parameters of the problem before making a change!

Thanks

[thibaut-decherit]

@Clorr

The getUsername() method is really confusing there, because you think you can use it to format the displayed name of the user, but actually, it is used internally!

For that reason I don't use $user->username as a username but strictly as an internal identifier for SF:

• When the user is created I generate a random string stored in $user->username
• If the app requires a username (nowadays it usually does not, an email is enough) I store that in another propery (e.g. $user->businessUsername)

$user->username is also used in remember-me tokens (base64 encoded), 3 points come to mind:

• if the app allows the user to edit this property, doing so will invalidate all remember-me tokens of this user, which is not great UX wise, nor expected behaviour IMO
• you could take advantage of this behaviour by having a random string as $user->username and regenerating it if you need to revoke remember-me tokens
• $user->username could contain sensitive/private data (e.g. social security number), a remember-me token could then "leak" this data to an attacker able to retrieve it from the web browser or disk (e.g. public computer in a library), even if said token is no longer valid (e.g. expired, user changed his password...)

[Clorr]

Thanks for your reply @thibaut-decherit , I was just about to add a comment because the make:user commands generates a file where, above the getUsername() method is written : * A visual identifier that represents this user. . So clearly, you don't expect to have it used in the internals to check the validity of users...

I was further reading a resource pointed to me by @tarlepp , which reference another page about the same problem. So clearly this is known problem.

I clearly agree with both approaches, which use a separate class to contain the data stored in session, but still it is a patch and new users won't be aware of the problem until they face an error.

Really, there is something to be done here to decouple the Entity and the session object. Maybe not as I was suggesting with only a string as identifier, but maybe more with some Data Transfer Object, but anyway, it is a problem that is to be solved because it leaks entity data into session objects, and that is a big security threat...

[carsonbot]

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

[ajgarlag]

Yes, I'd like it

[carsonbot]

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

[Clorr]

Yes, please

[carsonbot]

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?