Skip to content

Commit

Permalink
Merge pull request #157 from adaur/1.5-next
Browse files Browse the repository at this point in the history 
#1049 Add CSRF protection
  • Loading branch information
@franzliedke
franzliedke committed Oct 23, 2015
2 parents 2bded37 + 75e1e8a commit ea8039e
Show file tree
Hide file tree
Showing 12 changed files with 61 additions and 14 deletions.
10 changes: 6 additions & 4 deletions footer.php
View file
Expand Up @@ -29,6 +29,8 @@
{
echo "\t\t".'<div id="modcontrols" class="inbox">'."\n";

$token_url = '&amp;csrf_token='.pun_csrf_token();

if ($footer_style == 'viewforum')
{
echo "\t\t\t".'<dl>'."\n";
Expand All @@ -44,14 +46,14 @@
echo "\t\t\t\t".'<dd><span><a href="moderate.php?fid='.$forum_id.'&amp;move_topics='.$id.'">'.$lang_common['Move topic'].'</a></span></dd>'."\n";

if ($cur_topic['closed'] == '1')
echo "\t\t\t\t".'<dd><span><a href="moderate.php?fid='.$forum_id.'&amp;open='.$id.'">'.$lang_common['Open topic'].'</a></span></dd>'."\n";
echo "\t\t\t\t".'<dd><span><a href="moderate.php?fid='.$forum_id.'&amp;open='.$id.$token_url.'">'.$lang_common['Open topic'].'</a></span></dd>'."\n";
else
echo "\t\t\t\t".'<dd><span><a href="moderate.php?fid='.$forum_id.'&amp;close='.$id.'">'.$lang_common['Close topic'].'</a></span></dd>'."\n";
echo "\t\t\t\t".'<dd><span><a href="moderate.php?fid='.$forum_id.'&amp;close='.$id.$token_url.'">'.$lang_common['Close topic'].'</a></span></dd>'."\n";

if ($cur_topic['sticky'] == '1')
echo "\t\t\t\t".'<dd><span><a href="moderate.php?fid='.$forum_id.'&amp;unstick='.$id.'">'.$lang_common['Unstick topic'].'</a></span></dd>'."\n";
echo "\t\t\t\t".'<dd><span><a href="moderate.php?fid='.$forum_id.'&amp;unstick='.$id.$token_url.'">'.$lang_common['Unstick topic'].'</a></span></dd>'."\n";
else
echo "\t\t\t\t".'<dd><span><a href="moderate.php?fid='.$forum_id.'&amp;stick='.$id.'">'.$lang_common['Stick topic'].'</a></span></dd>'."\n";
echo "\t\t\t\t".'<dd><span><a href="moderate.php?fid='.$forum_id.'&amp;stick='.$id.$token_url.'">'.$lang_common['Stick topic'].'</a></span></dd>'."\n";

echo "\t\t\t".'</dl>'."\n";
}
Expand Down
2 changes: 1 addition & 1 deletion header.php
View file
Expand Up @@ -208,7 +208,7 @@ function process_form(the_form)
if ($pun_user['is_admmod'])
$links[] = '<li id="navadmin"'.((PUN_ACTIVE_PAGE == 'admin') ? ' class="isactive"' : '').'><a href="admin_index.php">'.$lang_common['Admin'].'</a></li>';

$links[] = '<li id="navlogout"><a href="login.php?action=out&amp;id='.$pun_user['id'].'&amp;csrf_token='.pun_hash($pun_user['id'].pun_hash(get_remote_address())).'">'.$lang_common['Logout'].'</a></li>';
$links[] = '<li id="navlogout"><a href="login.php?action=out&amp;id='.$pun_user['id'].'&amp;csrf_token='.pun_csrf_token().'">'.$lang_common['Logout'].'</a></li>';
}

// Are there any additional navlinks we should insert into the array before imploding it?
Expand Down
22 changes: 22 additions & 0 deletions include/functions.php
View file
Expand Up @@ -1141,6 +1141,28 @@ function pun_hash($str)
}


//
// Compute a random hash used against CSRF attacks
//
function pun_csrf_token()
{
global $pun_user;

return pun_hash($pun_user['id'].pun_hash(get_remote_address()));
}

//
// Check if the CSRF hash is correct
//
function check_csrf($token)
{
global $lang_common;

if (!isset($token) || $token != pun_csrf_token())
message($lang_common['Bad csrf hash'], false, '404 Not Found');
}


//
// Try to determine the correct remote IP-address
//
Expand Down
2 changes: 1 addition & 1 deletion index.php
View file
Expand Up @@ -60,7 +60,7 @@

// Display a "mark all as read" link
if (!$pun_user['is_guest'])
$forum_actions[] = '<a href="misc.php?action=markread">'.$lang_common['Mark all as read'].'</a>';
$forum_actions[] = '<a href="misc.php?action=markread&amp;csrf_token='.pun_csrf_token().'">'.$lang_common['Mark all as read'].'</a>';

