Ticket #1049 (fixed bug)
CSRF attack allows to stick, lock, etc.
- Created: 2015-09-27 19:34:32
- Reported by: Studio384
- Assigned to: adaur
- Milestone: 1.5.9
- Component: security
- Priority: highest
Earlier today, Sfideremo reported that my fork of FluxBB is vulnerable for a CSRF attack, after investigation, we've both concluded that FluxBB, too, is vulnerable for this issue.
In short, by simply adding a bit of code in a post, which will e.g. close or sticky a topic once a moderator visits the post. As long as the post exists, authorized users (mods and admins) can not change the state of the topics that are affected by that post.
It is possible to lock or reopen a topic, or to stick and unstick it. It seems like most - if not all - possible links that perform direct actions are vulnerable. Including other things like "Mark as read" and subscriptions. As this is very easy to perform, I consider this a "highest" priority issue.
History
Franz 2015-09-28 14:02:26
Interesting, thanks for the report. That's the problem with GET requests for critical actions.
We can probably fix this by requiring a token in the URL just like it's done for the logout link, right?
adaur 2015-09-30 12:29:11
I had a similar issue with my Like mod, I didn't thought this would happen in the core too 
adaur 2015-09-30 12:51:31
With a fresh installation, the topic is closed/locked/whatever only if an admin or a moderator visits the infected topic. It not as bad as it seems, but it should be fixed.
Studio384 2015-09-30 13:22:59
Yeah, but how small is the chance that no admin or moderator will ever visit these topics, usually, these people are there to check every topic, so it becomes very easy to exploit this.
Franz 2015-09-30 14:24:29
Yep. I'll be away until Wednesday, can we coordinate a release for, let's say, next Friday?
Franz 2015-10-01 14:05:45
Will do when I get back. I would have preferred to keep this private until next week, to be honest. But oh well, we can't change that anymore...
adaur 2015-10-01 15:24:34
It's not a critical one, and I'm not sure the repo is visited by the masses.
adaur 2015-10-09 08:32:53
The patch has been reviewed by quy, so it should be ok. I don't have the rights to push it to the repo, sorry.
"Delete avatar" is vulnerable when image tag is allowed in signatures and viewing a profile with the offending code.
Franz 2015-10-23 11:52:05
- Status changed from open to fixed.
Thanks a lot for the pull request!
Sorry for taking so long, I was really busy. I will take care of the release either Monday or Tuesday.
That said, can we agree that we should coordinate security-relevant things like this a bit better in the future, to avoid making them public prematurely:
- A PR should be sent very close before the release. We can review patches in advance.
- @Yannick: We could have coordinated releases (Luna & FluxBB).
Studio384 2015-10-28 18:16:52
I've already issued an update the Friday you proposed to do so, expecting you to do so that day as well.
Yep. I'll be away until Wednesday, can we coordinate a release for, let's say, next Friday?
Franz 2015-10-28 20:28:45
That's not exactly coordination, eh? 
Anyway: do we still need the confirm_referrer() calls where we added the check_csrf() calls? I'd wager no...
Studio384 2015-10-28 21:05:54
Well... you gave the date. I thought you would keep up with that yourself too. 
adaur 2015-10-30 11:21:46
I'd think confirm_referrer is a cheap a convenient protection.
Without it, I think we could call moderate.php?fid=1&stick=8282&csrf_token=XXX from anywhere and not viewtopic only.
Visman 2015-11-09 11:13:28
In my FluxBB version this vulnerability was closed in 2011 
https://fluxbb.org/development/core/tic … 92s3z54944
But you then didn't listen to me and now you don't listen to me.
Visman 2015-11-09 11:46:40
I speak, you don't want to listen to me.
It is easier for you to add new system of check, adding a new code, than to correct the old.
adaur 2015-11-09 12:59:04
We don't do it because it is easier, we do it because we think of all the users that would have to manually edit their old files. Sorry if you don't like it.
Visman 2015-11-10 08:02:04
$user_info[] = '<dd><span><a href="profile.php?action=promote&id='.$cur_post['poster_id'].'&pid='.$cur_post['id'].'">'.$lang_topic['Promote user'].'</a></span></dd>';