Skip to content
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.

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

Add throttle for password reset form #12221

Merged
merged 1 commit into from
Dec 8, 2022

Conversation

snipe
Copy link
Owner

@snipe snipe commented Dec 7, 2022

I personally HATE this fix. It was brought up as a CVE issue because IF you have the wrong token, it tells you that the token is wrong. This changes it so that you get a success message even if the token it wrong and adds throttling to the reset password routes.

It's one of those "better security but shittier UX" compromises and I'm not sure I even want to take this PR. We'll discuss offline.

Signed-off-by: snipe <snipe@snipe.net>
@what-the-diff
Copy link

what-the-diff bot commented Dec 7, 2022

  • Added a new middleware to the ResetPasswordController
  • Changed an error message in reset() method of the same controller

Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

I mean, I get why you feel weird about it. But I think it's a really fair fix, that I can understand and read what you're doing here, and it makes sense, and it dots an 'i' (or crosses a 't'?) in terms of our security posture. While I agree with you that it's extraordinarily unlikely for someone to try and 'walk' password reset keys, making it actually impossible to detect which keys are valid or invalid makes sense to me. And for the actual automated use of this - someone saying 'reset my password', then minutes later clicking on the link - it's going to work fine.

So I think it's a targeted, surgical, strategic PR, and I think the product will be better for it.

Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

While I understand your reticence, I can't reasonably reject this. The throttle isn't going to hurt anyone, and the regular users are going to be able to reset their passwords without noticing anything.

@snipe snipe merged commit 775df0a into develop Dec 8, 2022
@snipe snipe deleted the fixes/throttles_reset_password_form branch December 8, 2022 22:56
@cmaraziaris
Copy link

Greetings,

I am Charalampos Maraziaris, who opened CVE-2022-44381, regarding username fingerprinting in the reset functionality of Snipe-IT.

Unfortunately, the fix introduced in this PR is incomplete. It's still possible to infer whether a specific active username exists in the database, by examining the page that the browser is redirected to in the following two conditions:

  • when the reset token exists and the username is valid
  • when the reset token exists and the username is invalid

Let me know if you need any more information regarding this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants