From df296249d6ca4c9980bb23acdb6d9353d0d69996 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 17 Jan 2017 11:51:10 +0100 Subject: [PATCH 1/5] introduce brute force protection for api calls Signed-off-by: Bjoern Schiessle --- .../DependencyInjection/DIContainer.php | 21 ++++++++++++------- .../Security/SecurityMiddleware.php | 14 ++++++++++++- lib/private/Security/Bruteforce/Throttler.php | 13 +++++++++--- 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index ac42960f54..57499f3ffe 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 d60d5749d5..dcfab3544b 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->request->getRequestUri(); + $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/Security/Bruteforce/Throttler.php b/lib/private/Security/Bruteforce/Throttler.php index 031c5ffd41..765f109fdb 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; } From 29a0a239183c9a2b8ab89b32b7ce5afde922159b Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 17 Jan 2017 14:22:25 +0100 Subject: [PATCH 2/5] Fix the regex for annotations with values Signed-off-by: Joas Schilling --- lib/private/AppFramework/Utility/ControllerMethodReflector.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/AppFramework/Utility/ControllerMethodReflector.php b/lib/private/AppFramework/Utility/ControllerMethodReflector.php index 33a117d812..9f653b0384 100644 --- a/lib/private/AppFramework/Utility/ControllerMethodReflector.php +++ b/lib/private/AppFramework/Utility/ControllerMethodReflector.php @@ -55,7 +55,7 @@ class ControllerMethodReflector implements IControllerMethodReflector{ $docs = $reflection->getDocComment(); // extract everything prefixed by @ and first letter uppercase - preg_match_all('/@([A-Z]\w+)/', $docs, $matches); + preg_match_all('/^\h+\*\h+@(?P[A-Z]\w+)(\h+(?P\w+))?$/m', $docs, $matches); $this->annotations = $matches[1]; // extract type parameter information From 32e0ec3e585d516749f9b1a096abb78ca3003d61 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 17 Jan 2017 14:36:44 +0100 Subject: [PATCH 3/5] handle optional annotation parameters Signed-off-by: Bjoern Schiessle --- .../Security/SecurityMiddleware.php | 2 +- .../Utility/ControllerMethodReflector.php | 21 +++++++++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index dcfab3544b..edba6a3e75 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -192,7 +192,7 @@ class SecurityMiddleware extends Middleware { } if($this->reflector->hasAnnotation('BruteForceProtection')) { - $action = $this->request->getRequestUri(); + $action = $this->reflector->getAnnotationParameter('BruteForceProtection'); $this->throttler->sleepDelay($this->request->getRemoteAddress(), $action); $this->throttler->registerAttempt($action, $this->request->getRemoteAddress()); } diff --git a/lib/private/AppFramework/Utility/ControllerMethodReflector.php b/lib/private/AppFramework/Utility/ControllerMethodReflector.php index 9f653b0384..034fc3a175 100644 --- a/lib/private/AppFramework/Utility/ControllerMethodReflector.php +++ b/lib/private/AppFramework/Utility/ControllerMethodReflector.php @@ -56,7 +56,9 @@ class ControllerMethodReflector implements IControllerMethodReflector{ // extract everything prefixed by @ and first letter uppercase preg_match_all('/^\h+\*\h+@(?P[A-Z]\w+)(\h+(?P\w+))?$/m', $docs, $matches); - $this->annotations = $matches[1]; + foreach($matches['annotation'] as $key => $annontation) { + $this->annotations[$annontation] = $matches['parameter'][$key]; + } // extract type parameter information preg_match_all('/@param\h+(?P\w+)\h+\$(?P\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; } From 0271ae3b46e3421871b8eecb4b453dd5793e5e30 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 17 Jan 2017 17:11:34 +0100 Subject: [PATCH 4/5] add some unit tests Signed-off-by: Bjoern Schiessle --- .../Security/SecurityMiddlewareTest.php | 73 ++++++++++++++++++- .../Utility/ControllerMethodReflectorTest.php | 13 ++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index 5a98875107..164ea48de7 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 92d767e998..644245e196 100644 --- a/tests/lib/AppFramework/Utility/ControllerMethodReflectorTest.php +++ b/tests/lib/AppFramework/Utility/ControllerMethodReflectorTest.php @@ -76,6 +76,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 From cdf01feba78696aa74b7f57a43380757d67df4aa Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 17 Jan 2017 17:21:27 +0100 Subject: [PATCH 5/5] add action to existing brute force protection Signed-off-by: Bjoern Schiessle --- core/Controller/LoginController.php | 12 ++++++------ core/Controller/OCSController.php | 2 +- lib/private/User/Session.php | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 3c81ed5242..187c818b9e 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 c59b0d7ad3..dc9775f260 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/User/Session.php b/lib/private/User/Session.php index 1834bd025d..9cc42e671a 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);