Skip to content

[Security] Show user account specific errors #56830

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
wants to merge 1 commit into from
Closed

[Security] Show user account specific errors #56830

wants to merge 1 commit into from

Conversation

core23
Copy link
Contributor

@core23 core23 commented May 20, 2024 •
edited by nicolas-grekas
Loading

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #50028
License MIT

According to the hideUserNotFoundExceptions flag, only UserNotFoundException should be hidden, but the implementation was also silencing account specific errors.

To fix the issue in a BC way, a new show_account_status_exceptions config key was introduced to show account exceptions.

@core23 core23 requested a review from chalasr as a code owner May 20, 2024 19:37
@carsonbot carsonbot added this to the 5.4 milestone May 20, 2024
@core23 core23 force-pushed the fix-auth-exception-handling branch 2 times, most recently from f32956f to 32d8da7 Compare May 20, 2024 19:50
@core23
Copy link
Contributor Author

core23 commented May 20, 2024

Looks like fabbot.io is showing a false positive code style issue. When applying the (unrelated) changes, the tests are failing

@chalasr
Copy link
Member

chalasr commented May 20, 2024

Hi Christian,

The fact that whether built-in AccountStatusException are caught or not depends on the hide_user_not_found flag although they are not UserNotFoundException might sound weird, yet it is on purpose.
We did this as part of a security fix 3 years ago. Reasoning was that the use case such exceptions serve is pretty much the same as UserNotFoundException ones regarding user enumeration.

So if we want to change this, that would definitely be a new feature.
Also it should not be about suddenly stopping to catch those, first because that would be too much of a BC break and also because it would go against the decision that led to the CVE mentioned above. What could be done is to rename the flag for the sake of clarity or split it in two. Given the traction on related issues, I would be fine doing so.

@chalasr chalasr mentioned this pull request May 20, 2024
Closed
@antfarmer
Copy link

I'm not clear; is this fix now to revert the changes to only mask UserNotFoundException's?

Personally, I'm fine with that because I do not see much benefit to masking locked accounts. The account would be locked and there would normally be nothing a legitimate or malicious user could do to unlock the account. Only to accommodate others on this, it may be worth considering having two settings, eg. hideUserNotFoundExceptions and hideAllAuthExceptions.

In my project I see there are at least 3 places where this logic is implemented:

  • security-http: Symfony\Component\Security\Http\Authentication\AuthenticatorManager: handleAuthenticationFailure
  • security-core: Symfony\Component\Security\Core\Authentication\Provider\UserAuthenticationProvider: authenticate
  • security-guard: Symfony\Component\Security\Guard\Firewall\GuardAuthenticationListener: executeGuardAuthenticator

Will these also be updated or are these others being phased out? Also, I assume this will be patched in 5.4+ going forward.

@carsonbot carsonbot changed the title Show user account specify errors [Security] Show user account specify errors May 22, 2024
@nicolas-grekas
Copy link
Member

How can we move on with this PR?

@core23 core23 force-pushed the fix-auth-exception-handling branch from 32d8da7 to 26852b9 Compare August 29, 2024 06:34
@OskarStark OskarStark changed the title [Security] Show user account specify errors [Security] Show user account specific errors Aug 29, 2024
@core23 core23 force-pushed the fix-auth-exception-handling branch 2 times, most recently from 44b6ad7 to 181ff20 Compare August 30, 2024 06:39
@core23 core23 force-pushed the fix-auth-exception-handling branch from 181ff20 to 33663d3 Compare August 30, 2024 06:42
@core23
Copy link
Contributor Author

core23 commented Aug 30, 2024

I updated the PR, so it should be BC now.

@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 7.2 Sep 17, 2024
nicolas-grekas
nicolas-grekas reviewed Sep 17, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should target 7.2 as this is a new feature.
@chalasr can you please have another look?
I don't like adding new options so if there's a way without, suggestions welcome :)

src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php
@@ -131,4 +134,17 @@ abstract protected function retrieveUser(string $username, UsernamePasswordToken
* @throws AuthenticationException if the credentials could not be validated
*/
abstract protected function checkAuthentication(UserInterface $user, UsernamePasswordToken $token);

private function isFilteredException(Exception $exception): bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we be more specific about the type-hint? (same below)

@core23
Copy link
Contributor Author

core23 commented Sep 18, 2024

Closing in favor of #58300

@core23 core23 closed this Sep 18, 2024
@chalasr chalasr mentioned this pull request Sep 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees
No one assigned
Labels
Bug Security Status: Needs Review
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.
6 participants
@core23 @chalasr @antfarmer @nicolas-grekas @OskarStark @carsonbot