Skip to content

Commit 82835ea

Browse files
nickvergessennickvergessen
authored
Merge pull request #39870 from nextcloud/perf/noid/memcache-bfp-backend
feat(security): Add a bruteforce protection backend base on memcache
2 parents f7e7034 + bed3ffb commit 82835ea

21 files changed

Lines changed: 882 additions & 196 deletions

File tree

‎apps/settings/lib/Controller/CheckSetupController.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@
9090
use OCP\IURLGenerator;
9191
use OCP\Lock\ILockingProvider;
9292
use OCP\Notification\IManager;
93+
use OCP\Security\Bruteforce\IThrottler;
9394
use OCP\Security\ISecureRandom;
9495
use Psr\Log\LoggerInterface;
9596

@@ -123,6 +124,8 @@ class CheckSetupController extends Controller {
123124
private $iniGetWrapper;
124125
/** @var IDBConnection */
125126
private $connection;
127+
/** @var IThrottler */
128+
private $throttler;
126129
/** @var ITempManager */
127130
private $tempManager;
128131
/** @var IManager */
@@ -148,6 +151,7 @@ public function __construct($AppName,
148151
ISecureRandom $secureRandom,
149152
IniGetWrapper $iniGetWrapper,
150153
IDBConnection $connection,
154+
IThrottler $throttler,
151155
ITempManager $tempManager,
152156
IManager $manager,
153157
IAppManager $appManager,
@@ -162,6 +166,7 @@ public function __construct($AppName,
162166
$this->logger = $logger;
163167
$this->dispatcher = $dispatcher;
164168
$this->db = $db;
169+
$this->throttler = $throttler;
165170
$this->lockingProvider = $lockingProvider;
166171
$this->dateTimeFormatter = $dateTimeFormatter;
167172
$this->memoryInfo = $memoryInfo;
@@ -920,6 +925,8 @@ public function check() {
920925
'cronInfo' => $this->getLastCronInfo(),
921926
'cronErrors' => $this->getCronErrors(),
922927
'isFairUseOfFreePushService' => $this->isFairUseOfFreePushService(),
928+
'isBruteforceThrottled' => $this->throttler->getAttempts($this->request->getRemoteAddress()) !== 0,
929+
'bruteforceRemoteAddress' => $this->request->getRemoteAddress(),
923930
'serverHasInternetConnectionProblems' => $this->hasInternetConnectivityProblems(),
924931
'isMemcacheConfigured' => $this->isMemcacheConfigured(),
925932
'memcacheDocs' => $this->urlGenerator->linkToDocs('admin-performance'),

‎apps/settings/tests/Controller/CheckSetupControllerTest.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@
5959
use OCP\IURLGenerator;
6060
use OCP\Lock\ILockingProvider;
6161
use OCP\Notification\IManager;
62+
use OCP\Security\Bruteforce\IThrottler;
6263
use PHPUnit\Framework\MockObject\MockObject;
6364
use Psr\Http\Message\ResponseInterface;
6465
use Psr\Log\LoggerInterface;
@@ -143,6 +144,7 @@ protected function setUp(): void {
143144
$this->logger = $this->getMockBuilder(LoggerInterface::class)->getMock();
144145
$this->db = $this->getMockBuilder(Connection::class)
145146
->disableOriginalConstructor()->getMock();
147+
$this->throttler = $this->createMock(IThrottler::class);
146148
$this->lockingProvider = $this->getMockBuilder(ILockingProvider::class)->getMock();
147149
$this->dateTimeFormatter = $this->getMockBuilder(IDateTimeFormatter::class)->getMock();
148150
$this->memoryInfo = $this->getMockBuilder(MemoryInfo::class)
@@ -174,6 +176,7 @@ protected function setUp(): void {
174176
$this->secureRandom,
175177
$this->iniGetWrapper,
176178
$this->connection,
179+
$this->throttler,
177180
$this->tempManager,
178181
$this->notificationManager,
179182
$this->appManager,
@@ -659,6 +662,8 @@ public function testCheck() {
659662
'isFairUseOfFreePushService' => false,
660663
'temporaryDirectoryWritable' => false,
661664
\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'],
665+
'isBruteforceThrottled' => false,
666+
'bruteforceRemoteAddress' => '',
662667
]
663668
);
664669
$this->assertEquals($expected, $this->checkSetupController->check());
@@ -683,6 +688,7 @@ public function testGetCurlVersion() {
683688
$this->secureRandom,
684689
$this->iniGetWrapper,
685690
$this->connection,
691+
$this->throttler,
686692
$this->tempManager,
687693
$this->notificationManager,
688694
$this->appManager,
@@ -1410,6 +1416,7 @@ public function testIsMysqlUsedWithoutUTF8MB4(string $db, bool $useUTF8MB4, bool
14101416
$this->secureRandom,
14111417
$this->iniGetWrapper,
14121418
$this->connection,
1419+
$this->throttler,
14131420
$this->tempManager,
14141421
$this->notificationManager,
14151422
$this->appManager,
@@ -1464,6 +1471,7 @@ public function testIsEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed(string $m
14641471
$this->secureRandom,
14651472
$this->iniGetWrapper,
14661473
$this->connection,
1474+
$this->throttler,
14671475
$this->tempManager,
14681476
$this->notificationManager,
14691477
$this->appManager,

‎config/config.sample.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,19 @@
352352
*/
353353
'auth.bruteforce.protection.enabled' => true,
354354

355+
/**
356+
* Whether the bruteforce protection shipped with Nextcloud should be set to testing mode.
357+
*
358+
* In testing mode bruteforce attempts are still recorded, but the requests do
359+
* not sleep/wait for the specified time. They will still abort with
360+
* "429 Too Many Requests" when the maximum delay is reached.
361+
* Enabling this is discouraged for security reasons
362+
* and should only be done for debugging and on CI when running tests.
363+
*
364+
* Defaults to ``false``
365+
*/
366+
'auth.bruteforce.protection.testing' => false,
367+
355368
/**
356369
* Whether the rate limit protection shipped with Nextcloud should be enabled or not.
357370
*

‎core/Command/Security/BruteforceAttempts.php‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* @copyright Copyright (c) 2023 Joas Schilling <coding@schilljs.com>
6+
*
7+
* @author Joas Schilling <coding@schilljs.com>
8+
*
9+
* @license GNU AGPL version 3 or any later version
10+
*
11+
* This program is free software: you can redistribute it and/or modify
12+
* it under the terms of the GNU Affero General Public License as
13+
* published by the Free Software Foundation, either version 3 of the
14+
* License, or (at your option) any later version.
15+
*
16+
* This program is distributed in the hope that it will be useful,
17+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
18+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
19+
* GNU Affero General Public License for more details.
20+
*
21+
* You should have received a copy of the GNU Affero General Public License
22+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
23+
*
24+
*/
25+
namespace OC\Core\Command\Security;
26+
27+
use OC\Core\Command\Base;
28+
use OCP\Security\Bruteforce\IThrottler;
29+
use Symfony\Component\Console\Input\InputArgument;
30+
use Symfony\Component\Console\Input\InputInterface;
31+
use Symfony\Component\Console\Output\OutputInterface;
32+
33+
class BruteforceAttempts extends Base {
34+
public function __construct(
35+
protected IThrottler $throttler,
36+
) {
37+
parent::__construct();
38+
}
39+
40+
protected function configure(): void {
41+
parent::configure();
42+
$this
43+
->setName('security:bruteforce:attempts')
44+
->setDescription('resets bruteforce attempts for given IP address')
45+
->addArgument(
46+
'ipaddress',
47+
InputArgument::REQUIRED,
48+
'IP address for which the attempts are to be reset',
49+
)
50+
->addArgument(
51+
'action',
52+
InputArgument::OPTIONAL,
53+
'Only count attempts for the given action',
54+
)
55+
;
56+
}
57+
58+
protected function execute(InputInterface $input, OutputInterface $output): int {
59+
$ip = $input->getArgument('ipaddress');
60+
61+
if (!filter_var($ip, FILTER_VALIDATE_IP)) {
62+
$output->writeln('<error>"' . $ip . '" is not a valid IP address</error>');
63+
return 1;
64+
}
65+
66+
$data = [
67+
'bypass-listed' => $this->throttler->isBypassListed($ip),
68+
'attempts' => $this->throttler->getAttempts(
69+
$ip,
70+
(string) $input->getArgument('action'),
71+
),
72+
'delay' => $this->throttler->getDelay(
73+
$ip,
74+
(string) $input->getArgument('action'),
75+
),
76+
];
77+
78+
$this->writeArrayInOutputFormat($input, $output, $data);
79+
80+
return 0;
81+
}
82+
}

core/Command/Security/ResetBruteforceAttempts.php renamed to core/Command/Security/BruteforceResetAttempts.php

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
<?php
2+
3+
declare(strict_types=1);
24
/**
35
* @copyright Copyright (c) 2020, Johannes Riedel (johannes@johannes-riedel.de)
46
*
@@ -24,22 +26,22 @@
2426
namespace OC\Core\Command\Security;
2527

2628
use OC\Core\Command\Base;
27-
use OC\Security\Bruteforce\Throttler;
29+
use OCP\Security\Bruteforce\IThrottler;
2830
use Symfony\Component\Console\Input\InputArgument;
2931
use Symfony\Component\Console\Input\InputInterface;
3032
use Symfony\Component\Console\Output\OutputInterface;
3133

32-
class ResetBruteforceAttempts extends Base {
34+
class BruteforceResetAttempts extends Base {
3335
public function __construct(
34-
protected Throttler $throttler,
36+
protected IThrottler $throttler,
3537
) {
3638
parent::__construct();
3739
}
3840

39-
protected function configure() {
41+
protected function configure(): void {
4042
$this
4143
->setName('security:bruteforce:reset')
42-
->setDescription('resets bruteforce attemps for given IP address')
44+
->setDescription('resets bruteforce attempts for given IP address')
4345
->addArgument(
4446
'ipaddress',
4547
InputArgument::REQUIRED,

‎core/js/setupchecks.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,14 @@
215215
type: OC.SetupChecks.MESSAGE_TYPE_INFO
216216
});
217217
}
218+
if (data.isBruteforceThrottled) {
219+
messages.push({
220+
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 })
221+
.replace('{linkstart}', '<a target="_blank" rel="noreferrer noopener" class="external" href="' + data.reverseProxyDocs + '">')
222+
.replace('{linkend}', '</a>'),
223+
type: OC.SetupChecks.MESSAGE_TYPE_ERROR
224+
});
225+
}
218226
if(!data.hasWorkingFileLocking) {
219227
messages.push({
220228
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.')

‎core/js/tests/specs/setupchecksSpec.js‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -814,6 +814,67 @@ describe('OC.SetupChecks tests', function() {
814814
});
815815
});
816816

817+
it('should return an error if the admin IP is bruteforce throttled', function(done) {
818+
var async = OC.SetupChecks.checkSetup();
819+
820+
suite.server.requests[0].respond(
821+
200,
822+
{
823+
'Content-Type': 'application/json',
824+
},
825+
JSON.stringify({
826+
hasFileinfoInstalled: true,
827+
isGetenvServerWorking: true,
828+
isReadOnlyConfig: false,
829+
wasEmailTestSuccessful: true,
830+
hasWorkingFileLocking: true,
831+
hasDBFileLocking: false,
832+
hasValidTransactionIsolationLevel: true,
833+
suggestedOverwriteCliURL: '',
834+
isRandomnessSecure: true,
835+
isFairUseOfFreePushService: true,
836+
isBruteforceThrottled: true,
837+
bruteforceRemoteAddress: '::1',
838+
serverHasInternetConnectionProblems: false,
839+
isMemcacheConfigured: true,
840+
forwardedForHeadersWorking: true,
841+
reverseProxyDocs: 'https://docs.nextcloud.com/foo/bar.html',
842+
isCorrectMemcachedPHPModuleInstalled: true,
843+
hasPassedCodeIntegrityCheck: true,
844+
OpcacheSetupRecommendations: [],
845+
isSettimelimitAvailable: true,
846+
hasFreeTypeSupport: true,
847+
missingIndexes: [],
848+
missingPrimaryKeys: [],
849+
missingColumns: [],
850+
cronErrors: [],
851+
cronInfo: {
852+
diffInSeconds: 0
853+
},
854+
isMemoryLimitSufficient: true,
855+
appDirsWithDifferentOwner: [],
856+
isImagickEnabled: true,
857+
areWebauthnExtensionsEnabled: true,
858+
is64bit: true,
859+
recommendedPHPModules: [],
860+
pendingBigIntConversionColumns: [],
861+
isMysqlUsedWithoutUTF8MB4: false,
862+
isDefaultPhoneRegionSet: true,
863+
isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true,
864+
reverseProxyGeneratedURL: 'https://server',
865+
temporaryDirectoryWritable: true,
866+
})
867+
);
868+
869+
async.done(function( data, s, x ){
870+
expect(data).toEqual([{
871+
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>.',
872+
type: OC.SetupChecks.MESSAGE_TYPE_ERROR
873+
}]);
874+
done();
875+
});
876+
});
877+
817878
it('should return an error if set_time_limit is unavailable', function(done) {
818879
var async = OC.SetupChecks.checkSetup();
819880

‎core/register_command.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,8 @@
209209
$application->add(new OC\Core\Command\Security\ListCertificates(\OC::$server->getCertificateManager(), \OC::$server->getL10N('core')));
210210
$application->add(new OC\Core\Command\Security\ImportCertificate(\OC::$server->getCertificateManager()));
211211
$application->add(new OC\Core\Command\Security\RemoveCertificate(\OC::$server->getCertificateManager()));
212-
$application->add(new OC\Core\Command\Security\ResetBruteforceAttempts(\OC::$server->getBruteForceThrottler()));
212+
$application->add(\OC::$server->get(\OC\Core\Command\Security\BruteforceAttempts::class));
213+
$application->add(\OC::$server->get(\OC\Core\Command\Security\BruteforceResetAttempts::class));
213214
} else {
214215
$application->add(\OC::$server->get(\OC\Core\Command\Maintenance\Install::class));
215216
}

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1021,10 +1021,11 @@
10211021
'OC\\Core\\Command\\Preview\\Generate' => $baseDir . '/core/Command/Preview/Generate.php',
10221022
'OC\\Core\\Command\\Preview\\Repair' => $baseDir . '/core/Command/Preview/Repair.php',
10231023
'OC\\Core\\Command\\Preview\\ResetRenderedTexts' => $baseDir . '/core/Command/Preview/ResetRenderedTexts.php',
1024+
'OC\\Core\\Command\\Security\\BruteforceAttempts' => $baseDir . '/core/Command/Security/BruteforceAttempts.php',
1025+
'OC\\Core\\Command\\Security\\BruteforceResetAttempts' => $baseDir . '/core/Command/Security/BruteforceResetAttempts.php',
10241026
'OC\\Core\\Command\\Security\\ImportCertificate' => $baseDir . '/core/Command/Security/ImportCertificate.php',
10251027
'OC\\Core\\Command\\Security\\ListCertificates' => $baseDir . '/core/Command/Security/ListCertificates.php',
10261028
'OC\\Core\\Command\\Security\\RemoveCertificate' => $baseDir . '/core/Command/Security/RemoveCertificate.php',
1027-
'OC\\Core\\Command\\Security\\ResetBruteforceAttempts' => $baseDir . '/core/Command/Security/ResetBruteforceAttempts.php',
10281029
'OC\\Core\\Command\\Status' => $baseDir . '/core/Command/Status.php',
10291030
'OC\\Core\\Command\\SystemTag\\Add' => $baseDir . '/core/Command/SystemTag/Add.php',
10301031
'OC\\Core\\Command\\SystemTag\\Delete' => $baseDir . '/core/Command/SystemTag/Delete.php',
@@ -1584,6 +1585,9 @@
15841585
'OC\\Search\\Result\\Image' => $baseDir . '/lib/private/Search/Result/Image.php',
15851586
'OC\\Search\\SearchComposer' => $baseDir . '/lib/private/Search/SearchComposer.php',
15861587
'OC\\Search\\SearchQuery' => $baseDir . '/lib/private/Search/SearchQuery.php',
1588+
'OC\\Security\\Bruteforce\\Backend\\DatabaseBackend' => $baseDir . '/lib/private/Security/Bruteforce/Backend/DatabaseBackend.php',
1589+
'OC\\Security\\Bruteforce\\Backend\\IBackend' => $baseDir . '/lib/private/Security/Bruteforce/Backend/IBackend.php',
1590+
'OC\\Security\\Bruteforce\\Backend\\MemoryCacheBackend' => $baseDir . '/lib/private/Security/Bruteforce/Backend/MemoryCacheBackend.php',
15871591
'OC\\Security\\Bruteforce\\Capabilities' => $baseDir . '/lib/private/Security/Bruteforce/Capabilities.php',
15881592
'OC\\Security\\Bruteforce\\CleanupJob' => $baseDir . '/lib/private/Security/Bruteforce/CleanupJob.php',
15891593
'OC\\Security\\Bruteforce\\Throttler' => $baseDir . '/lib/private/Security/Bruteforce/Throttler.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1054,10 +1054,11 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
10541054
'OC\\Core\\Command\\Preview\\Generate' => __DIR__ . '/../../..' . '/core/Command/Preview/Generate.php',
10551055
'OC\\Core\\Command\\Preview\\Repair' => __DIR__ . '/../../..' . '/core/Command/Preview/Repair.php',
10561056
'OC\\Core\\Command\\Preview\\ResetRenderedTexts' => __DIR__ . '/../../..' . '/core/Command/Preview/ResetRenderedTexts.php',
1057+
'OC\\Core\\Command\\Security\\BruteforceAttempts' => __DIR__ . '/../../..' . '/core/Command/Security/BruteforceAttempts.php',
1058+
'OC\\Core\\Command\\Security\\BruteforceResetAttempts' => __DIR__ . '/../../..' . '/core/Command/Security/BruteforceResetAttempts.php',
10571059
'OC\\Core\\Command\\Security\\ImportCertificate' => __DIR__ . '/../../..' . '/core/Command/Security/ImportCertificate.php',
10581060
'OC\\Core\\Command\\Security\\ListCertificates' => __DIR__ . '/../../..' . '/core/Command/Security/ListCertificates.php',
10591061
'OC\\Core\\Command\\Security\\RemoveCertificate' => __DIR__ . '/../../..' . '/core/Command/Security/RemoveCertificate.php',
1060-
'OC\\Core\\Command\\Security\\ResetBruteforceAttempts' => __DIR__ . '/../../..' . '/core/Command/Security/ResetBruteforceAttempts.php',
10611062
'OC\\Core\\Command\\Status' => __DIR__ . '/../../..' . '/core/Command/Status.php',
10621063
'OC\\Core\\Command\\SystemTag\\Add' => __DIR__ . '/../../..' . '/core/Command/SystemTag/Add.php',
10631064
'OC\\Core\\Command\\SystemTag\\Delete' => __DIR__ . '/../../..' . '/core/Command/SystemTag/Delete.php',
@@ -1617,6 +1618,9 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
16171618
'OC\\Search\\Result\\Image' => __DIR__ . '/../../..' . '/lib/private/Search/Result/Image.php',
16181619
'OC\\Search\\SearchComposer' => __DIR__ . '/../../..' . '/lib/private/Search/SearchComposer.php',
16191620
'OC\\Search\\SearchQuery' => __DIR__ . '/../../..' . '/lib/private/Search/SearchQuery.php',
1621+
'OC\\Security\\Bruteforce\\Backend\\DatabaseBackend' => __DIR__ . '/../../..' . '/lib/private/Security/Bruteforce/Backend/DatabaseBackend.php',
1622+
'OC\\Security\\Bruteforce\\Backend\\IBackend' => __DIR__ . '/../../..' . '/lib/private/Security/Bruteforce/Backend/IBackend.php',
1623+
'OC\\Security\\Bruteforce\\Backend\\MemoryCacheBackend' => __DIR__ . '/../../..' . '/lib/private/Security/Bruteforce/Backend/MemoryCacheBackend.php',
16201624
'OC\\Security\\Bruteforce\\Capabilities' => __DIR__ . '/../../..' . '/lib/private/Security/Bruteforce/Capabilities.php',
16211625
'OC\\Security\\Bruteforce\\CleanupJob' => __DIR__ . '/../../..' . '/lib/private/Security/Bruteforce/CleanupJob.php',
16221626
'OC\\Security\\Bruteforce\\Throttler' => __DIR__ . '/../../..' . '/lib/private/Security/Bruteforce/Throttler.php',

0 commit comments

Comments
 (0)