Remove possibility of backend username enumeration #5516
Conversation
|
@cosminbosutar Security fixes must be disclosed according to our security policy to be considered. |
LukeTowers
changed the title
Thank you @bennothommo for your mention. In future, I will follow the instructions you mention. |
| @@ -167,32 +168,28 @@ public function restore_onSubmit() | |||
| ]; | |||
|
|
|||
| $validation = Validator::make(post(), $rules); | |||
| if ($validation->fails()) { | |||
| throw new ValidationException($validation); | |||
| } | |||
There was a problem hiding this comment.
The validation failing should still come into play here shouldn't it? I don't see how that's leaking any information
There was a problem hiding this comment.
@LukeTowers A different statuscode can also leak information.
There was a problem hiding this comment.
Doesn't the ValidationException return a 406 http error code? Really any indication that could give away that an account exists should be avoided.
There was a problem hiding this comment.
@LukeTowers like @jacobdekeizer and @Klaasie mentioned, I consider it's better to not display the information that a username must have a specific length.
There was a problem hiding this comment.
@jacobdekeizer @cosminbosutar @Klaasie the validation exception has nothing to do with whether an account exists or not, it just an early indicator to the user that they screwed up their request. My main problem with it right now is that it's not necessarily accurate given some DB configurations result in a max length of string columns of 191; so on that basis I'm fine with removing the length part, but validating that the input is required period is probably still a good idea.
There was a problem hiding this comment.
@cosminbosutar any thoughts on the above?
There was a problem hiding this comment.
@LukeTowers I agree with you that validating that the input is received is a good idea.
I'll make an update to the PR in which I'll add the logic that validates that the input exists and the validation messages.
There was a problem hiding this comment.
@cosminbosutar just waiting for your update.
|
@bennothommo we can let this one through given that it's mostly a minor UX tweak rather than a major security issue; but yes, generally speaking all security reports should be submitted through the documented process. |
|
Thanks @cosminbosutar 🙏 This is a good find. I've implemented a fix in a different way than what is here. It will be available in the next update we've got coming soon. Cheers |
Security fix: Removed backend username enumeration
The current implementation of the reset password functionality allows backend username enumeration because. This is because the platform response for a user that exists is different from the response for a user that does not exist.
Example:
Proposed solution