MyBB Community Forums

Since my original thread was moved to a place where I couldn't comment again, I would like to say that this is NOT a bogus bug report. Yes the fix allows to not have either but your v1212 code doesn't allow a person to logout with only the sid in the url which as far as I can tell is still the current standard but looks like a new logoutKey is going to be the new standard for MyBB.

So lets try this again and if you have a problem with my code fix at least leave it open to let me respond.

--------------------------------

file: member.php
line: 950
added: "$mybb->input['logoutKey'] && " to elseif statment
php version: 5.2.4

comments: this came to be after performing the proper upgrade from 1.2.9 to 1.2.12. I ran a "find updated" on the templates and found nothing about logoutKey. I see the board here is using it.

Original Code from v1212:

else if($mybb->input['action'] == "logout")
{
    $plugins->run_hooks("member_logout_start");

    if(!$mybb->user['uid'])
    {
        redirect("index.php", $lang->redirect_alreadyloggedout);
    }
    
    // Check session ID if we have one
    if($mybb->input['sid'] && $mybb->input['sid'] != $session->sid)
    {   
        error($lang->error_notloggedout);
    }
    // Otherwise, check logoutkey
    else if($mybb->input['logoutkey'] != $mybb->user['logoutkey'])
    {   
        error($lang->error_notloggedout);
    }

    my_unsetcookie("mybbuser");
    my_unsetcookie("sid");
    if($mybb->user['uid'])
    {
        $time = time();
        $lastvisit = array(
            "lastactive" => $time-900,
            "lastvisit" => $time,
            );
        $db->update_query(TABLE_PREFIX."users", $lastvisit, "uid='".$mybb->user['uid']."'");
        $db->delete_query(TABLE_PREFIX."sessions", "sid='".$session->sid."'");
    }
    $plugins->run_hooks("member_logout_end");

    redirect("index.php", $lang->redirect_loggedout);
}

Code Fix proposal:

else if($mybb->input['action'] == "logout")
{
    $plugins->run_hooks("member_logout_start");

    if(!$mybb->user['uid'])
    {
        redirect("index.php", $lang->redirect_alreadyloggedout);
    }
    
    // Check session ID or logoutKey if either one exists
    if( $mybb->input['sid'] || $mybb->input['logoutKey'] ) {
        // Check session ID if we have one
        if($mybb->input['sid'] && $mybb->input['sid'] != $session->sid)
        {
            error($lang->error_notloggedout);
        }
        // Otherwise, check logoutkey
        else if($mybb->input['logoutKey'] && $mybb->input['logoutkey'] != $mybb->user['logoutkey'])
        {
            error($lang->error_notloggedout);
        }
    }
    // Neither sid nor logoutKey provided
    else {
        error($lang->error_notloggedout);
    }

    my_unsetcookie("mybbuser");
    my_unsetcookie("sid");
    if($mybb->user['uid'])
    {
        $time = time();
        $lastvisit = array(
            "lastactive" => $time-900,
            "lastvisit" => $time,
            );
        $db->update_query(TABLE_PREFIX."users", $lastvisit, "uid='".$mybb->user['uid']."'");
        $db->delete_query(TABLE_PREFIX."sessions", "sid='".$session->sid."'");
    }
    $plugins->run_hooks("member_logout_end");

    redirect("index.php", $lang->redirect_loggedout);
}

Double check your templatesets. Chances are, you may have a complex theme which the updater didn't manage to replace.
Search your header_welcomeblock_member template for:

sid={$session->sid}

and replace it with

logoutkey={$mybb->user['logoutkey']}

The reason why your code doesn't work is because an attacker can easily perform a CSRF attack by not supplying a logoutkey or a sid (examine what your code does if none is supplied).

I can assure you this isn't a MyBB bug, so I'll move this to General Support, where you can reply if you need help.

Sure it is. The way the code is written is that it still allows for just the sid to be intended to be used. If we analyze the origional segment of code:

// Check session ID if we have one
if($mybb->input['sid'] && $mybb->input['sid'] != $session->sid)
{
    error($lang->error_notloggedout);
}
// Otherwise, check logoutkey
    else if($mybb->input['logoutkey'] != $mybb->user['logoutkey'])
{
    error($lang->error_notloggedout);
}

IF we have a sid in the url AND the sid in the url is NOT THE SAME as what we have in the db for you then display error.

ELSE if the logoutKey in the url is NOT THE SAME as what we have in the db fore you then display the error.

So where is the case for if the sid is the url AND is THE SAME as what is in the db (e.g. a valid logout request)?

