fix: Add bruteforce protection to federation endpoint

Signed-off-by: Joas Schilling <[email protected]>
nextcloud · Feb 21, 2024 · 5f1413e · 5f1413e
1 parent dd291e1
commit 5f1413e
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 3 deletions.
diff --git a/apps/federation/lib/Controller/OCSAuthAPIController.php b/apps/federation/lib/Controller/OCSAuthAPIController.php
@@ -36,6 +36,7 @@
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\IJobList;
 use OCP\IRequest;
+use OCP\Security\Bruteforce\IThrottler;
 use OCP\Security\ISecureRandom;
 use Psr\Log\LoggerInterface;
 
@@ -53,6 +54,7 @@ class OCSAuthAPIController extends OCSController {
 	private DbHandler $dbHandler;
 	private LoggerInterface $logger;
 	private ITimeFactory $timeFactory;
+	private IThrottler $throttler;
 
 	public function __construct(
 		string $appName,
@@ -62,7 +64,8 @@ public function __construct(
 		TrustedServers $trustedServers,
 		DbHandler $dbHandler,
 		LoggerInterface $logger,
-		ITimeFactory $timeFactory
+		ITimeFactory $timeFactory,
+		IThrottler $throttler
 	) {
 		parent::__construct($appName, $request);
 
@@ -72,13 +75,15 @@ public function __construct(
 		$this->dbHandler = $dbHandler;
 		$this->logger = $logger;
 		$this->timeFactory = $timeFactory;
+		$this->throttler = $throttler;
 	}
 
 	/**
 	 * Request received to ask remote server for a shared secret, for legacy end-points
 	 *
 	 * @NoCSRFRequired
 	 * @PublicPage
+	 * @BruteForceProtection(action=federationSharedSecret)
 	 * @throws OCSForbiddenException
 	 */
 	public function requestSharedSecretLegacy(string $url, string $token): DataResponse {
@@ -91,6 +96,7 @@ public function requestSharedSecretLegacy(string $url, string $token): DataRespo
 	 *
 	 * @NoCSRFRequired
 	 * @PublicPage
+	 * @BruteForceProtection(action=federationSharedSecret)
 	 * @throws OCSForbiddenException
 	 */
 	public function getSharedSecretLegacy(string $url, string $token): DataResponse {
@@ -102,10 +108,12 @@ public function getSharedSecretLegacy(string $url, string $token): DataResponse
 	 *
 	 * @NoCSRFRequired
 	 * @PublicPage
+	 * @BruteForceProtection(action=federationSharedSecret)
 	 * @throws OCSForbiddenException
 	 */
 	public function requestSharedSecret(string $url, string $token): DataResponse {
 		if ($this->trustedServers->isTrustedServer($url) === false) {
+			$this->throttler->registerAttempt('federationSharedSecret', $this->request->getRemoteAddress());
 			$this->logger->error('remote server not trusted (' . $url . ') while requesting shared secret', ['app' => 'federation']);
 			throw new OCSForbiddenException();
 		}
@@ -138,15 +146,18 @@ public function requestSharedSecret(string $url, string $token): DataResponse {
 	 *
 	 * @NoCSRFRequired
 	 * @PublicPage
+	 * @BruteForceProtection(action=federationSharedSecret)
 	 * @throws OCSForbiddenException
 	 */
 	public function getSharedSecret(string $url, string $token): DataResponse {
 		if ($this->trustedServers->isTrustedServer($url) === false) {
+			$this->throttler->registerAttempt('federationSharedSecret', $this->request->getRemoteAddress());
 			$this->logger->error('remote server not trusted (' . $url . ') while getting shared secret', ['app' => 'federation']);
 			throw new OCSForbiddenException();
 		}
 
 		if ($this->isValidToken($url, $token) === false) {
+			$this->throttler->registerAttempt('federationSharedSecret', $this->request->getRemoteAddress());
 			$expectedToken = $this->dbHandler->getToken($url);
 			$this->logger->error(
 				'remote server (' . $url . ') didn\'t send a valid token (got "' . $token . '" but expected "'. $expectedToken . '") while getting shared secret',

diff --git a/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php b/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php
@@ -33,6 +33,7 @@
 use OCP\AppFramework\OCS\OCSForbiddenException;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IRequest;
+use OCP\Security\Bruteforce\IThrottler;
 use OCP\Security\ISecureRandom;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
@@ -60,6 +61,9 @@ class OCSAuthAPIControllerTest extends TestCase {
 	/** @var \PHPUnit\Framework\MockObject\MockObject|ITimeFactory */
 	private $timeFactory;
 
+	/** @var \PHPUnit\Framework\MockObject\MockObject|IThrottler */
+	private $throttler;
+
 	private OCSAuthAPIController $ocsAuthApi;
 
 	/** @var int simulated timestamp */
@@ -75,6 +79,7 @@ protected function setUp(): void {
 		$this->jobList = $this->createMock(JobList::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->timeFactory = $this->createMock(ITimeFactory::class);
+		$this->throttler = $this->createMock(IThrottler::class);
 
 		$this->ocsAuthApi = new OCSAuthAPIController(
 			'federation',
@@ -84,7 +89,8 @@ protected function setUp(): void {
 			$this->trustedServers,
 			$this->dbHandler,
 			$this->logger,
-			$this->timeFactory
+			$this->timeFactory,
+			$this->throttler
 		);
 
 		$this->timeFactory->method('getTime')
@@ -109,8 +115,14 @@ public function testRequestSharedSecret(string $token, string $localToken, bool
 		} else {
 			$this->jobList->expects($this->never())->method('add');
 			$this->jobList->expects($this->never())->method('remove');
+			if (!$isTrustedServer) {
+				$this->throttler->expects($this->once())
+					->method('registerAttempt')
+					->with('federationSharedSecret');
+			}
 		}
 
+
 		try {
 			$this->ocsAuthApi->requestSharedSecret($url, $token);
 			$this->assertTrue($ok);
@@ -145,7 +157,8 @@ public function testGetSharedSecret(bool $isTrustedServer, bool $isValidToken, b
 					$this->trustedServers,
 					$this->dbHandler,
 					$this->logger,
-					$this->timeFactory
+					$this->timeFactory,
+					$this->throttler
 				]
 			)->setMethods(['isValidToken'])->getMock();
 
@@ -163,6 +176,9 @@ public function testGetSharedSecret(bool $isTrustedServer, bool $isValidToken, b
 		} else {
 			$this->secureRandom->expects($this->never())->method('generate');
 			$this->trustedServers->expects($this->never())->method('addSharedSecret');
+			$this->throttler->expects($this->once())
+				->method('registerAttempt')
+				->with('federationSharedSecret');
 		}
 
 		try {