Merge pull request #43713 from nextcloud/backport/43712/stable28

[stable28] fix: Add bruteforce protection to federation endpoint

Author: nickvergessen

apps/federation/lib/Controller/OCSAuthAPIController.php

@@ -38,6 +38,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;
 
@@ -56,6 +57,7 @@ class OCSAuthAPIController extends OCSController {
 	private DbHandler $dbHandler;
 	private LoggerInterface $logger;
 	private ITimeFactory $timeFactory;
+	private IThrottler $throttler;
 
 	public function __construct(
 		string $appName,
@@ -65,7 +67,8 @@ public function __construct(
 		TrustedServers $trustedServers,
 		DbHandler $dbHandler,
 		LoggerInterface $logger,
-		ITimeFactory $timeFactory
+		ITimeFactory $timeFactory,
+		IThrottler $throttler
 	) {
 		parent::__construct($appName, $request);
 
@@ -75,13 +78,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)
 	 *
 	 * @param string $url URL of the server
 	 * @param string $token Token of the server
@@ -100,6 +105,7 @@ public function requestSharedSecretLegacy(string $url, string $token): DataRespo
 	 *
 	 * @NoCSRFRequired
 	 * @PublicPage
+	 * @BruteForceProtection(action=federationSharedSecret)
 	 *
 	 * @param string $url URL of the server
 	 * @param string $token Token of the server
@@ -117,6 +123,7 @@ public function getSharedSecretLegacy(string $url, string $token): DataResponse
 	 *
 	 * @NoCSRFRequired
 	 * @PublicPage
+	 * @BruteForceProtection(action=federationSharedSecret)
 	 *
 	 * @param string $url URL of the server
 	 * @param string $token Token of the server
@@ -127,6 +134,7 @@ public function getSharedSecretLegacy(string $url, string $token): DataResponse
 	 */
 	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();
 		}
@@ -159,6 +167,7 @@ public function requestSharedSecret(string $url, string $token): DataResponse {
 	 *
 	 * @NoCSRFRequired
 	 * @PublicPage
+	 * @BruteForceProtection(action=federationSharedSecret)
 	 *
 	 * @param string $url URL of the server
 	 * @param string $token Token of the server
@@ -169,11 +178,13 @@ public function requestSharedSecret(string $url, string $token): DataResponse {
 	 */
 	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',

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;
@@ -59,6 +60,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 */
@@ -74,6 +78,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',
@@ -83,7 +88,8 @@ protected function setUp(): void {
 			$this->trustedServers,
 			$this->dbHandler,
 			$this->logger,
-			$this->timeFactory
+			$this->timeFactory,
+			$this->throttler
 		);
 
 		$this->timeFactory->method('getTime')
@@ -108,8 +114,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);
@@ -144,7 +156,8 @@ public function testGetSharedSecret(bool $isTrustedServer, bool $isValidToken, b
 					$this->trustedServers,
 					$this->dbHandler,
 					$this->logger,
-					$this->timeFactory
+					$this->timeFactory,
+					$this->throttler
 				]
 			)->setMethods(['isValidToken'])->getMock();
 
@@ -162,6 +175,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 {