Skip to content
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.

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

feat(security): Add a bruteforce protection backend base on memcache #39870

Merged
merged 7 commits into from
Aug 22, 2023
7 changes: 7 additions & 0 deletions apps/settings/lib/Controller/CheckSetupController.php
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
use OCP\IURLGenerator;
use OCP\Lock\ILockingProvider;
use OCP\Notification\IManager;
use OCP\Security\Bruteforce\IThrottler;
use OCP\Security\ISecureRandom;
use Psr\Log\LoggerInterface;

Expand Down Expand Up @@ -123,6 +124,8 @@
private $iniGetWrapper;
/** @var IDBConnection */
private $connection;
/** @var IThrottler */
private $throttler;
/** @var ITempManager */
private $tempManager;
/** @var IManager */
Expand All @@ -148,6 +151,7 @@
ISecureRandom $secureRandom,
IniGetWrapper $iniGetWrapper,
IDBConnection $connection,
IThrottler $throttler,
ITempManager $tempManager,
IManager $manager,
IAppManager $appManager,
Expand All @@ -162,6 +166,7 @@
$this->logger = $logger;
$this->dispatcher = $dispatcher;
$this->db = $db;
$this->throttler = $throttler;
Fixed Show fixed Hide fixed
$this->lockingProvider = $lockingProvider;
$this->dateTimeFormatter = $dateTimeFormatter;
$this->memoryInfo = $memoryInfo;
Expand Down Expand Up @@ -920,6 +925,8 @@
'cronInfo' => $this->getLastCronInfo(),
'cronErrors' => $this->getCronErrors(),
'isFairUseOfFreePushService' => $this->isFairUseOfFreePushService(),
'isBruteforceThrottled' => $this->throttler->getAttempts($this->request->getRemoteAddress()) !== 0,

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\Security\Bruteforce\IThrottler::getAttempts has been marked as deprecated
'bruteforceRemoteAddress' => $this->request->getRemoteAddress(),
'serverHasInternetConnectionProblems' => $this->hasInternetConnectivityProblems(),
'isMemcacheConfigured' => $this->isMemcacheConfigured(),
'memcacheDocs' => $this->urlGenerator->linkToDocs('admin-performance'),
Expand Down
8 changes: 8 additions & 0 deletions apps/settings/tests/Controller/CheckSetupControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
use OCP\IURLGenerator;
use OCP\Lock\ILockingProvider;
use OCP\Notification\IManager;
use OCP\Security\Bruteforce\IThrottler;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Http\Message\ResponseInterface;
use Psr\Log\LoggerInterface;
Expand Down Expand Up @@ -143,6 +144,7 @@ protected function setUp(): void {
$this->logger = $this->getMockBuilder(LoggerInterface::class)->getMock();
$this->db = $this->getMockBuilder(Connection::class)
->disableOriginalConstructor()->getMock();
$this->throttler = $this->createMock(IThrottler::class);
$this->lockingProvider = $this->getMockBuilder(ILockingProvider::class)->getMock();
$this->dateTimeFormatter = $this->getMockBuilder(IDateTimeFormatter::class)->getMock();
$this->memoryInfo = $this->getMockBuilder(MemoryInfo::class)
Expand Down Expand Up @@ -174,6 +176,7 @@ protected function setUp(): void {
$this->secureRandom,
$this->iniGetWrapper,
$this->connection,
$this->throttler,
$this->tempManager,
$this->notificationManager,
$this->appManager,
Expand Down Expand Up @@ -659,6 +662,8 @@ public function testCheck() {
'isFairUseOfFreePushService' => false,
'temporaryDirectoryWritable' => false,
\OCA\Settings\SetupChecks\LdapInvalidUuids::class => ['pass' => true, 'description' => 'Invalid UUIDs of LDAP users or groups have been found. Please review your "Override UUID detection" settings in the Expert part of the LDAP configuration and use "occ ldap:update-uuid" to update them.', 'severity' => 'warning'],
'isBruteforceThrottled' => false,
'bruteforceRemoteAddress' => '',
]
);
$this->assertEquals($expected, $this->checkSetupController->check());
Expand All @@ -683,6 +688,7 @@ public function testGetCurlVersion() {
$this->secureRandom,
$this->iniGetWrapper,
$this->connection,
$this->throttler,
$this->tempManager,
$this->notificationManager,
$this->appManager,
Expand Down Expand Up @@ -1410,6 +1416,7 @@ public function testIsMysqlUsedWithoutUTF8MB4(string $db, bool $useUTF8MB4, bool
$this->secureRandom,
$this->iniGetWrapper,
$this->connection,
$this->throttler,
$this->tempManager,
$this->notificationManager,
$this->appManager,
Expand Down Expand Up @@ -1464,6 +1471,7 @@ public function testIsEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed(string $m
$this->secureRandom,
$this->iniGetWrapper,
$this->connection,
$this->throttler,
$this->tempManager,
$this->notificationManager,
$this->appManager,
Expand Down
13 changes: 13 additions & 0 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,19 @@
*/
'auth.bruteforce.protection.enabled' => true,

/**
* Whether the bruteforce protection shipped with Nextcloud should be set to testing mode.
*
* In testing mode bruteforce attempts are still recorded, but the requests do
* not sleep/wait for the specified time. They will still abort with
* "429 Too Many Requests" when the maximum delay is reached.
* Enabling this is discouraged for security reasons
* and should only be done for debugging and on CI when running tests.
*
* Defaults to ``false``
*/
'auth.bruteforce.protection.testing' => false,

/**
* Whether the rate limit protection shipped with Nextcloud should be enabled or not.
*
Expand Down
82 changes: 82 additions & 0 deletions core/Command/Security/BruteforceAttempts.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
<?php

declare(strict_types=1);
/**
* @copyright Copyright (c) 2023 Joas Schilling <[email protected]>
*
* @author Joas Schilling <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/
namespace OC\Core\Command\Security;

use OC\Core\Command\Base;
use OCP\Security\Bruteforce\IThrottler;
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

class BruteforceAttempts extends Base {
public function __construct(
protected IThrottler $throttler,
) {
parent::__construct();
}

protected function configure(): void {
parent::configure();
$this
->setName('security:bruteforce:attempts')
->setDescription('resets bruteforce attempts for given IP address')
->addArgument(
'ipaddress',
InputArgument::REQUIRED,
'IP address for which the attempts are to be reset',
)
->addArgument(
'action',
InputArgument::OPTIONAL,
'Only count attempts for the given action',
)
;
}

protected function execute(InputInterface $input, OutputInterface $output): int {
$ip = $input->getArgument('ipaddress');

if (!filter_var($ip, FILTER_VALIDATE_IP)) {
$output->writeln('<error>"' . $ip . '" is not a valid IP address</error>');
return 1;
}

$data = [
'bypass-listed' => $this->throttler->isBypassListed($ip),
'attempts' => $this->throttler->getAttempts(

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\Security\Bruteforce\IThrottler::getAttempts has been marked as deprecated
$ip,
(string) $input->getArgument('action'),
),
'delay' => $this->throttler->getDelay(

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\Security\Bruteforce\IThrottler::getDelay has been marked as deprecated
$ip,
(string) $input->getArgument('action'),
),
];

$this->writeArrayInOutputFormat($input, $output, $data);

return 0;
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
<?php

declare(strict_types=1);
/**
* @copyright Copyright (c) 2020, Johannes Riedel ([email protected])
*
Expand All @@ -24,22 +26,22 @@
namespace OC\Core\Command\Security;

use OC\Core\Command\Base;
use OC\Security\Bruteforce\Throttler;
use OCP\Security\Bruteforce\IThrottler;
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

class ResetBruteforceAttempts extends Base {
class BruteforceResetAttempts extends Base {
public function __construct(
protected Throttler $throttler,
protected IThrottler $throttler,
) {
parent::__construct();
}

protected function configure() {
protected function configure(): void {
$this
->setName('security:bruteforce:reset')
->setDescription('resets bruteforce attemps for given IP address')
->setDescription('resets bruteforce attempts for given IP address')
->addArgument(
'ipaddress',
InputArgument::REQUIRED,
Expand Down
8 changes: 8 additions & 0 deletions core/js/setupchecks.js
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,14 @@
type: OC.SetupChecks.MESSAGE_TYPE_INFO
});
}
if (data.isBruteforceThrottled) {
messages.push({
msg: t('core', 'Your remote address was identified as "{remoteAddress}" and is bruteforce throttled at the moment slowing down the performance of various requests. If the remote address is not your address this can be an indication that a proxy is not configured correctly. Further information can be found in the {linkstart}documentation ↗{linkend}.', { remoteAddress: data.bruteforceRemoteAddress })
.replace('{linkstart}', '<a target="_blank" rel="noreferrer noopener" class="external" href="' + data.reverseProxyDocs + '">')
.replace('{linkend}', '</a>'),
type: OC.SetupChecks.MESSAGE_TYPE_ERROR
});
}
if(!data.hasWorkingFileLocking) {
messages.push({
msg: t('core', 'Transactional file locking is disabled, this might lead to issues with race conditions. Enable "filelocking.enabled" in config.php to avoid these problems. See the {linkstart}documentation ↗{linkend} for more information.')
Expand Down
61 changes: 61 additions & 0 deletions core/js/tests/specs/setupchecksSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,67 @@ describe('OC.SetupChecks tests', function() {
});
});

it('should return an error if the admin IP is bruteforce throttled', function(done) {
var async = OC.SetupChecks.checkSetup();

suite.server.requests[0].respond(
200,
{
'Content-Type': 'application/json',
},
JSON.stringify({
hasFileinfoInstalled: true,
isGetenvServerWorking: true,
isReadOnlyConfig: false,
wasEmailTestSuccessful: true,
hasWorkingFileLocking: true,
hasDBFileLocking: false,
hasValidTransactionIsolationLevel: true,
suggestedOverwriteCliURL: '',
isRandomnessSecure: true,
isFairUseOfFreePushService: true,
isBruteforceThrottled: true,
bruteforceRemoteAddress: '::1',
serverHasInternetConnectionProblems: false,
isMemcacheConfigured: true,
forwardedForHeadersWorking: true,
reverseProxyDocs: 'https://docs.nextcloud.com/foo/bar.html',
isCorrectMemcachedPHPModuleInstalled: true,
hasPassedCodeIntegrityCheck: true,
OpcacheSetupRecommendations: [],
isSettimelimitAvailable: true,
hasFreeTypeSupport: true,
missingIndexes: [],
missingPrimaryKeys: [],
missingColumns: [],
cronErrors: [],
cronInfo: {
diffInSeconds: 0
},
isMemoryLimitSufficient: true,
appDirsWithDifferentOwner: [],
isImagickEnabled: true,
areWebauthnExtensionsEnabled: true,
is64bit: true,
recommendedPHPModules: [],
pendingBigIntConversionColumns: [],
isMysqlUsedWithoutUTF8MB4: false,
isDefaultPhoneRegionSet: true,
isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true,
reverseProxyGeneratedURL: 'https://server',
temporaryDirectoryWritable: true,
})
);

async.done(function( data, s, x ){
expect(data).toEqual([{
msg: 'Your remote address was identified as "::1" and is bruteforce throttled at the moment slowing down the performance of various requests. If the remote address is not your address this can be an indication that a proxy is not configured correctly. Further information can be found in the <a target="_blank" rel="noreferrer noopener" class="external" href="https://docs.nextcloud.com/foo/bar.html">documentation ↗</a>.',
type: OC.SetupChecks.MESSAGE_TYPE_ERROR
}]);
done();
});
});

it('should return an error if set_time_limit is unavailable', function(done) {
var async = OC.SetupChecks.checkSetup();

Expand Down
3 changes: 2 additions & 1 deletion core/register_command.php
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,8 @@
$application->add(new OC\Core\Command\Security\ListCertificates(\OC::$server->getCertificateManager(), \OC::$server->getL10N('core')));
$application->add(new OC\Core\Command\Security\ImportCertificate(\OC::$server->getCertificateManager()));
$application->add(new OC\Core\Command\Security\RemoveCertificate(\OC::$server->getCertificateManager()));
$application->add(new OC\Core\Command\Security\ResetBruteforceAttempts(\OC::$server->getBruteForceThrottler()));
$application->add(\OC::$server->get(\OC\Core\Command\Security\BruteforceAttempts::class));
$application->add(\OC::$server->get(\OC\Core\Command\Security\BruteforceResetAttempts::class));
} else {
$application->add(\OC::$server->get(\OC\Core\Command\Maintenance\Install::class));
}
6 changes: 5 additions & 1 deletion lib/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -1021,10 +1021,11 @@
'OC\\Core\\Command\\Preview\\Generate' => $baseDir . '/core/Command/Preview/Generate.php',
'OC\\Core\\Command\\Preview\\Repair' => $baseDir . '/core/Command/Preview/Repair.php',
'OC\\Core\\Command\\Preview\\ResetRenderedTexts' => $baseDir . '/core/Command/Preview/ResetRenderedTexts.php',
'OC\\Core\\Command\\Security\\BruteforceAttempts' => $baseDir . '/core/Command/Security/BruteforceAttempts.php',
'OC\\Core\\Command\\Security\\BruteforceResetAttempts' => $baseDir . '/core/Command/Security/BruteforceResetAttempts.php',
'OC\\Core\\Command\\Security\\ImportCertificate' => $baseDir . '/core/Command/Security/ImportCertificate.php',
'OC\\Core\\Command\\Security\\ListCertificates' => $baseDir . '/core/Command/Security/ListCertificates.php',
'OC\\Core\\Command\\Security\\RemoveCertificate' => $baseDir . '/core/Command/Security/RemoveCertificate.php',
'OC\\Core\\Command\\Security\\ResetBruteforceAttempts' => $baseDir . '/core/Command/Security/ResetBruteforceAttempts.php',
'OC\\Core\\Command\\Status' => $baseDir . '/core/Command/Status.php',
'OC\\Core\\Command\\SystemTag\\Add' => $baseDir . '/core/Command/SystemTag/Add.php',
'OC\\Core\\Command\\SystemTag\\Delete' => $baseDir . '/core/Command/SystemTag/Delete.php',
Expand Down Expand Up @@ -1584,6 +1585,9 @@
'OC\\Search\\Result\\Image' => $baseDir . '/lib/private/Search/Result/Image.php',
'OC\\Search\\SearchComposer' => $baseDir . '/lib/private/Search/SearchComposer.php',
'OC\\Search\\SearchQuery' => $baseDir . '/lib/private/Search/SearchQuery.php',
'OC\\Security\\Bruteforce\\Backend\\DatabaseBackend' => $baseDir . '/lib/private/Security/Bruteforce/Backend/DatabaseBackend.php',
'OC\\Security\\Bruteforce\\Backend\\IBackend' => $baseDir . '/lib/private/Security/Bruteforce/Backend/IBackend.php',
'OC\\Security\\Bruteforce\\Backend\\MemoryCacheBackend' => $baseDir . '/lib/private/Security/Bruteforce/Backend/MemoryCacheBackend.php',
'OC\\Security\\Bruteforce\\Capabilities' => $baseDir . '/lib/private/Security/Bruteforce/Capabilities.php',
'OC\\Security\\Bruteforce\\CleanupJob' => $baseDir . '/lib/private/Security/Bruteforce/CleanupJob.php',
'OC\\Security\\Bruteforce\\Throttler' => $baseDir . '/lib/private/Security/Bruteforce/Throttler.php',
Expand Down
6 changes: 5 additions & 1 deletion lib/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -1054,10 +1054,11 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OC\\Core\\Command\\Preview\\Generate' => __DIR__ . '/../../..' . '/core/Command/Preview/Generate.php',
'OC\\Core\\Command\\Preview\\Repair' => __DIR__ . '/../../..' . '/core/Command/Preview/Repair.php',
'OC\\Core\\Command\\Preview\\ResetRenderedTexts' => __DIR__ . '/../../..' . '/core/Command/Preview/ResetRenderedTexts.php',
'OC\\Core\\Command\\Security\\BruteforceAttempts' => __DIR__ . '/../../..' . '/core/Command/Security/BruteforceAttempts.php',
'OC\\Core\\Command\\Security\\BruteforceResetAttempts' => __DIR__ . '/../../..' . '/core/Command/Security/BruteforceResetAttempts.php',
'OC\\Core\\Command\\Security\\ImportCertificate' => __DIR__ . '/../../..' . '/core/Command/Security/ImportCertificate.php',
'OC\\Core\\Command\\Security\\ListCertificates' => __DIR__ . '/../../..' . '/core/Command/Security/ListCertificates.php',
'OC\\Core\\Command\\Security\\RemoveCertificate' => __DIR__ . '/../../..' . '/core/Command/Security/RemoveCertificate.php',
'OC\\Core\\Command\\Security\\ResetBruteforceAttempts' => __DIR__ . '/../../..' . '/core/Command/Security/ResetBruteforceAttempts.php',
'OC\\Core\\Command\\Status' => __DIR__ . '/../../..' . '/core/Command/Status.php',
'OC\\Core\\Command\\SystemTag\\Add' => __DIR__ . '/../../..' . '/core/Command/SystemTag/Add.php',
'OC\\Core\\Command\\SystemTag\\Delete' => __DIR__ . '/../../..' . '/core/Command/SystemTag/Delete.php',
Expand Down Expand Up @@ -1617,6 +1618,9 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OC\\Search\\Result\\Image' => __DIR__ . '/../../..' . '/lib/private/Search/Result/Image.php',
'OC\\Search\\SearchComposer' => __DIR__ . '/../../..' . '/lib/private/Search/SearchComposer.php',
'OC\\Search\\SearchQuery' => __DIR__ . '/../../..' . '/lib/private/Search/SearchQuery.php',
'OC\\Security\\Bruteforce\\Backend\\DatabaseBackend' => __DIR__ . '/../../..' . '/lib/private/Security/Bruteforce/Backend/DatabaseBackend.php',
'OC\\Security\\Bruteforce\\Backend\\IBackend' => __DIR__ . '/../../..' . '/lib/private/Security/Bruteforce/Backend/IBackend.php',
'OC\\Security\\Bruteforce\\Backend\\MemoryCacheBackend' => __DIR__ . '/../../..' . '/lib/private/Security/Bruteforce/Backend/MemoryCacheBackend.php',
'OC\\Security\\Bruteforce\\Capabilities' => __DIR__ . '/../../..' . '/lib/private/Security/Bruteforce/Capabilities.php',
'OC\\Security\\Bruteforce\\CleanupJob' => __DIR__ . '/../../..' . '/lib/private/Security/Bruteforce/CleanupJob.php',
'OC\\Security\\Bruteforce\\Throttler' => __DIR__ . '/../../..' . '/lib/private/Security/Bruteforce/Throttler.php',
Expand Down
Loading
Loading