Fixed Control flow error in XenForo_ControllerPublic_Forum

[PaulB]

Around line 105, the following problematic code exists in library/XenForo/ControllerPublic/Forum.php:

foreach ($threads AS $id => &$thread)
{
	$thread['permissions'] = XenForo_Permission::unserializePermissions($thread['node_permission_cache']);
	if (!$threadModel->canViewThreadAndContainer($thread, $thread, $null, $thread['permissions']))
	{
		unset($threads[$id]);
	}

	$thread = $threadModel->prepareThread($thread, $thread, $thread['permissions']);
	if (!empty($thread['lastPostInfo']['isIgnoring']) || $visitor->isIgnoring($thread['user_id']))
	{
		unset($threads[$id]);
	}
}
$threads = array_slice($threads, 0, $threadLimit, true);

The first unset statement should be followed by a continue statement. Note that this doesn't actually result in an information disclosure vulnerability due to the unintuitive behavior of references in PHP: when $threads[$id] is unset, $thread is converted from a reference to a value. prepareThread will still receive and return the original thread data, but as $thread is no longer a reference, the information will be discarded.

There's a minor performance impact here, of course, but my primary concern is that future versions of PHP and alternative implementations may not preserve this odd behavior: assigning to $thread may reverse the effects of the unset operation. Both vanilla PHP and HHVM seem to work the same way at the moment, but, given the history of alternative implementations like HHVM, adding a continue statement would be a much safer option.

Additionally, I don't believe the current logic was intentional; it happens to work, but I'm fairly certain the programmer who wrote it meant to skip the rest of the foreach block if the first condition is true, either with a continue statement or an else block. It's likely that this was originally an if-elseif construct that was broken apart to add the call to prepareThread.

Expected code:

foreach ($threads AS $id => &$thread)
{
	$thread['permissions'] = XenForo_Permission::unserializePermissions($thread['node_permission_cache']);
	if (!$threadModel->canViewThreadAndContainer($thread, $thread, $null, $thread['permissions']))
	{
		unset($threads[$id]);
		continue;
	}

	$thread = $threadModel->prepareThread($thread, $thread, $thread['permissions']);
	if (!empty($thread['lastPostInfo']['isIgnoring']) || $visitor->isIgnoring($thread['user_id']))
	{
		unset($threads[$id]);
	}
}
$threads = array_slice($threads, 0, $threadLimit, true);

or

foreach ($threads AS $id => &$thread)
{
	$thread['permissions'] = XenForo_Permission::unserializePermissions($thread['node_permission_cache']);
	if (!$threadModel->canViewThreadAndContainer($thread, $thread, $null, $thread['permissions']))
	{
		unset($threads[$id]);
	}
	else
	{
		$thread = $threadModel->prepareThread($thread, $thread, $thread['permissions']);
		if (!empty($thread['lastPostInfo']['isIgnoring']) || $visitor->isIgnoring($thread['user_id']))
		{
			unset($threads[$id]);
		}
	}
}
$threads = array_slice($threads, 0, $threadLimit, true);

 

[Mike]

I've fixed this now, thanks. A similar thing happened with profile posts.

I think this is more about clarity and not bothering with preparation. I can't see anything changing here in PHP as this would be a pretty major language/semantics change (if setting the value of a reference would recreate a previously broken reference) and if an alternative engine implementation didn't handle this the same way, that would be a clear bug in that engine.