Skip to content

Commit

Permalink
[SEC] Potential information leak in redirect plugin
Browse files Browse the repository at this point in the history
  • Loading branch information
wilsonge committed Mar 21, 2015
1 parent 3010914 commit ebc9199
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 55 deletions.
1 change: 1 addition & 0 deletions administrator/language/en-GB/en-GB.plg_system_redirect.ini
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@

PLG_REDIRECT_XML_DESCRIPTION="The system redirect plugin enables the Joomla Redirect system to catch missing pages and redirect users."
PLG_SYSTEM_REDIRECT="System - Redirect"
PLG_SYSTEM_REDIRECT_ERROR_UPDATING_DATABASE="An error occurred while updating the database."
PLG_SYSTEM_REDIRECT_FIELD_COLLECT_URLS_DESC="This option controls the collection of URLs. This is useful to avoid unnecessary load on the database."
PLG_SYSTEM_REDIRECT_FIELD_COLLECT_URLS_LABEL="Collect URLs"
123 changes: 68 additions & 55 deletions plugins/system/redirect/redirect.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@
*/
class PlgSystemRedirect extends JPlugin
{
/**
* Affects constructor behavior. If true, language files will be loaded automatically.
*
* @var boolean
* @since 3.4
*/
protected $autoloadLanguage = false;

/**
* Constructor.
*
Expand Down Expand Up @@ -50,74 +58,80 @@ public static function handleError(&$error)
$app = JFactory::getApplication();

// Make sure the error is a 404 and we are not in the administrator.
if (!$app->isAdmin() and ($error->getCode() == 404))
if ($app->isAdmin() && $error->getCode() != 404)

This comment has been minimized.

Copy link
@Hoffi1

Hoffi1 Mar 23, 2015

Contributor

Was it the intention to change the logic here? The new line is not a full negation of the old one because and has not inverted to or.

This comment has been minimized.

Copy link
@mbabker

mbabker Mar 23, 2015

Contributor

You might be right. I honestly can't wrap my head around it at the moment but I've gone back and forth in my head 3 times over it now and am doubting myself on it.

This comment has been minimized.

Copy link
@Hoffi1

Hoffi1 Mar 23, 2015

Contributor

If the intention was to only negate the old line to
if (! (!$app->isAdmin() and ($error->getCode() == 404)))
it must be
if ($app->isAdmin() or ($error->getCode() != 404))
This will early exit on case of Admin or non-404 and only continue with the rest of the function in case of 404 on frontend which would match the comment above.

This comment has been minimized.

Copy link
@mbabker

mbabker Mar 23, 2015

Contributor

See #6564

{
// Get the full current URI.
$uri = JUri::getInstance();
$current = rawurldecode($uri->toString(array('scheme', 'host', 'port', 'path', 'query', 'fragment')));
// Render the error page.
JError::customErrorPage($error);
}

// Attempt to ignore idiots.
if ((strpos($current, 'mosConfig_') !== false) || (strpos($current, '=http://') !== false))
{
// Render the error page.
JError::customErrorPage($error);
}
// Get the full current URI.
$uri = JUri::getInstance();
$current = rawurldecode($uri->toString(array('scheme', 'host', 'port', 'path', 'query', 'fragment')));

// See if the current url exists in the database as a redirect.
$db = JFactory::getDbo();
// Attempt to ignore idiots.
if ((strpos($current, 'mosConfig_') !== false) || (strpos($current, '=http://') !== false))
{
// Render the error page.
JError::customErrorPage($error);
}

// See if the current url exists in the database as a redirect.
$db = JFactory::getDbo();
$query = $db->getQuery(true)
->select($db->quoteName(array('new_url', 'header')))
->select($db->quoteName('published'))
->from($db->quoteName('#__redirect_links'))
->where($db->quoteName('old_url') . ' = ' . $db->quote($current));
$db->setQuery($query, 0, 1);
$link = $db->loadObject();

// If no published redirect was found try with the server-relative URL
if (!$link or ($link->published != 1))
{
$currRel = rawurldecode($uri->toString(array('path', 'query', 'fragment')));
$query = $db->getQuery(true)
->select($db->quoteName(array('new_url', 'header')))
->select($db->quoteName('new_url'))
->select($db->quoteName('published'))
->from($db->quoteName('#__redirect_links'))
->where($db->quoteName('old_url') . ' = ' . $db->quote($current));
->where($db->quoteName('old_url') . ' = ' . $db->quote($currRel));
$db->setQuery($query, 0, 1);
$link = $db->loadObject();
}

// If no published redirect was found try with the server-relative URL
if (!$link or ($link->published != 1))
// If a redirect exists and is published, permanently redirect.
if ($link and ($link->published == 1))
{
// If no header is set use a 301 permanent redirect
if (!$link->header || JComponentHelper::getParams('com_redirect')->get('mode', 0) == false)
{
$currRel = rawurldecode($uri->toString(array('path', 'query', 'fragment')));
$query = $db->getQuery(true)
->select($db->quoteName('new_url'))
->select($db->quoteName('published'))
->from($db->quoteName('#__redirect_links'))
->where($db->quoteName('old_url') . ' = ' . $db->quote($currRel));
$db->setQuery($query, 0, 1);
$link = $db->loadObject();
$link->header = 301;
}

// If a redirect exists and is published, permanently redirect.
if ($link and ($link->published == 1))
// If we have a redirect in the 300 range use JApplicationWeb::redirect().
if ($link->header < 400 && $link->header >= 300)
{
// If no header is set use a 301 permanent redirect
if (!$link->header || JComponentHelper::getParams('com_redirect')->get('mode', 0) == false)
{
$link->header = 301;
}
$new_link = JUri::isInternal($link->new_url) ? JRoute::_($link->new_url) : $link->new_url;

// If we have a redirect in the 300 range use JApplicationWeb::redirect().
if ($link->header < 400 && $link->header >= 300)
$app->redirect($new_link, intval($link->header));
}
else
{
// Else rethrow the exeception with the new header and return
try
{
$new_link = JUri::isInternal($link->new_url) ? JRoute::_($link->new_url) : $link->new_url;

$app->redirect($new_link, intval($link->header));
throw new RuntimeException($error->getMessage(), $link->header, $error);
}
else
catch (Exception $e)
{
// Else rethrow the exeception with the new header and return
try
{
throw new RuntimeException($error->getMessage(), $link->header, $error);
}
catch (Exception $e)
{
$newError = $e;
}

JError::customErrorPage($newError);
$newError = $e;
}

JError::customErrorPage($newError);
}
else
}
else
{
try
{
$referer = empty($_SERVER['HTTP_REFERER']) ? '' : $_SERVER['HTTP_REFERER'];
$query = $db->getQuery(true)
Expand Down Expand Up @@ -167,13 +181,12 @@ public static function handleError(&$error)
$db->setQuery($query);
$db->execute();
}

// Render the error page.
JError::customErrorPage($error);
}
}
else
{
catch (RuntimeException $exception)
{
JError::customErrorPage(new Exception(JText::_('PLG_SYSTEM_REDIRECT_ERROR_UPDATING_DATABASE'), 404));
}

// Render the error page.
JError::customErrorPage($error);
}
Expand Down

0 comments on commit ebc9199

Please sign in to comment.