CSRF prevention in plugins - Printable Version

CSRF prevention in plugins - Printable Version

+- MyBB Community Forums (https://community.mybb.com)
+-- Forum: Extensions (https://community.mybb.com/forum-201.html)
+--- Forum: Plugins (https://community.mybb.com/forum-73.html)
+---- Forum: Plugin Development (https://community.mybb.com/forum-68.html)
+---- Thread: CSRF prevention in plugins (/thread-26457.html)

Pages: 1 2

CSRF prevention in plugins - User 2877 - 2007-12-13

What would be the best method to prevent CSRF in a plugin? From my understanding this will work:

&& $mybb->request_method == "post"

Basically adding that to the IF ACTION lines

example:

if($mybb->input['action'] == "do_newthread" && $mybb->request_method == "post")

Supposedly mybb is safe from CSRF known attacks and it has added the request_method to the $mybb class but I only see it in some of the actions and not all of them. One example is the showthread.php page....that has a lot of actions and none contain the request_method. How is that page checking against CSRF?

Thank you for any help. I just want to make sure my plugins are as secure as I can make them.

RE: CSRF prevention in plugins - Dennis Tsang - 2007-12-13

showthread.php is mostly a retrieval-type page (as opposed to a page which changes things like newreply.php), and URL parameters are there so that links can easily be made to different pages, etc. This is a "get" request and the request method is obviously "get".

A "post" request is a submission of a <form method="post"> (or a Javascript doing so dynamically).

The request-method check in the "if" is mainly to prevent people using URLs to simulate a "post" operation. It isn't needed for the actions ones in showthread.php because on that page, access is intended to be from the URL.

RE: CSRF prevention in plugins - User 2877 - 2007-12-13

Thanks Dennis but not sure my question is answered. Does adding (&& $mybb->request_method == "post") to my actions IF statements help prevent CSRF attacks?

RE: CSRF prevention in plugins - Yumi - 2007-12-13

Checking request methods ain't a sure way of preventing CSRF attacks.
As DenisTT said, Javascript can perform dynamic submits.

In fact, MyBB is somewhat vulnerable to CSRF attacks at the moment, but Tikitiki said that 1.4 should have protection against it.

MyBB does check for a CSRF attempt for logging out - note the SID or LogoutID in the link?

MyPlaza checks SIDs when performing actions, ie:

if($session->sid != $mybb->input['sid']) die("Invalid request");

I believe the upcoming release of MyBB will also do something similar.

IIRC, IE6 allows AJAX requests to be performed on "non-owner" pages. I've never bothered to fully examine the nature, and haven't tested this on IE7. AJAX requests to "non-owner" pages is potentially quite dangerous, however, if cookies aren't sent, then it isn't too much of an issue.

An alternative is to check the HTTP referrer. However, this has inherit problems in itself, especially if you have multiple domain names for a single forum.

RE: CSRF prevention in plugins - Chris Boulton - 2007-12-13

Quote:Thanks Dennis but not sure my question is answered. Does adding (&& $mybb->request_method == "post") to my actions IF statements help prevent CSRF attacks?

Not really.

A CSRF attack can still be forged to perform a POST request.

The best solution would be similar to what ZiNga BuRgA mentioned, checking a session ID or some sort of other variable. I'd however advise against using the session ID and maybe (in 1.2.10) opt for something like $mybb->user['logoutkey'] instead. The reason I say this is because we've encountered a strange problem where on some servers where a session ID was being re-generated on every page load.

And yes, MyBB 1.4 has this built in - using a unique key we generate per user called the post key. In 1.4 you'll simply be able to call verify_post_key() and have the magic done for you, as long as you include the post key as a hidden field on the previous form.

Chris

RE: CSRF prevention in plugins - Yumi - 2007-12-13

Chris Boulton Wrote:I'd however advise against using the session ID and maybe (in 1.2.10) opt for something like $mybb->user['logoutkey'] instead. The reason I say this is because we've encountered a strange problem where on some servers where a session ID was being re-generated on every page load.

Ahh, that explains the change in 1.2.10.

I wonder why the SIDs would regenerate Confused

Anyways, thanks! Smile

RE: CSRF prevention in plugins - User 2877 - 2007-12-13

Any chance you can post the verify_post_key() function now? I could include it in my plugins and then just remove the function later. I have seen some form verification examples to prevent CSRF but if something is coming in 1.4 then I would prefer to minimize my code changes later.

Thanks for clear replies.

RE: CSRF prevention in plugins - Yumi - 2007-12-14


if(!function_exists('verify_post_key'))
{
 function verify_post_key() {
  global $mybb, $session;
  if(isset($mybb->user['logoutkey']))
  {
   if($mybb->user['logoutkey'] != $mybb->input['logoutkey']) die("invalid request");
  }
  elseif($mybb->input['sid'] != $session->sid) die("invalid request");
 }
}

I don't know MyBB 1.4's implementation, so this is unlikely going to be MyBB 1.2/1.4 transparent.

RE: CSRF prevention in plugins - User 2877 - 2007-12-14

At least if it's named the same I only need to remove the extra function for updating..that's easy.

Thanks. I will try that code.

RE: CSRF prevention in plugins - Chris Boulton - 2007-12-14

The following are the 1.4 functions:

/**
 * Generates a unique code for POST requests to prevent XSS/CSRF attacks
 *
 * @return string The generated code
 */
function generate_post_check()
{
	global $mybb;
	if($mybb->user['uid'])
	{
		return md5($mybb->user['loginkey'].$mybb->user['salt'].$mybb->user['regdate']);
	}
	// Guests get a special string
	else
	{
		return md5($mybb->config['database']['hostname'].$mybb->config['database']['username'].$mybb->settings['bburl']);
	}
}

/**
 * Verifies a POST check code is valid, if not shows an error (silently returns false on silent parameter)
 *
 * @param string The incoming POST check code
 * @param boolean Silent mode or not (silent mode will not show the error to the user but returns false)
 */
function verify_post_check($code, $silent=false)
{
	global $lang;
	if(generate_post_check() != $code)
	{
		if($silent == true)
		{
			return false;
		}
		else
		{
			error($lang->invalid_post_code);
		}
	}
	else
	{
		return true;
	}
}

MyBB assigns the post code for the current user in global.php as $mybb->post_code:

// Set our POST validation code here
$mybb->post_code = generate_post_check();

Chris