Cross-Site Request Forgery (CSRF) in bookstackapp/bookstack
Reported on
Nov 8th 2021
Description
Attacker is able to logout a user if a logged in user visits attacker website.
Impact
This vulnerability is capable of forging user to unintentional logout.
Test
Tested on Edge, firefox, chrome and safari.
Fix
You should use POST instead of GET.
To expand:
One way GET could be abused here is that a person (competitor perhaps:) placed an image tag with src="<your logout link>" ANYWHERE on the internet, and if a user of your site stumbles upon that page, he will be unknowingly logged out.
This is why it should be a POST with a @csrf token.
While this cannot harm a users account it can be a great annoyance and is considered a valid CSRF.
As noted in the description. "While this cannot harm a users account it can be a great annoyance and is aa valid CSRF." As a maintainer of a few Laravel projects myself this is a simple fix. You will see that laravel itself now uses a POST request for logout and not GET. See laravel-ui, laravel-breeze and laravel-jetstream for references. You can also find info on Laracasts forums and laravel issue tracker.
Thanks for reporting @hdvinnie. I did see that change in Laravel but didn't think it was worth implementing at that time since I'd kinda consider it as a breaking change in the event users have self implemented some funky flows, and since I couldn't see this having significant impact due to the targeted situational requirement for the low pay-off of causing a minor annoyance.
Granted that POST would be technically better, and be something that we should probably change in the future with some minor potential breaking change advisories, but I don't think this would be worth the creation of a CVE, probably more suited to be tracked as a GitHub issue to be honest. Unless I'm missing something and the risk to users is greater then I'm assessing?
@Dan Brown validating this would not create a public CVE. You could choose when you implement a fix to link it here to get the fix bounty for yourself and could make a security advisory on your repo via GitHub if you wanted but not required.
All validating it here would do is mark it that you acknowledge this is valid. It would then stay private between me and you and the sites admins until you publish and link a fix.
Once validated I could even make a PR if you wanted. Im not really seeing how it would be a breaking change though.
@hdvinnie Oh, All other reports I've validated here have led to CVE reports being generated. Thought that was just part of it.
In regards to breaking changes, yeah, It's likely not something to affect a lot of users but it wouldn't surprise me at this stage if users are using auth systems that propagate logout via GET. Plus I'll need to check potential SAML flows since we have the same for the SAML logout initiation. It something I'd like to advise about in release notes just in case.
For me, on this report, I'm more wondering if marking this as valid is correct for this platform. AFAICT this would not pose a security risk to users. Accepting this would cause it to show in hacktivity where it may get used as a model against other projects by those seeking bounties, rather than such things just being reported via GitHub etc... From my experience, handling reports here has a greater weight than if reported via that project's normal tracker. That's fine if there's a legitimate security concern to users but I'm not sure that can be validated here.
@Dan Brown
I understand and I have talked to admin here on this topic. It basically boils down to while this cannot harm a users account it can be a great annoyance and is a valid CSRF and thus therefor is a valid report on this platform.
CVE report will not be generated based on the nature of this type of CSRF. In the future I do believe they plan too implement a checkbox on the disclosure page when disclosing a CSRF to say if it's a logout CSRF or not and if so will be lower.
Again as of now this is what we have to work with and if you could validate it since it is technically a real issue that would be great. If not that is fine I guess. Either way lmk. Again all validating it here would do is mark it that you acknowledge this is valid. It would then stay private between me and you and the sites admins until you publish a patch and link it here if you so wished. You could never publish the fi here and then it would staff private and not be on the hacktivity.
Thanks.
Okay, have marked as valid now. Still not sure about it though. Please don't submit a PR for this one, would just lead to extra time consumed on the project's part, Have opened an issue to track on our side.
@admin please don't raise a CVE for this one. Also, the bounties and severities seem greatly inflated for this relative to other things that have been reported here. Would have queries ahead of validating but the values didn't show to me until validated.
Just for clarity on a few points here 👋
We only encourage reports to be marked valid if the maintainer believes the report to be a security issue.
Our CVE beta assigns CVEs to a select set of repositories that breach a certain popularity score through our platform. Today, we only publish CVEs once they have received quality vetting from me. I will go ahead and remove the CVE from this report.
Whilst we want to encourage a healthy back and forth between researchers and maintainers on this platform, the final decision is entirely up to the maintainer, and the platform does not take a position on the final say of the maintainer.
Let me know if either of you have any further thoughts or feedback here ♥️
@admin There is no security impact in logout CSRF attack by itself. There may be cases when this may be used in a multi-stage attack to first log someone out, then prompt them to log in on a spoofed page, thus stealing their credentials. Logout/Login CSRF attacks have been known for some time now and notably noticed and fixed by platforms like Laravel, Drupal, WP, etc.