If your intention was to replace sid with logoutKey then your code should be more like:

// Check logout key
if($mybb->input['logoutkey'] != $mybb->user['logoutkey'])
{
    error($lang->error_notloggedout);
}

WTF does this do, anyway?

 if($mybb->input['sid'] && $mybb->input['sid'] != $session->sid)

I don't think sid is valid twice.

StingReay Wrote:WTF does this do, anyway?

 if($mybb->input['sid'] && $mybb->input['sid'] != $session->sid)

I don't think sid is valid twice.

No, it checks that we have a sid (not 0 [false]) and then it checks that the sid is not equal to the current sid. When checking bool logic, the "== true", or "== false" part can be omitted.

MrDoom Wrote:

StingReay Wrote:WTF does this do, anyway?

 if($mybb->input['sid'] && $mybb->input['sid'] != $session->sid)

I don't think sid is valid twice.

No, it checks that we have a sid (not 0 [false]) and then it checks that the sid is not equal to the current sid. When checking bool logic, the "== true", or "== false" part can be omitted.

Ah, thanks. I'm a total PHP noob, so... I suck at this kind of thing.

That kind of omission is common to all C style languages.

Example (in C++):

#include <iostream>

int main()
{
	bool testOne = false;

	std::cout << "Test One is ";
	if(testOne)
		std::cout << "True";
	else
		std::cout << "False";
	std::cout << "\n" << std::endl;

	std::cout << "Test One is ";
	if(!testOne)
		std::cout << "False";
	else
		std::cout << "True";
	std::cout << std::endl;

	return 0;
}

And since int's can be cast to bool values, it also works on int's.

#include <iostream>

int main()
{
	int testInt = 0;
	std::cout << "Test Int is ";
	if(testInt)
		std::cout << "True";
	else
		std::cout << "False";
	std::cout << "\n" << std::endl;

	std::cout << "Test Int is ";
	if(!testInt)
		std::cout << "False";
	else
		std::cout << "True";
	std::cout << "\n" << std::endl;

	testInt = 3;
	std::cout << "Test Int is ";
	if(testInt)
		std::cout << "True";
	else
		std::cout << "False";
	std::cout << "\n" << std::endl;

	std::cout << "Test Int is ";
	if(!testInt)
		std::cout << "False";
	else
		std::cout << "True";
	std::cout << "\n" << std::endl;

	system("pause");

	return 0;
}

nobleclem Wrote:IF we have a sid in the url AND the sid in the url is NOT THE SAME as what we have in the db for you then display error.

ELSE if the logoutKey in the url is NOT THE SAME as what we have in the db fore you then display the error.

So where is the case for if the sid is the url AND is THE SAME as what is in the db (e.g. a valid logout request)?

If that is the case, it will fall past both the if, and if else, and will then execute the code below; no need for another else statement.

no what happens is the first if statement it fails because the input['sid'] == $session->sid so then it checks the else if in which input['logoutkey'] does not exist but user['logoutkey'] does. So it succeeds the elseif statement and executes the error statement and thus doesn't perform a logout action.

Good point, I misread what you said. You could try:

if($mybb->input['sid'] && $mybb->input['sid'] != $session->sid)
{
    error($lang->error_notloggedout);
}
else if(!$mybb->input['sid'] && $mybb->input['logoutkey'] != $mybb->user['logoutkey'])
{
    error($lang->error_notloggedout);
}

That way:
IF the SID evaluates to TRUE AND the SID IS NOT the current SID; error.
ELSE IF the SID evaluates to FALSE AND the logout key IS NOT the current logout key; error.

That way, either the SID, or the logout key will be valid. (The SID being sanity checked in the first statement, and the logout key being sanity checked in the second).

Now if the SID is valid, the "!$mybb->input['sid']" part of the "if else" statement will evaluate to FALSE and it won't error. Since it made it to the "if else" statement, and the first part failed, it means that the SID must be equal to the current SID since it failed the first "if" statement and if the SID itself was invalid, the "!$mybb->input['sid']" part of the "if else" statement would evaluate to TRUE and it would then check the logout key and if that also evaluated to TRUE the whole "if else" statement would be TRUE and an error would be shown.

Mind dead yet? Mine is...


This is probably an easier to read version of what I put above.

if(($mybb->input['sid'] == true) && ($mybb->input['sid'] != $session->sid))
{
    error($lang->error_notloggedout);
}
else if(($mybb->input['sid'] == false) && ($mybb->input['logoutkey'] != $mybb->user['logoutkey']))
{
    error($lang->error_notloggedout);
}