- Added supports for fixed execution times to migitate possible timin…

…g attacks
metaclass-nl · May 9, 2015 · 4de9ae1 · 4de9ae1
1 parent d6c588c
commit 4de9ae1
Show file tree

Hide file tree

Showing 5 changed files with 86 additions and 38 deletions.
diff --git a/DependencyInjection/Configuration.php b/DependencyInjection/Configuration.php
@@ -37,6 +37,8 @@ public function getConfigTreeBuilder()
                         ->scalarNode('allowReleasedUserOnAddressFor')->defaultValue('30 days')->end()
                         ->scalarNode('releaseUserOnLoginSuccess')->defaultValue(false)->end()
                         ->scalarNode('keepCountsFor')->defaultValue('4 days')->end()
+                        ->scalarNode('fixedExecutionSeconds')->defaultValue('0.1')->end()
+                        ->scalarNode('randomSleepingNanosecondsMax')->defaultValue('99999')->end()
                     ->end()
                 ->end()
             ->end();

diff --git a/README.md b/README.md
@@ -41,12 +41,16 @@ This is a pre-release version under development.
 Currently the Bundle can only protect form-based authentication using the security.authentication.listener.form service 
 (Default: Symfony\Component\Security\Http\Firewall\UsernamePasswordFormAuthenticationListener).
 
-May be vurnerable to user enumeration through timing attacks because of differences in database query performance 
-for frequently and infrequently used usernames,
-
-Throws specific types of Exceptions for different situations (for logging purposes) and leaves it to the 
+Throws specific types of Exceptions for different situations (for logging purposes) and leaves it to the
 login form to hyde differences between them that should not be reported to users.
 
+May be vurnerable to enumeration of usernames through timing attacks because of
+differences in database query performance for frequently and infrequently used usernames.
+This can be mitigated by calling ::sleepUntilFixedExecutionTime. Under normal circomstances
+that should be sufficient if the fixedExecutionSeconds is set long enough, but under
+high (database) server loads when performance degrades, under specific conditons
+information may still be extractable by timing.
+
 DOCUMENTATION
 -------------
 - [Installation and configuration](Resources/doc/Installation.md)

diff --git a/Resources/doc/Installation.md b/Resources/doc/Installation.md
@@ -73,6 +73,7 @@ Installation
             abstract: true
             calls:
                 - [setGovenor, ["@metaclass_auth_guard.tresholds_governor"] ] # REQUIRED
+                - [setAuthExecutionSeconds, [0.99]] # voluntary
     ```
 
 7. You may also add the following configuraton parameters (defaults shown):
@@ -88,7 +89,10 @@ metaclass_authentication_guard:
         blockIpAddressesFor: "17 minutes"     # actual blocking for up to counterDurationInSeconds shorter!
         limitBasePerIpAddress: 10
         releaseUserOnLoginSuccess: false
-        allowReleasedUserOnAddressFor: "30 days" 
+        allowReleasedUserOnAddressFor: "30 days"
+        keepCountsFor: "4 days"
+        fixedExecutionSeconds: "0.1"
+        randomSleepingNanosecondsMax: 99999
     ```
 
 8. From cron or so you may garbage-collect/pack stored RequestCounts:
@@ -252,6 +256,20 @@ Configurations
     garbage collected, but if allowReleasedUserOnAddressFor (or allowReleasedUserByCookieFor)
     is set to a longer duration, the releases will be kept longer (according to the longest one).
 
+10. Fixed execution time
+
+    fixedExecutionSeconds
+
+    Fixed execution time in order to mitigate timing attacks. To apply, call ::sleepUntilFixedExecutionTime.
+
+11. Maximum random sleeping time in nanoseconds
+
+    randomSleepingNanosecondsMax
+
+    Because of doubts about the accurateness of microtime() and to hide system clock
+    details a random between 0 and this value is added by ::sleepUntilSinceInit (which
+    is called by ::sleepUntilFixedExecutionTime).
+
 Notes
 
 - releasing is possible for a username in general, an IP address in general, or for the combination of a username with an ip address

diff --git a/Resources/doc/changelog.txt b/Resources/doc/changelog.txt
@@ -202,8 +202,11 @@ comitted, pushed
 - README.md added user interface for user administrators to features list
 - doc/Installation.md documented configuration for user interface for user administrators
     added keepCountsFor parameter to configurations
-comitted, pushed
+committed, pushed
 -------------------
+- Added supports for fixed execution times to migitate possible timing attacks
+committed, pushed
+--------------------
 2DO:
 - dependency statistics.html.twig van MetaclassCoreBundle::layout.html.twig configurable
     also editrow.html.twig (may use default)
diff --git a/Service/UsernamePasswordFormAuthenticationGuard.php b/Service/UsernamePasswordFormAuthenticationGuard.php
@@ -33,7 +33,8 @@ class UsernamePasswordFormAuthenticationGuard extends AbstractAuthenticationList
     protected $mySecurityContext;
 
     protected $governor;
