8000 [Security] Multiple user providers on 1 authenticator can wrongfully re-authenticate · Issue #35045 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Multiple user providers on 1 authenticator can wrongfully re-authenticate #35045

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.

Sign up for GitHub

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

Jump to bottom
Closed
linaori opened this issue Dec 19, 2019 · 2 comments
Closed

[Security] Multiple user providers on 1 authenticator can wrongfully re-authenticate #35045

linaori opened this issue Dec 19, 2019 · 2 comments
Labels
Bug Security Status: Needs Review

Comments

@linaori
Copy link
Contributor
linaori commented Dec 19, 2019 •
edited
Loading

Symfony version(s) affected: 3.4, 4.3, 4.4, 5.0

Description
Report to clarify #29653, which was closed due to lack of activity.

There's no check whether a user provider supports a given user during the refreshUser() process, thus will attempt to refresh the user against every provider, rather than just the supported providers:

symfony/src/Symfony/Component/Security/Http/Firewall/ContextListener.php

Lines 185 to 196 in 4d064a2

foreach ($this->userProviders as $provider) {
if (!$provider instanceof UserProviderInterface) {
throw new \InvalidArgumentException(sprintf('User provider "%s" must implement "%s".', \get_class($provider), UserProviderInterface::class));
}
try {
$refreshedUser = $provider->refreshUser($user);
$newToken = clone $token;
$newToken->setUser($refreshedUser);
// tokens can be deauthenticated if the user has been changed.
if (!$newToken->isAuthenticated()) {

I've found a case where:

  • The first the correct provider is hit (WebsiteUserProvider), this de-authenticates the token because the user has changed
  • The second provider is hit (ApiUserProvider), this one does not support this user class, and immediately returns the User object
    public function refreshUser(UserInterface $user)
    {
        return $user;
    }

Because the ApiUserProvider gives the same user object back, the token is not de-authenticated after the setUser() is called.

$refreshedUser = $provider->refreshUser($user); // $user = WebsiteUser instance 1
$newToken = clone $token;
$newToken->setUser($refreshedUser); // $refreshedUser = WebsiteUser instance 1

When the token receives a setUser($user) where the instance of $user is identical to the instance it contains internally, the token's authentication state will not be changed:

symfony/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php

Lines 108 to 114 in 4d064a2

} else {
$changed = (string) $this->user !== (string) $user;
}
if ($changed) {
$this->setAuthenticated(false);
}

Because the second token will not hit the continue;, it will return the token and be considered "authenticated", thus the user won't be authenticated. This breaks the de-authentication flow and could result in accounts being logged in while they shouldn't.

Edit:

  • The isSupported() is not called in the refreshUser() for the ChainUserProvider, would have to be changed in a similar fashion.

How to reproduce
n/a - will have to make a failing test to prove this

Possible Solution
While refreshUser() indicates it can throw a UnsupportedUserException, this is not intuitive and is easy to miss, especially as supportsUser() exists. I think the solution of #29653 is the right direction. There was some feedback that had not been processed yet and is missing some tests.

Additional Context
Does the UnsupportedUserException even make sense at this point? It duplicates a feature in two different ways. Some core classes actually throw this exception, which would no longer be required if the supportsClass() would be used.
@chalasr
Copy link
Member
chalasr commented Dec 19, 2019

Works for me as a bugfix as we'll keep catching UnsupportedUserException anyways.

@linaori
Copy link
Contributor Author
linaori commented Dec 19, 2019

While looking further, the isSupported() is not called in the refreshUser() for the ChainUserProvider either, which means it would break there as well. Adding this to the main issue.

Some other classes will throw the UnsupportedUserException in certain scenarios, and I feel like all of those cases could eventually be refactored to all work with isSupported(), to reduce the amount of code duplication and complexity. The other option would be to deprecate the isSupported() and rely on the refreshUser() to throw this, though this feels more error prone, it's an implementation detail that's easily forgotten.

@linaori linaori mentioned this issue Dec 20, 2019
Merged
nicolas-grekas added a commit that referenced this issue Jan 21, 2020
@nicolas-grekas
bug #35065 [Security] Use supportsClass in addition to UnsupportedUse…
fb0be81 
…rException (linaori)

This PR was merged into the 3.4 branch.

Discussion
----------

[Security] Use supportsClass in addition to UnsupportedUserException

| Q             | A
| ------------- | ---
| Branch?       | 3.4+
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #35045
| License       | MIT
| Doc PR        | ~

This PR fixes the issue where user providers rely on just the UnsupportedUserException from `refreshUser()`, causing a flow where users are wrongfully re-authenticated.

There's one issue where `refreshUser()` can do far more sophisticated checks on the user class, which it will never reach if the class is not supported. As far as I know it was never intended to support instances that are rejected by `supportsClass()`, though people could've implemented this (by accident). So the question is more if we should add a BC layer for this; for example:

```php
try {
    $refreshedUser = $provider->refreshUser($user);
    $newToken = clone $token;
    $newToken->setUser($refreshedUser);

    if (!$provider->supportsClass($userClass)) {
        if ($this->shouldCheckSupportsClass) {
            continue;
        }
        // have to think of a proper deprecation here for 6.0
        @trigger_error('Provider %s does not support user class %s via supportsClass() while it does support it via refreshUser .. please set option X and fix %s::supportsUser() ', E_USER_DEPRECATED);
    }
```
This would prevent behavior from breaking but also means we can't fix this on anything less than 5.1.

Commits
-------

d3942cb Use supportsClass where possible
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Security Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nicolas-grekas @linaori @chalasr @carsonbot
0