$page_title = array(pun_htmlspecialchars($pun_config['o_board_title']));
define('PUN_ALLOW_INDEX', 1);
Expand Down
1 change: 1 addition & 0 deletions lang/English/common.php
View file
Expand Up @@ -16,6 +16,7 @@
'No view' => 'You do not have permission to view these forums.',
'No permission' => 'You do not have permission to access this page.',
'Bad referrer' => 'Bad HTTP_REFERER. You were referred to this page from an unauthorized source. If the problem persists please make sure that \'Base URL\' is correctly set in Admin/Options and that you are visiting the forum by navigating to that URL. More information regarding the referrer check can be found in the FluxBB documentation.',
'Bad csrf hash' => 'Bad CSRF hash. You were referred to this page from an unauthorized source.',
'No cookie' => 'You appear to have logged in successfully, however a cookie has not been set. Please check your settings and if applicable, enable cookies for this website.',
'Pun include extension' => 'Unable to process user include %s from template %s. "%s" files are not allowed',
'Pun include directory' => 'Unable to process user include %s from template %s. Directory traversal is not allowed',
Expand Down
4 changes: 3 additions & 1 deletion login.php
View file
Expand Up @@ -102,12 +102,14 @@

else if ($action == 'out')
{
if ($pun_user['is_guest'] || !isset($_GET['id']) || $_GET['id'] != $pun_user['id'] || !isset($_GET['csrf_token']) || $_GET['csrf_token'] != pun_hash($pun_user['id'].pun_hash(get_remote_address())))
if ($pun_user['is_guest'] || !isset($_GET['id']) || $_GET['id'] != $pun_user['id'])
{
header('Location: index.php');
exit;
}

check_csrf($_GET['csrf_token']);

// Remove user from "users online" list
$db->query('DELETE FROM '.$db->prefix.'online WHERE user_id='.$pun_user['id']) or error('Unable to delete from online list', __FILE__, __LINE__, $db->error());

Expand Down
8 changes: 8 additions & 0 deletions misc.php
View file
Expand Up @@ -51,6 +51,8 @@
if ($pun_user['is_guest'])
message($lang_common['No permission'], false, '403 Forbidden');

check_csrf($_GET['csrf_token']);

$db->query('UPDATE '.$db->prefix.'users SET last_visit='.$pun_user['logged'].' WHERE id='.$pun_user['id']) or error('Unable to update user last visit data', __FILE__, __LINE__, $db->error());

// Reset tracked topics
Expand All @@ -66,6 +68,8 @@
if ($pun_user['is_guest'])
message($lang_common['No permission'], false, '403 Forbidden');

check_csrf($_GET['csrf_token']);

$fid = isset($_GET['fid']) ? intval($_GET['fid']) : 0;
if ($fid < 1)
message($lang_common['Bad request'], false, '404 Not Found');
Expand Down Expand Up @@ -317,6 +321,8 @@
if ($pun_user['is_guest'])
message($lang_common['No permission'], false, '403 Forbidden');

check_csrf($_GET['csrf_token']);

$topic_id = isset($_GET['tid']) ? intval($_GET['tid']) : 0;
$forum_id = isset($_GET['fid']) ? intval($_GET['fid']) : 0;
if ($topic_id < 1 && $forum_id < 1)
Expand Down Expand Up @@ -367,6 +373,8 @@
if ($pun_user['is_guest'])
message($lang_common['No permission'], false, '403 Forbidden');

check_csrf($_GET['csrf_token']);

$topic_id = isset($_GET['tid']) ? intval($_GET['tid']) : 0;
$forum_id = isset($_GET['fid']) ? intval($_GET['fid']) : 0;
if ($topic_id < 1 && $forum_id < 1)
Expand Down
6 changes: 6 additions & 0 deletions moderate.php
View file
Expand Up @@ -754,6 +754,8 @@
{
confirm_referrer('viewtopic.php');

check_csrf($_GET['csrf_token']);

$topic_id = ($action) ? intval($_GET['close']) : intval($_GET['open']);
if ($topic_id < 1)
message($lang_common['Bad request'], false, '404 Not Found');
Expand All @@ -771,6 +773,8 @@
{
confirm_referrer('viewtopic.php');

check_csrf($_GET['csrf_token']);

$stick = intval($_GET['stick']);
if ($stick < 1)
message($lang_common['Bad request'], false, '404 Not Found');
Expand All @@ -786,6 +790,8 @@
{
confirm_referrer('viewtopic.php');

check_csrf($_GET['csrf_token']);

$unstick = intval($_GET['unstick']);
if ($unstick < 1)
message($lang_common['Bad request'], false, '404 Not Found');
Expand Down
4 changes: 3 additions & 1 deletion profile.php
View file
Expand Up @@ -449,6 +449,8 @@

confirm_referrer('profile.php');

check_csrf($_GET['csrf_token']);

delete_avatar($id);

redirect('profile.php?section=personality&amp;id='.$id, $lang_profile['Avatar deleted redirect']);
Expand Down Expand Up @@ -1533,7 +1535,7 @@

$user_avatar = generate_avatar_markup($id);
if ($user_avatar)
$avatar_field .= ' <span><a href="profile.php?action=delete_avatar&amp;id='.$id.'">'.$lang_profile['Delete avatar'].'</a></span>';
$avatar_field .= ' <span><a href="profile.php?action=delete_avatar&amp;id='.$id.'&amp;csrf_token='.pun_csrf_token().'">'.$lang_profile['Delete avatar'].'</a></span>';
else
$avatar_field = '<span><a href="profile.php?action=upload_avatar&amp;id='.$id.'">'.$lang_profile['Upload avatar'].'</a></span>';

Expand Down
2 changes: 1 addition & 1 deletion search.php
View file
Expand Up @@ -452,7 +452,7 @@

// If we're on the new posts search, display a "mark all as read" link
if (!$pun_user['is_guest'] && $search_type[0] == 'action' && $search_type[1] == 'show_new')
$forum_actions[] = '<a href="misc.php?action=markread">'.$lang_common['Mark all as read'].'</a>';
$forum_actions[] = '<a href="misc.php?action=markread&amp;csrf_token='.pun_csrf_token().'">'.$lang_common['Mark all as read'].'</a>';

// Fetch results to display
if (!empty($search_ids))
Expand Down
8 changes: 5 additions & 3 deletions viewforum.php
View file
Expand Up @@ -99,15 +99,17 @@

if (!$pun_user['is_guest'])
{
$token_url = '&amp;csrf_token='.pun_csrf_token();

if ($pun_config['o_forum_subscriptions'] == '1')
{
if ($cur_forum['is_subscribed'])
$forum_actions[] = '<span>'.$lang_forum['Is subscribed'].' - </span><a href="misc.php?action=unsubscribe&amp;fid='.$id.'">'.$lang_forum['Unsubscribe'].'</a>';
$forum_actions[] = '<span>'.$lang_forum['Is subscribed'].' - </span><a href="misc.php?action=unsubscribe&amp;fid='.$id.$token_url.'">'.$lang_forum['Unsubscribe'].'</a>';
else
$forum_actions[] = '<a href="misc.php?action=subscribe&amp;fid='.$id.'">'.$lang_forum['Subscribe'].'</a>';
$forum_actions[] = '<a href="misc.php?action=subscribe&amp;fid='.$id.$token_url.'">'.$lang_forum['Subscribe'].'</a>';
}

$forum_actions[] = '<a href="misc.php?action=markforumread&amp;fid='.$id.'">'.$lang_common['Mark forum read'].'</a>';
$forum_actions[] = '<a href="misc.php?action=markforumread&amp;fid='.$id.$token_url.'">'.$lang_common['Mark forum read'].'</a>';
}

$page_title = array(pun_htmlspecialchars($pun_config['o_board_title']), pun_htmlspecialchars($cur_forum['forum_name']));
Expand Down
6 changes: 4 additions & 2 deletions viewtopic.php
View file
Expand Up @@ -158,11 +158,13 @@

if (!$pun_user['is_guest'] && $pun_config['o_topic_subscriptions'] == '1')
{
$token_url = '&amp;csrf_token='.pun_csrf_token();

if ($cur_topic['is_subscribed'])
// I apologize for the variable naming here. It's a mix of subscription and action I guess :-)
$subscraction = "\t\t".'<p class="subscribelink clearb"><span>'.$lang_topic['Is subscribed'].' - </span><a href="misc.php?action=unsubscribe&amp;tid='.$id.'">'.$lang_topic['Unsubscribe'].'</a></p>'."\n";
$subscraction = "\t\t".'<p class="subscribelink clearb"><span>'.$lang_topic['Is subscribed'].' - </span><a href="misc.php?action=unsubscribe&amp;tid='.$id.$token_url.'">'.$lang_topic['Unsubscribe'].'</a></p>'."\n";
else
$subscraction = "\t\t".'<p class="subscribelink clearb"><a href="misc.php?action=subscribe&amp;tid='.$id.'">'.$lang_topic['Subscribe'].'</a></p>'."\n";
$subscraction = "\t\t".'<p class="subscribelink clearb"><a href="misc.php?action=subscribe&amp;tid='.$id.$token_url.'">'.$lang_topic['Subscribe'].'</a></p>'."\n";
}
else
$subscraction = '';
Expand Down

1 comment on commit ea8039e

@MioVisman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you created one more system for check if it was possible to finish confirm_referrer()?
See confirm_referrer() + csrf_hash() functions https://github.com/MioVisman/FluxBB_by_Visman/blob/master/include/functions.php#L1150 (no HTTP_REFERER, used page name (script), id and ip of user and salt of forum).

Please sign in to comment.