Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed a bunch of XSRF issues #95

Merged
merged 3 commits into from Jan 8, 2014
Merged

Fixed a bunch of XSRF issues #95

merged 3 commits into from Jan 8, 2014

Conversation

@Aslai
Copy link
Contributor

Aslai commented Jan 8, 2014

With the way it currently is, I've successfully crafted XSRF links that can force anyone to post an arbitrary post on any forum, and can force non-admods to do a ton of things, specifically:

  • Modify any aspect of their profile such as timezone, signature, and even email if secure email updating is not enabled.
  • Post arbitrary posts anywhere on the forum.
  • Edit any post they've made.
  • Submit bogus reports.

It is also possible to carry out an XSRF attack using only XMLHttpRequest objects, so a potentially malicious site could do an attack completely unnoticed. One could even spam posts on a timer in order to post as many as possible without tripping the flood limiter.

These are some pretty serious issues and I'm surprised they made it this far. All I've done is made confirm_referrer() throw a bad HTTP_REFERER error if the valid script name is an empty string, and the host is different from the forum's base url. I then inserted confirm_referrer(''); wherever one was lacking. The reason why I didn't just do confirm_referrer('scriptname') is because some things (such as posting) are accessible from multiple scripts, so my approach is very flexible. However, it would be vulnerable if there's an unsafe redirect somewhere in the code, though there doesn't appear to be one currently.

post spoofing, report spoofing, and signature spoofing.)
@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 8, 2014

Some notes on coding style: please remove the spaces after the opening and before the closing parenthesis in the if statement. We don't do that.

You can then use these left-over spaces between the // and the first character of your comments ;)

misc.php Outdated
@@ -216,6 +216,10 @@
if (isset($_POST['form_sent']))
{

This comment has been minimized.

Copy link
@franzliedke

franzliedke Jan 8, 2014

Member

This line can be removed. Last one, I promise. =)

franzliedke added a commit that referenced this pull request Jan 8, 2014
#940: Fixed a bunch of XSRF issues
@franzliedke franzliedke merged commit 66490b1 into fluxbb:master Jan 8, 2014
@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 8, 2014

Thank you very much!

@Aslai

This comment has been minimized.

Copy link
Contributor Author

Aslai commented Jan 8, 2014

No problem. Just doing my part 👍

@Quy

This comment has been minimized.

Copy link
Member

Quy commented on delete.php in 94f1da4 Jan 9, 2014

Would it be okay to delete lines 54 & 55 since the confirm_referrer() check is done again in line 58?

This comment has been minimized.

Copy link
Member

franzliedke replied Jan 9, 2014

Will handle that. Fixing another bug at the moment.

This comment has been minimized.

Copy link
Member

Quy replied Jan 9, 2014

There are other locations with the same issue.

This comment has been minimized.

Copy link
Member

franzliedke replied Jan 9, 2014

Fixed now. There was another issue that essentially prevented any posts from being made on FluxBB.org. Not good.

@Aslai

This comment has been minimized.

Copy link
Contributor Author

Aslai commented Jan 9, 2014

While confirm_referrer('') will make sure the host is the same, it doesn't guarantee the script is the same. Just keep that in mind.

@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 9, 2014

I re-implemented confirm_referrer() in such a way that an array with valid referrers can be passed.

@Aslai

This comment has been minimized.

Copy link
Contributor Author

Aslai commented Jan 9, 2014

Yea, that makes more sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.