BookStack icon indicating copy to clipboard operation
BookStack copied to clipboard

Rate limit on password resets

Open KrisLowet opened this issue 9 months ago • 9 comments

Describe the feature you'd like

Currently, there is no rate limit for resetting passwords. Unlimited addresses can be entered. An idea is to limit the resetting password feature for IP's that requests new passwords for non-existing accounts.

Describe the benefits this would bring to existing BookStack users

More security due to blocking malafide requests.

Can the goal of this request already be achieved via other means?

A captcha method. Logging resets for unknown emails addresses (like logging failed access) to block the IP via failed2ban.

Have you searched for an existing open/closed issue?

  • [X] I have searched for existing issues and none cover my fundamental request

How long have you been using BookStack?

1 to 5 years

Additional context

No response

KrisLowet avatar May 11 '24 15:05 KrisLowet

I am interested in implementing this feature for the project. This feature will provide rate limiting for password reset requests based on IPs that submit excessive requests for non-existent accounts, thereby enhancing the overall security of the project.

To implement this feature, I will utilize technologies such as CAPTCHA and logging for suspicious IPs to effectively identify and prevent abnormal requests, thus mitigating potential malicious attacks.

I can provide further details regarding the implementation specifics and associated costs after conducting a more thorough review and understanding of the project requirements. I am ready to enhance the security of this project by incorporating this valuable feature.

Thank you for considering me for this opportunity, and I look forward to your response.

samadha56 avatar May 11 '24 19:05 samadha56

hi this vulnerability would be valid to be recognised as a cve

adriantirado avatar May 11 '24 19:05 adriantirado

Thanks for raising @KrisLowet, I agree that a rate limit would be ideal here, and also a random delay if not already there. I've assigned this to be something for our next patch release.

@samadha56 Thanks for the offer but please don't provide a PR. Your message indicates you may go down a more complex path than needed and this will be something I'd want to merge soon so is something I'd take on myself.

ssddanbrown avatar May 11 '24 21:05 ssddanbrown

hi this vulnerability would be valid to be recognised as a cve thanks you

adriantirado avatar May 11 '24 21:05 adriantirado

@adriantirado Okay, you repeated the same message as above. Or are you asking if we'll create a CVE?

I've always had trouble with the CVE process, and lack of control of CVEs. In the past, they been opened by bounty platforms or the reporter via their own CNA process. I did open some CVEs through GitHub before but I'm not fully keen on their process and don't really want deeper reliance on GitHub. Maybe something we need to spend time on to formalize, but I remember having trouble understanding CVE management when looking before.

ssddanbrown avatar May 11 '24 21:05 ssddanbrown

hi so you won't create a CVE for me? and how else could it be formalised, you can try it now, maybe it will work out well?

thanks

adriantirado avatar May 12 '24 05:05 adriantirado

hi is it possible to publish it myself from the cveform page, you have to give me the data that it asks me for the version for example, if you accept it I will give you the information that I need to complete the cveform

thanks you

adriantirado avatar May 12 '24 07:05 adriantirado

hi

adriantirado avatar May 13 '24 11:05 adriantirado

@adriantirado I've opened #5004 to better think through and formalise our security announcement & CVE process. I've opened this when thinking about CVEs from the above, and since I'm not sure in cases like this if such an issue/change is something within the scope of what we'd announce since to me this is improving/hardening security rather than fixing a vulnerability. Even adding IP-based rate-limiting, the same exploit could still be used but just at a higher cost/effort.

If you have experience in this area (especially in open source), feel free to add your comments in #5004 to help build that process.

ssddanbrown avatar May 13 '24 15:05 ssddanbrown

This has now been added within 69af9e0dbdefd8c6c951e8afbe2bba141d454beb, and will be part of the patch release to be soon release. This includes a 10 per minute per-IP request limit, in addition to a random pause period during request handling.

Thanks again @KrisLowet for raising this request.

ssddanbrown avatar May 21 '24 10:05 ssddanbrown

In the end I made v24.05.1 a security release, and was therefore announced as a security release. If it was the lack of these referenced rate limits alone, I would not have been too concerned (since we do have rate limits on known emails) but this led me to additional and more substantial concerns in how some other endpoints could be used without limit, and therefore I wrapped up these together into a security release.

I am also testing out requesting CVEs directly with mitre for this, and have requested a CVE ID.

ssddanbrown avatar May 21 '24 11:05 ssddanbrown