Skip to content

Password recovery exemption: let only recovery requests through (B1, B3) #251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
codeling wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Password recovery exemption: let only recovery requests through (B1, B3) #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 62 additions & 5 deletions src/Extension/Bfstop.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,46 @@ private function isUnblockRequest($blockIds)
return $result;
}

/**
* The controller and the method of it which Joomla's component dispatcher
* runs for this request, in lower case, as it determines them
* (ComponentDispatcher::dispatch(), BaseController::execute()): "task" is
* either "controller.method" - anything after a second dot is ignored - or
* just the method, with the controller from the "controller" parameter;
* the method's case doesn't matter. Both are '' if there is no task (which
* shows a view). Checks of the task have to go by this, not by the raw
* parameter, or "task=user.LOGIN" or "task=user.login.x" would not be
* recognised as the login they are.
*
* @return string[] array(controller, method)
*/
private function requestedTask()
{
$task = strtolower($this->requestString('task'));
if ($task === '')
{
return array('', '');
}
if (strpos($task, '.') !== false)
{
$parts = explode('.', $task);
return array($parts[0], $parts[1]);
}
$controller = strtolower($this->requestString('controller'));
return array($controller === '' ? 'display' : $controller, $task);
}

// the tasks of com_users (controller.method) with which a user who forgot
// their password or username gets a new one: the forms of the reset and
// remind views post to them (index.php?task=reset.request, ...)
private const PasswordRecoveryTasks = array(
'reset.request',
'reset.confirm',
'reset.complete',
'remind.remind',
'user.remind',
);

/**
* Password reset / username reminder must stay reachable even for a
* blocked IP - otherwise a legitimate user who got blocked (and is told
Expand All @@ -670,17 +710,34 @@ private function isUnblockRequest($blockIds)
* that advice, turning the block itself into a denial-of-service
* against them (see OWASP Authentication Cheat Sheet guidance on
* lockouts).
*
* That is: showing the reset and remind views, and submitting their
* forms - nothing else. In particular not any task with view=reset: the
* task is what Joomla runs, the view is ignored then, so that would let a
* blocked IP log in (option=com_users&view=reset&task=user.login).
*/
private function isPasswordRecoveryRequest()
{
$option = $this->requestString('option');
$view = $this->requestString('view');
$result = (strcmp($option, 'com_users') == 0 &&
(strcmp($view, 'reset') == 0 || strcmp($view, 'remind') == 0));
if (strcasecmp($this->requestString('option'), 'com_users') != 0)
{
return false;
}
list($controller, $method) = $this->requestedTask();
if ($method === '' || ($controller === 'display' && $method === 'display'))
{
$view = strtolower($this->requestString('view'));
$result = ($view === 'reset' || $view === 'remind');
$what = 'view '.$view;
}
else
{
$result = in_array($controller.'.'.$method, self::PasswordRecoveryTasks, true);
$what = 'task '.$controller.'.'.$method;
}
if ($result)
{
$this->logger->log('Allowing blocked IP through to the '.
'password recovery view ('.$view.'), so a blocked '.
'password recovery ('.$what.'), so a blocked '.
'legitimate user can still recover their account',
Log::INFO);
}
Expand Down
37 changes: 37 additions & 0 deletions tests/Integration/BlockedRequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,46 @@ public function testPasswordRecoveryStaysReachable()
$this->block();
$this->assertNotBlocked('option=com_users&view=reset');
$this->assertNotBlocked('option=com_users&view=remind');
$this->assertNotBlocked('option=com_users&view=reset&layout=confirm');
$this->assertNotBlocked('option=com_users&view=reset&layout=complete');
$this->assertBlocked('option=com_users&view=login');
}

public function testPasswordRecoveryFormsCanBeSubmitted()
{
// the forms of the reset and remind views post a task, usually without
// view=reset (index.php?task=reset.request): without these the blocked
// user could see the form, but not get the mail
$this->configure();
$this->block();
$this->assertNotBlocked('option=com_users&task=reset.request');
$this->assertNotBlocked('option=com_users&task=reset.confirm');
$this->assertNotBlocked('option=com_users&task=reset.complete');
$this->assertNotBlocked('option=com_users&task=remind.remind');
$this->assertNotBlocked('option=com_users&task=user.remind');
$this->assertNotBlocked('option=com_users&view=reset&task=reset.request');
// spelled the way Joomla's dispatcher understands it as well
$this->assertNotBlocked('option=com_users&task=Reset.Request');
$this->assertNotBlocked('option=com_users&controller=reset&task=request');
}

public function testPasswordRecoveryViewIsNoPassForOtherTasks()
{
// Joomla runs the task, not the view: with view=reset in the request,
// a login (or anything else) must not get around the block
$this->configure();
$this->block();
$this->assertBlocked('option=com_users&view=reset&task=user.login');
$this->assertBlocked('option=com_users&view=remind&task=user.login');
$this->assertBlocked('option=com_users&view=reset&task=login&controller=user');
$this->assertBlocked('option=com_users&view=reset&task=user.LOGIN');
$this->assertBlocked('option=com_users&view=reset&task=registration.register');
$this->assertBlocked('option=com_users&view=reset&task[]=reset.request');
// "remind" is only a recovery task of the remind and user controllers
$this->assertBlocked('option=com_users&task=profile.remind');
$this->assertBlocked('option=com_content&task=reset.request');
}

public function testValidUnblockTokenGetsThrough()
{
$this->configure();
Expand Down
Loading