Fork me on GitHub
Subscribe 4

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

Studio384 2015-09-27 19:41:05

  • Description changed. (Diff)

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?

Studio384 2015-09-28 16:09:12

Yes, that should fix this.

Comment edited 1 times (Diff)

Studio384 2015-09-28 16:10:17

  • Description changed. (Diff)

Studio384 2015-09-28 16:11:28

  • Description changed. (Diff)

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 hmm

https://www.wareziens.net/forum/topic-2 … age-1.html

Comment edited 1 times (Diff)

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.

Studio384 2015-10-08 10:54:59

Franz, any progress on this?

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.

Franz 2015-10-12 09:11:59

I'm back. I'll prepare the release in the next days.

quy 2015-10-20 13:49:55

"Delete avatar" is vulnerable when image tag is allowed in signatures and viewing a profile with the offending code.

adaur 2015-10-21 11:30:24

Fixed

Franz 2015-10-23 11:49:23

Commit ea8039e to fluxbb 1.5-next

Merge pull request #157 from adaur/1.5-next

#1049 Add CSRF protection

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

Franz 2015-10-23 11:52:20

  • Owner set to adaur.

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.

Franz wrote:

Yep. I'll be away until Wednesday, can we coordinate a release for, let's say, next Friday?

Comment edited 1 times (Diff)

Franz 2015-10-28 20:28:45

That's not exactly coordination, eh? wink

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

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.

Franz 2015-11-09 10:46:39

  • Visibility set to public.

Visman 2015-11-09 11:13:28

In my FluxBB version this vulnerability was closed in 2011 tongue
https://fluxbb.org/development/core/tic … 92s3z54944

But you then didn't listen to me and now you don't listen to me.

Comment edited 2 times (Diff, Diff 2)

adaur 2015-11-09 11:40:52

We didn't want to replace the whole referrer check, which is different.

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&amp;id='.$cur_post['poster_id'].'&amp;pid='.$cur_post['id'].'">'.$lang_topic['Promote user'].'</a></span></dd>';

Franz 2015-11-12 06:31:59

  • Description changed. (Diff)