From c1680b2dede92d2c1baf9d9a6493c7af4dbb7bbc Mon Sep 17 00:00:00 2001 From: Mark Story Date: Sat, 29 Oct 2016 23:45:05 -0400 Subject: [PATCH] Implement more robust nonce handling in Digest authentication. Shore up some of the implementation gaps we have around nonces. Nonces should expire and should be generated by the server. I've followed the Symfony approach to generating nonces which results in short lived (5 minutes by default) nonces that are regenerated each time the client receives a 401. By validating and requiring nonce rotation we can avoid replay attacks with our Digest authentication implementation. I've targetted 3.next as validating/requiring nonce rotation is a behavior change that I feel could trip up application developers. By including it in a minor release we can more effectively communicate the changes. Refs #9668 --- src/Auth/DigestAuthenticate.php | 66 ++++- .../TestCase/Auth/DigestAuthenticateTest.php | 235 ++++++++++++++---- 2 files changed, 243 insertions(+), 58 deletions(-) diff --git a/src/Auth/DigestAuthenticate.php b/src/Auth/DigestAuthenticate.php index 3b9fd0df12a..7d8f8a0fed2 100644 --- a/src/Auth/DigestAuthenticate.php +++ b/src/Auth/DigestAuthenticate.php @@ -15,6 +15,8 @@ namespace Cake\Auth; use Cake\Controller\ComponentRegistry; +use Cake\Core\Configure; +use Cake\Network\Exception\UnauthorizedException; use Cake\Network\Request; /** @@ -69,11 +71,12 @@ class DigestAuthenticate extends BasicAuthenticate * Besides the keys specified in BaseAuthenticate::$_defaultConfig, * DigestAuthenticate uses the following extra keys: * + * - `secret` The secret to use for nonce validation. Defaults to Security.salt. * - `realm` The realm authentication is for, Defaults to the servername. - * - `nonce` A nonce used for authentication. Defaults to `uniqid()`. * - `qop` Defaults to 'auth', no other values are supported at this time. * - `opaque` A string that must be returned unchanged by clients. * Defaults to `md5($config['realm'])` + * - `nonceExpires` The number of seconds that nonces are valid for. Defaults to 300. * * @param \Cake\Controller\ComponentRegistry $registry The Component registry * used on this request. @@ -84,9 +87,10 @@ public function __construct(ComponentRegistry $registry, array $config = []) $this->_registry = $registry; $this->config([ + 'nonceExpires' => 300, + 'secret' => Configure::read('Security.salt'), 'realm' => null, 'qop' => 'auth', - 'nonce' => uniqid(''), 'opaque' => null, ]); @@ -111,6 +115,10 @@ public function getUser(Request $request) return false; } + if (!$this->validNonce($digest['nonce'])) { + return false; + } + $field = $this->_config['fields']['password']; $password = $user[$field]; unset($user[$field]); @@ -215,15 +223,65 @@ public function loginHeaders(Request $request) $options = [ 'realm' => $realm, 'qop' => $this->_config['qop'], - 'nonce' => $this->_config['nonce'], + 'nonce' => $this->generateNonce(), 'opaque' => $this->_config['opaque'] ?: md5($realm) ]; + $digest = $this->_getDigest($request); + if ($digest && isset($digest['nonce'])) { + if (!$this->validNonce($digest['nonce'])) { + $options['stale'] = true; + } + } + $opts = []; foreach ($options as $k => $v) { - $opts[] = sprintf('%s="%s"', $k, $v); + if (is_bool($v)) { + $v = $v ? 'true' : 'false'; + $opts[] = sprintf('%s=%s', $k, $v); + } else { + $opts[] = sprintf('%s="%s"', $k, $v); + } } return 'WWW-Authenticate: Digest ' . implode(',', $opts); } + + /** + * Generate a nonce value that is validated in future requests. + * + * @return string + */ + protected function generateNonce() + { + $expiryTime = microtime(true) + $this->config('nonceExpires'); + $signatureValue = md5($expiryTime . ':' . $this->config('secret')); + $nonceValue = $expiryTime . ':' . $signatureValue; + + return base64_encode($nonceValue); + } + + /** + * Check the nonce to ensure it is valid and not expired. + * + * @param string $nonce The nonce value to check. + * @return bool + */ + protected function validNonce($nonce) + { + $value = base64_decode($nonce); + if ($value === false) { + return false; + } + $parts = explode(':', $value); + if (count($parts) !== 2) { + return false; + } + list($expires, $checksum) = $parts; + if ($expires < microtime(true)) { + return false; + } + + return md5($expires . ':' . $this->config('secret')) === $checksum; + } } diff --git a/tests/TestCase/Auth/DigestAuthenticateTest.php b/tests/TestCase/Auth/DigestAuthenticateTest.php index a1a56665c99..30abca0775e 100644 --- a/tests/TestCase/Auth/DigestAuthenticateTest.php +++ b/tests/TestCase/Auth/DigestAuthenticateTest.php @@ -18,6 +18,7 @@ use Cake\Auth\DigestAuthenticate; use Cake\Controller\ComponentRegistry; +use Cake\Core\Configure; use Cake\I18n\Time; use Cake\Network\Exception\UnauthorizedException; use Cake\Network\Request; @@ -106,18 +107,17 @@ public function testAuthenticateWrongUsername() $request = new Request('posts/index'); $request->addParams(['pass' => []]); - $digest = <<env('PHP_AUTH_DIGEST', $digest); + $data = [ + 'username' => 'incorrect_user', + 'realm' => 'localhost', + 'nonce' => $this->generateNonce(), + 'uri' => '/dir/index.html', + 'qop' => 'auth', + 'nc' => 0000001, + 'cnonce' => '0a4f113b' + ]; + $data['response'] = $this->auth->generateResponseHash($data, '09faa9931501bf30f0d4253fa7763022', 'GET'); + $request->env('PHP_AUTH_DIGEST', $this->digestHeader($data)); $this->auth->unauthenticated($request, $this->response); } @@ -142,8 +142,97 @@ public function testAuthenticateChallenge() $this->assertNotEmpty($e); - $expected = ['WWW-Authenticate: Digest realm="localhost",qop="auth",nonce="123",opaque="123abc"']; - $this->assertEquals($expected, $e->responseHeader()); + $header = $e->responseHeader()[0]; + $this->assertRegexp( + '/^WWW\-Authenticate: Digest realm="localhost",qop="auth",nonce="[a-zA-Z0-9=]+",opaque="123abc"$/', + $e->responseHeader()[0] + ); + } + + /** + * test that challenge headers include stale when the nonce is stale + * + * @return void + */ + public function testAuthenticateChallengeIncludesStaleAttributeOnStaleNonce() + { + $request = new Request([ + 'url' => 'posts/index', + 'environment' => ['REQUEST_METHOD' => 'GET'] + ]); + $request->addParams(['pass' => []]); + $data = [ + 'uri' => '/dir/index.html', + 'nonce' => $this->generateNonce(null, 5, strtotime('-10 minutes')), + 'nc' => 1, + 'cnonce' => '123', + 'qop' => 'auth', + ]; + $data['response'] = $this->auth->generateResponseHash($data, '09faa9931501bf30f0d4253fa7763022', 'GET'); + $request->env('PHP_AUTH_DIGEST', $this->digestHeader($data)); + + try { + $this->auth->unauthenticated($request, $this->response); + } catch (UnauthorizedException $e) { + } + $this->assertNotEmpty($e); + + $header = $e->responseHeader()[0]; + $this->assertContains('stale=true', $header); + } + + /** + * Test that authentication fails when a nonce is stale + * + * @return void + */ + public function testAuthenticateFailsOnStaleNonce() + { + $request = new Request([ + 'url' => 'posts/index', + 'environment' => ['REQUEST_METHOD' => 'GET'] + ]); + $request->addParams(['pass' => []]); + + $data = [ + 'uri' => '/dir/index.html', + 'nonce' => $this->generateNonce(null, 5, strtotime('-10 minutes')), + 'nc' => 1, + 'cnonce' => '123', + 'qop' => 'auth', + ]; + $data['response'] = $this->auth->generateResponseHash($data, '09faa9931501bf30f0d4253fa7763022', 'GET'); + $request->env('PHP_AUTH_DIGEST', $this->digestHeader($data)); + $result = $this->auth->authenticate($request, $this->response); + $this->assertFalse($result, 'Stale nonce should fail'); + } + + /** + * Test that nonces are required. + * + * @return void + */ + public function testAuthenticateValidUsernamePasswordNoNonce() + { + $request = new Request([ + 'url' => 'posts/index', + 'environment' => ['REQUEST_METHOD' => 'GET'] + ]); + $request->addParams(['pass' => []]); + + $data = [ + 'username' => 'mariano', + 'realm' => 'localhos', + 'uri' => '/dir/index.html', + 'nonce' => '', + 'nc' => 1, + 'cnonce' => '123', + 'qop' => 'auth', + ]; + $data['response'] = $this->auth->generateResponseHash($data, '09faa9931501bf30f0d4253fa7763022', 'GET'); + $request->env('PHP_AUTH_DIGEST', $this->digestHeader($data)); + $result = $this->auth->authenticate($request, $this->response); + $this->assertFalse($result, 'Empty nonce should fail'); } /** @@ -159,18 +248,15 @@ public function testAuthenticateSuccess() ]); $request->addParams(['pass' => []]); - $digest = <<env('PHP_AUTH_DIGEST', $digest); + $data = [ + 'uri' => '/dir/index.html', + 'nonce' => $this->generateNonce(), + 'nc' => 1, + 'cnonce' => '123', + 'qop' => 'auth', + ]; + $data['response'] = $this->auth->generateResponseHash($data, '09faa9931501bf30f0d4253fa7763022', 'GET'); + $request->env('PHP_AUTH_DIGEST', $this->digestHeader($data)); $result = $this->auth->authenticate($request, $this->response); $expected = [ @@ -196,18 +282,16 @@ public function testAuthenticateSuccessSimulatedRequestMethod() ]); $request->addParams(['pass' => []]); - $digest = <<env('PHP_AUTH_DIGEST', $digest); + $data = [ + 'username' => 'mariano', + 'uri' => '/dir/index.html', + 'nonce' => $this->generateNonce(), + 'nc' => 1, + 'cnonce' => '123', + 'qop' => 'auth', + ]; + $data['response'] = $this->auth->generateResponseHash($data, '09faa9931501bf30f0d4253fa7763022', 'GET'); + $request->env('PHP_AUTH_DIGEST', $this->digestHeader($data)); $result = $this->auth->authenticate($request, $this->response); $expected = [ @@ -235,19 +319,16 @@ public function testAuthenticateFailReChallenge() ]); $request->addParams(['pass' => []]); - $digest = <<env('PHP_AUTH_DIGEST', $digest); - + $data = [ + 'username' => 'invalid', + 'uri' => '/dir/index.html', + 'nonce' => $this->generateNonce(), + 'nc' => 1, + 'cnonce' => '123', + 'qop' => 'auth', + ]; + $data['response'] = $this->auth->generateResponseHash($data, '09faa9931501bf30f0d4253fa7763022', 'GET'); + $request->env('PHP_AUTH_DIGEST', $this->digestHeader($data)); $this->auth->unauthenticated($request, $this->response); } @@ -263,11 +344,13 @@ public function testLoginHeaders() ]); $this->auth = new DigestAuthenticate($this->Collection, [ 'realm' => 'localhost', - 'nonce' => '123' ]); - $expected = 'WWW-Authenticate: Digest realm="localhost",qop="auth",nonce="123",opaque="421aa90e079fa326b6494f812ad13e79"'; $result = $this->auth->loginHeaders($request); - $this->assertEquals($expected, $result); + + $this->assertRegexp( + '/^WWW\-Authenticate: Digest realm="localhost",qop="auth",nonce="[a-zA-Z0-9=]+",opaque="[a-f0-9]+"$/', + $result + ); } /** @@ -374,4 +457,48 @@ public function testPassword() $expected = md5('mark:localhost:password'); $this->assertEquals($expected, $result); } + + /** + * Generate a nonce for testing. + * + * @param string $secret The secret to use. + * @param int $expires Time to live + * @return string + */ + protected function generateNonce($secret = null, $expires = 300, $time = null) + { + $secret = $secret ?: Configure::read('Security.salt'); + $time = $time ?: microtime(true); + $expiryTime = $time + $expires; + $signatureValue = md5($expiryTime . ':' . $secret); + $nonceValue = $expiryTime . ':' . $signatureValue; + return base64_encode($nonceValue); + } + + /** + * Create a digest header string from an array of data. + * + * @param array $data the data to convert into a header. + * @return string + */ + protected function digestHeader($data) + { + $data += [ + 'username' => 'mariano', + 'realm' => 'localhost', + 'opaque' => '123abc' + ]; + $digest = <<