-    static $usernamePattern = '/([^\\x20-\\x7E])/u'; //default is to allow all 1 to 1 visible ASCII characters (from space to ~). This excludes CR, LF, Tab , FF
+    public $authExecutionSeconds;
+    static $usernamePattern = '/([^\\x20-\\x7E])/u'; //default is to allow all 1 to 1 visible ASCII characters (from space to ~). This excludes CR, LF, Tab , FF. If you want to be able to register e-mail addresses, don't exclude @
     static $passwordPattern; //if not set, usernamePattern is used
 
     /**
@@ -64,8 +65,17 @@ public function setValidationPatterns($usernamePattern, $passwordPattern=null) {
             self::$passwordPattern = $passwordPattern;
         }
     }
-    
+
     /**
+     * In order to hide execution time differences when authentication is not blocked
+     * @var float How long execution of the authentication process should take
+     */
+    public function setAuthExecutionSeconds($seconds)
+    {
+        $this->authExecutionSeconds = $seconds;
+    }
+
+/**
      * {@inheritdoc}
      */
     protected function requiresAuthentication(Request $request)
@@ -82,6 +92,7 @@ protected function requiresAuthentication(Request $request)
      */
     protected function attemptAuthentication(Request $request)
     {
+        $exception = null;
         $originalCred = $this->getCredentials($request);
         $filteredCred = $this->filterCredentials($originalCred);
         $request->getSession()->set(SecurityContextInterface::LAST_USERNAME, $originalCred[0]);
@@ -90,7 +101,7 @@ protected function attemptAuthentication(Request $request)
             $this->checkCrsfToken($request);
         }
 
-        //initialize the governer so that we can register a failure
+        //initialize the governor so that we can register a failure
         $this->governor->initFor(
                  $request->getClientIp()
                 , $filteredCred[0]
@@ -100,42 +111,52 @@ protected function attemptAuthentication(Request $request)
 
         if ($originalCred != $filteredCred) { //we can not accept invalid characters
             $this->governor->registerAuthenticationFailure();
-            throw new BadCredentialsException('Credentials contain invalid character(s)');
-        }
-
-        $this->throwExceptionOnRejection($this->governor->checkAuthentication()); //may register failure 
-
-        //not blocked, try to authenticate
-        try {
-            $newToken = $this->authenticationManager->authenticate(new UsernamePasswordToken($filteredCred[0], $filteredCred[1], $this->providerKey));
-        } catch (AuthenticationException $e) {
-            if ($this->isClientResponsibleFor($e)) {
-                $this->governor->registerAuthenticationFailure();
-            } //else do not register service errors as failures
-            throw $e;
-        }
-
-        //authenticated!
-        $this->governor->registerAuthenticationSuccess();
-
-        //when the user goes to the login page without logging out or on reauthentication because of 
-        //an InsufficientAuthenticationException there may still be a UsernamePasswordToken 
-        $oldToken = $this->mySecurityContext->getToken();
-        $oldUserName = $oldToken instanceof UsernamePasswordToken ? $oldToken->getUserName() : '';
-        if ($newToken instanceof UsernamePasswordToken && trim($newToken->getUserName()) != trim($oldUserName)) {
-            //user has changed without logout, clear session so that the data of the old user can not leak to the new user
-            $request->getSession()->clear();
-        }
+            $exception = new BadCredentialsException('Credentials contain invalid character(s)');
+        } else {
+            $exception = $this->getExceptionOnRejection($this->governor->checkAuthentication()); //may register failure
+            if ($exception === null) {
+                //not blocked, try to authenticate
+                try {
+                    $newToken = $this->authenticationManager->authenticate(new UsernamePasswordToken($filteredCred[0], $filteredCred[1], $this->providerKey));
+
+                    //authenticated! No need to hide timing
+                    $this->governor->registerAuthenticationSuccess();
+
+                    //when the user goes to the login page without logging out or on reauthentication because of
+                    //an InsufficientAuthenticationException there may still be a UsernamePasswordToken
+                    $oldToken = $this->mySecurityContext->getToken();
+                    $oldUserName = $oldToken instanceof UsernamePasswordToken ? $oldToken->getUserName() : '';
+                    if ($newToken instanceof UsernamePasswordToken && trim($newToken->getUserName()) != trim($oldUserName)) {
+                        //user has changed without logout, clear session so that the data of the old user can not leak to the new user
+                        $request->getSession()->clear();
+                    }
+
+                    return $newToken;
+               } catch (AuthenticationException $e) {
+                    if ($this->isClientResponsibleFor($e)) {
+                        $this->governor->registerAuthenticationFailure();
+                    } //else do not register service errors as failures
+                    // wait to hide eventual execution time differences
+                    if ($this->authExecutionSeconds) {
+                        // \Gen::show($this->governor->getSecondsPassedSinceInit()); die();
+                        $this->governor->sleepUntilSinceInit($this->authExecutionSeconds);
+                    }
+                    throw $e;
+               }
+           }
+        } // end $originalCred != $filteredCred
+
+        $this->governor->sleepUntilFixedExecutionTime(); // hides execution time differences of tresholds governor
 
-        return $newToken;
+        throw $exception;
     }
 
-    protected function throwExceptionOnRejection($rejection)
+    protected function getExceptionOnRejection($rejection)
     {
         if ($rejection) {
             $exceptionClass = 'Metaclass\\AuthenticationGuardBundle\\Exception\\'
                 . subStr(get_class($rejection), 35). 'Exception';
-            throw new $exceptionClass(strtr($rejection->message, $rejection->parameters));
+            return new $exceptionClass(strtr($rejection->message, $rejection->parameters));
         }
     }