Merge pull request #3120 from nextcloud/brute-force-protection-api

brute force protection for api calls

9 files changed, 152 insertions, 25 deletions

diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php
index 3c81ed5242a..187c818b9e1 100644
--- a/core/Controller/LoginController.php
+++ b/core/Controller/LoginController.php
@@ -205,8 +205,8 @@ class LoginController extends Controller {
 	 * @return RedirectResponse
 	 */
 	public function tryLogin($user, $password, $redirect_url, $remember_login = false, $timezone = '', $timezone_offset = '') {
-		$currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress());
-		$this->throttler->sleepDelay($this->request->getRemoteAddress());
+		$currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress(), 'login');
+		$this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login');
 
 		// If the user is already logged in and the CSRF check does not pass then
 		// simply redirect the user to the correct page as required. This is the
@@ -230,7 +230,7 @@ class LoginController extends Controller {
 		if ($loginResult === false) {
 			$this->throttler->registerAttempt('login', $this->request->getRemoteAddress(), ['user' => $originalUser]);
 			if($currentDelay === 0) {
-				$this->throttler->sleepDelay($this->request->getRemoteAddress());
+				$this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login');
 			}
 			$this->session->set('loginMessages', [
 				['invalidpassword'], []
@@ -295,15 +295,15 @@ class LoginController extends Controller {
 	 * @return DataResponse
 	 */
 	public function confirmPassword($password) {
-		$currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress());
-		$this->throttler->sleepDelay($this->request->getRemoteAddress());
+		$currentDelay = $this->throttler->getDelay($this->request->getRemoteAddress(), 'sudo');
+		$this->throttler->sleepDelay($this->request->getRemoteAddress(), 'sudo');
 
 		$loginName = $this->userSession->getLoginName();
 		$loginResult = $this->userManager->checkPassword($loginName, $password);
 		if ($loginResult === false) {
 			$this->throttler->registerAttempt('sudo', $this->request->getRemoteAddress(), ['user' => $loginName]);
 			if ($currentDelay === 0) {
-				$this->throttler->sleepDelay($this->request->getRemoteAddress());
+				$this->throttler->sleepDelay($this->request->getRemoteAddress(), 'sudo');
 			}
 
 			return new DataResponse([], Http::STATUS_FORBIDDEN);
diff --git a/core/Controller/OCSController.php b/core/Controller/OCSController.php
index c59b0d7ad3f..dc9775f2603 100644
--- a/core/Controller/OCSController.php
+++ b/core/Controller/OCSController.php
@@ -128,7 +128,7 @@ class OCSController extends \OCP\AppFramework\OCSController {
 	 */
 	public function personCheck($login = '', $password = '') {
 		if ($login !== '' && $password !== '') {
-			$this->throttler->sleepDelay($this->request->getRemoteAddress());
+			$this->throttler->sleepDelay($this->request->getRemoteAddress(), 'login');
 			if ($this->userManager->checkPassword($login, $password)) {
 				return new DataResponse([
 					'person' => [
diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php
index ac42960f54d..57499f3ffe8 100644
--- a/lib/private/AppFramework/DependencyInjection/DIContainer.php
+++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php
@@ -43,8 +43,10 @@ use OC\AppFramework\Middleware\OCSMiddleware;
 use OC\AppFramework\Middleware\Security\SecurityMiddleware;
 use OC\AppFramework\Middleware\SessionMiddleware;
 use OC\AppFramework\Utility\SimpleContainer;
+use OC\AppFramework\Utility\TimeFactory;
 use OC\Core\Middleware\TwoFactorMiddleware;
 use OC\RichObjectStrings\Validator;
+use OC\Security\Bruteforce\Throttler;
 use OCP\AppFramework\IApi;
 use OCP\AppFramework\IAppContainer;
 use OCP\Files\IAppData;
@@ -376,20 +378,25 @@ class DIContainer extends SimpleContainer implements IAppContainer {
 		 */
 		$app = $this;
 		$this->registerService('SecurityMiddleware', function($c) use ($app){
+			/** @var \OC\Server $server */
+			$server = $app->getServer();
+
 			return new SecurityMiddleware(
 				$c['Request'],
 				$c['ControllerMethodReflector'],
-				$app->getServer()->getNavigationManager(),
-				$app->getServer()->getURLGenerator(),
-				$app->getServer()->getLogger(),
-				$app->getServer()->getSession(),
+				$server->getNavigationManager(),
+				$server->getURLGenerator(),
+				$server->getLogger(),
+				$server->getSession(),
 				$c['AppName'],
 				$app->isLoggedIn(),
 				$app->isAdminUser(),
-				$app->getServer()->getContentSecurityPolicyManager(),
-				$app->getServer()->getCsrfTokenManager(),
-				$app->getServer()->getContentSecurityPolicyNonceManager()
+				$server->getContentSecurityPolicyManager(),
+				$server->getCsrfTokenManager(),
+				$server->getContentSecurityPolicyNonceManager(),
+				$server->getBruteForceThrottler()
 			);
+
 		});
 
 		$this->registerService('CORSMiddleware', function($c) {
diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
index d60d5749d57..edba6a3e759 100644
--- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
+++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php
@@ -36,6 +36,7 @@ use OC\AppFramework\Middleware\Security\Exceptions\NotConfirmedException;
 use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException;
 use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException;
 use OC\AppFramework\Utility\ControllerMethodReflector;
+use OC\Security\Bruteforce\Throttler;
 use OC\Security\CSP\ContentSecurityPolicyManager;
 use OC\Security\CSP\ContentSecurityPolicyNonceManager;
 use OC\Security\CSRF\CsrfTokenManager;
@@ -87,6 +88,8 @@ class SecurityMiddleware extends Middleware {
 	private $csrfTokenManager;
 	/** @var ContentSecurityPolicyNonceManager */
 	private $cspNonceManager;
+	/** @var Throttler */
+	private $throttler;
 
 	/**
 	 * @param IRequest $request
@@ -101,6 +104,7 @@ class SecurityMiddleware extends Middleware {
 	 * @param ContentSecurityPolicyManager $contentSecurityPolicyManager
 	 * @param CSRFTokenManager $csrfTokenManager
 	 * @param ContentSecurityPolicyNonceManager $cspNonceManager
+	 * @param Throttler $throttler
 	 */
 	public function __construct(IRequest $request,
 								ControllerMethodReflector $reflector,
@@ -113,7 +117,8 @@ class SecurityMiddleware extends Middleware {
 								$isAdminUser,
 								ContentSecurityPolicyManager $contentSecurityPolicyManager,
 								CsrfTokenManager $csrfTokenManager,
-								ContentSecurityPolicyNonceManager $cspNonceManager) {
+								ContentSecurityPolicyNonceManager $cspNonceManager,
+								Throttler $throttler) {
 		$this->navigationManager = $navigationManager;
 		$this->request = $request;
 		$this->reflector = $reflector;
@@ -126,6 +131,7 @@ class SecurityMiddleware extends Middleware {
 		$this->contentSecurityPolicyManager = $contentSecurityPolicyManager;
 		$this->csrfTokenManager = $csrfTokenManager;
 		$this->cspNonceManager = $cspNonceManager;
+		$this->throttler = $throttler;
 	}
 
 
@@ -185,6 +191,12 @@ class SecurityMiddleware extends Middleware {
 			}
 		}
 
+		if($this->reflector->hasAnnotation('BruteForceProtection')) {
+			$action = $this->reflector->getAnnotationParameter('BruteForceProtection');
+			$this->throttler->sleepDelay($this->request->getRemoteAddress(), $action);
+			$this->throttler->registerAttempt($action, $this->request->getRemoteAddress());
+		}
+
 		/**
 		 * FIXME: Use DI once available
 		 * Checks if app is enabled (also includes a check whether user is allowed to access the resource)
diff --git a/lib/private/AppFramework/Utility/ControllerMethodReflector.php b/lib/private/AppFramework/Utility/ControllerMethodReflector.php
index 33a117d8121..034fc3a1759 100644
--- a/lib/private/AppFramework/Utility/ControllerMethodReflector.php
+++ b/lib/private/AppFramework/Utility/ControllerMethodReflector.php
@@ -55,8 +55,10 @@ class ControllerMethodReflector implements IControllerMethodReflector{
 		$docs = $reflection->getDocComment();
 
 		// extract everything prefixed by @ and first letter uppercase
-		preg_match_all('/@([A-Z]\w+)/', $docs, $matches);
-		$this->annotations = $matches[1];
+		preg_match_all('/^\h+\*\h+@(?P<annotation>[A-Z]\w+)(\h+(?P<parameter>\w+))?$/m', $docs, $matches);
+		foreach($matches['annotation'] as $key => $annontation) {
+			$this->annotations[$annontation] = $matches['parameter'][$key];
+		}
 
 		// extract type parameter information
 		preg_match_all('/@param\h+(?P<type>\w+)\h+\$(?P<var>\w+)/', $docs, $matches);
@@ -112,7 +114,22 @@ class ControllerMethodReflector implements IControllerMethodReflector{
 	 * @return bool true if the annotation is found
 	 */
 	public function hasAnnotation($name){
-		return in_array($name, $this->annotations);
+		return array_key_exists($name, $this->annotations);
+	}
+
+
+	/**
+	 * Get optional annotation parameter
+	 * @param string $name the name of the annotation
+	 * @return string
+	 */
+	public function getAnnotationParameter($name){
+		$parameter = '';
+		if($this->hasAnnotation($name)) {
+			$parameter = $this->annotations[$name];
+		}
+
+		return $parameter;
 	}
 
 
diff --git a/lib/private/Security/Bruteforce/Throttler.php b/lib/private/Security/Bruteforce/Throttler.php
index 031c5ffd411..765f109fdb3 100644
--- a/lib/private/Security/Bruteforce/Throttler.php
+++ b/lib/private/Security/Bruteforce/Throttler.php
@@ -189,9 +189,10 @@ class Throttler {
 	 * Get the throttling delay (in milliseconds)
 	 *
 	 * @param string $ip
+	 * @param string $action optionally filter by action
 	 * @return int
 	 */
-	public function getDelay($ip) {
+	public function getDelay($ip, $action = '') {
 		$cutoffTime = (new \DateTime())
 			->sub($this->getCutoff(43200))
 			->getTimestamp();
@@ -201,6 +202,11 @@ class Throttler {
 			->from('bruteforce_attempts')
 			->where($qb->expr()->gt('occurred', $qb->createNamedParameter($cutoffTime)))
 			->andWhere($qb->expr()->eq('subnet', $qb->createNamedParameter($this->getSubnet($ip))));
+
+		if ($action !== '') {
+			$qb->andWhere($qb->expr()->eq('action', $qb->createNamedParameter($action)));
+		}
+
 		$attempts = count($qb->execute()->fetchAll());
 
 		if ($attempts === 0) {
@@ -225,10 +231,11 @@ class Throttler {
 	 * Will sleep for the defined amount of time
 	 *
 	 * @param string $ip
+	 * @param string $action optionally filter by action
 	 * @return int the time spent sleeping
 	 */
-	public function sleepDelay($ip) {
-		$delay = $this->getDelay($ip);
+	public function sleepDelay($ip, $action = '') {
+		$delay = $this->getDelay($ip, $action);
 		usleep($delay * 1000);
 		return $delay;
 	}
diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php
index 1834bd025d1..9cc42e671a8 100644
--- a/lib/private/User/Session.php
+++ b/lib/private/User/Session.php
@@ -317,7 +317,7 @@ class Session implements IUserSession, Emitter {
 								$password,
 								IRequest $request,
 								OC\Security\Bruteforce\Throttler $throttler) {
-		$currentDelay = $throttler->sleepDelay($request->getRemoteAddress());
+		$currentDelay = $throttler->sleepDelay($request->getRemoteAddress(), 'login');
 
 		$isTokenPassword = $this->isTokenPassword($password);
 		if (!$isTokenPassword && $this->isTokenAuthEnforced()) {
@@ -334,7 +334,7 @@ class Session implements IUserSession, Emitter {
 
 			$throttler->registerAttempt('login', $request->getRemoteAddress(), ['uid' => $user]);
 			if($currentDelay === 0) {
-				$throttler->sleepDelay($request->getRemoteAddress());
+				$throttler->sleepDelay($request->getRemoteAddress(), 'login');
 			}
 			return false;
 		}
@@ -768,7 +768,7 @@ class Session implements IUserSession, Emitter {
 			try {
 				$this->tokenProvider->invalidateToken($this->session->getId());
 			} catch (SessionNotAvailableException $ex) {
-				
+
 			}
 		}
 		$this->setUser(null);
diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
index 5a988751070..164ea48de70 100644
--- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
+++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
@@ -34,6 +34,7 @@ use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
 use OC\Appframework\Middleware\Security\Exceptions\StrictCookieMissingException;
 use OC\AppFramework\Middleware\Security\SecurityMiddleware;
 use OC\AppFramework\Utility\ControllerMethodReflector;
+use OC\Security\Bruteforce\Throttler;
 use OC\Security\CSP\ContentSecurityPolicy;
 use OC\Security\CSP\ContentSecurityPolicyManager;
 use OC\Security\CSP\ContentSecurityPolicyNonceManager;
@@ -82,6 +83,8 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 	private $csrfTokenManager;
 	/** @var ContentSecurityPolicyNonceManager|\PHPUnit_Framework_MockObject_MockObject */
 	private $cspNonceManager;
+	/** @var  Throttler|\PHPUnit_Framework_MockObject_MockObject */
+	private $bruteForceThrottler;
 
 	protected function setUp() {
 		parent::setUp();
@@ -96,6 +99,7 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 		$this->contentSecurityPolicyManager = $this->createMock(ContentSecurityPolicyManager::class);
 		$this->csrfTokenManager = $this->createMock(CsrfTokenManager::class);
 		$this->cspNonceManager = $this->createMock(ContentSecurityPolicyNonceManager::class);
+		$this->bruteForceThrottler = $this->getMockBuilder(Throttler::class)->disableOriginalConstructor()->getMock();
 		$this->middleware = $this->getMiddleware(true, true);
 		$this->secException = new SecurityException('hey', false);
 		$this->secAjaxException = new SecurityException('hey', true);
@@ -119,7 +123,8 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 			$isAdminUser,
 			$this->contentSecurityPolicyManager,
 			$this->csrfTokenManager,
-			$this->cspNonceManager
+			$this->cspNonceManager,
+			$this->bruteForceThrottler
 		);
 	}
 
@@ -652,4 +657,70 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 
 		$this->assertEquals($response, $this->middleware->afterController($this->controller, 'test', $response));
 	}
+
+	/**
+	 * @dataProvider dataTestBeforeControllerBruteForce
+	 */
+	public function testBeforeControllerBruteForce($bruteForceProtectionEnabled) {
+		/** @var ControllerMethodReflector|\PHPUnit_Framework_MockObject_MockObject $reader */
+		$reader = $this->getMockBuilder(ControllerMethodReflector::class)->disableOriginalConstructor()->getMock();
+
+		$middleware = new SecurityMiddleware(
+			$this->request,
+			$reader,
+			$this->navigationManager,
+			$this->urlGenerator,
+			$this->logger,
+			$this->session,
+			'files',
+			false,
+			false,
+			$this->contentSecurityPolicyManager,
+			$this->csrfTokenManager,
+			$this->cspNonceManager,
+			$this->bruteForceThrottler
+		);
+
+		$reader->expects($this->any())->method('hasAnnotation')
+			->willReturnCallback(
+				function($annotation) use ($bruteForceProtectionEnabled) {
+
+					switch ($annotation) {
+						case 'BruteForceProtection':
+							return $bruteForceProtectionEnabled;
+						case 'PasswordConfirmationRequired':
+						case 'StrictCookieRequired':
+							return false;
+						case 'PublicPage':
+						case 'NoCSRFRequired':
+							return true;
+					}
+
+					return true;
+			}
+			);
+
+		$reader->expects($this->any())->method('getAnnotationParameter')->willReturn('action');
+		$this->request->expects($this->any())->method('getRemoteAddress')->willReturn('remoteAddress');
+
+		if ($bruteForceProtectionEnabled) {
+			$this->bruteForceThrottler->expects($this->once())->method('sleepDelay')
+				->with('remoteAddress', 'action');
+			$this->bruteForceThrottler->expects($this->once())->method('registerAttempt')
+				->with('action', 'remoteAddress');
+		} else {
+			$this->bruteForceThrottler->expects($this->never())->method('sleepDelay');
+			$this->bruteForceThrottler->expects($this->never())->method('registerAttempt');
+		}
+
+		$middleware->beforeController($this->controller, 'test');
+
+	}
+
+	public function dataTestBeforeControllerBruteForce() {
+		return [
+			[true],
+			[false]
+		];
+	}
 }
diff --git a/tests/lib/AppFramework/Utility/ControllerMethodReflectorTest.php b/tests/lib/AppFramework/Utility/ControllerMethodReflectorTest.php
index 92d767e9987..644245e1967 100644
--- a/tests/lib/AppFramework/Utility/ControllerMethodReflectorTest.php
+++ b/tests/lib/AppFramework/Utility/ControllerMethodReflectorTest.php
@@ -77,6 +77,19 @@ class ControllerMethodReflectorTest extends \Test\TestCase {
 
 
 	/**
+	 * @Annotation parameter
+	 */
+	public function testGetAnnotationParameter(){
+		$reader = new ControllerMethodReflector();
+		$reader->reflect(
+			'\Test\AppFramework\Utility\ControllerMethodReflectorTest',
+			'testGetAnnotationParameter'
+		);
+
+		$this->assertSame('parameter', $reader->getAnnotationParameter('Annotation'));
+	}
+
+	/**
 	 * @Annotation
 	 * @param test
 	 */