BookStackApp / BookStack

A platform to create documentation/wiki content built with PHP & Laravel
https://www.bookstackapp.com/
MIT License
15.49k stars 1.95k forks source link

Rate limit on password resets #4993

Closed KrisLowet closed 6 months ago

KrisLowet commented 6 months ago

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?

How long have you been using BookStack?

1 to 5 years

Additional context

No response

samadha56 commented 6 months ago

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.

adriantirado commented 6 months ago

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

ssddanbrown commented 6 months ago

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.

adriantirado commented 6 months ago

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

ssddanbrown commented 6 months ago

@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.

adriantirado commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

hi

ssddanbrown commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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.