From c0675863681aab029123e9c5b7df54c4ed594c3b Mon Sep 17 00:00:00 2001 From: Vincent Simonin Date: Fri, 7 Sep 2012 17:02:54 +0200 Subject: [PATCH 1/5] [Security] Fixed digest authentication Digest authentication fail if digest parameters contains `=` character or `, ` string. --- .../Firewall/DigestAuthenticationListener.php | 11 +- .../Security/Http/Firewall/DigestDataTest.php | 110 ++++++++++++++++++ 2 files changed, 116 insertions(+), 5 deletions(-) create mode 100644 tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php diff --git a/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php index 5c529dab02..8567a0005c 100644 --- a/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php @@ -141,11 +141,12 @@ class DigestData public function __construct($header) { $this->header = $header; - $parts = preg_split('/, /', $header); + preg_match_all('/(\w+)=("([^"]+)"|([^\s,$]+))/', $header, $matches, PREG_SET_ORDER); $this->elements = array(); - foreach ($parts as $part) { - list($key, $value) = explode('=', $part); - $this->elements[$key] = '"' === $value[0] ? substr($value, 1, -1) : $value; + foreach ($matches as $match) { + if (isset($match[1]) && isset($match[3])) { + $this->elements[$match[1]] = isset($match[4]) ? $match[4] : $match[3]; + } } } @@ -188,7 +189,7 @@ class DigestData $this->nonceExpiryTime = $nonceTokens[0]; if (md5($this->nonceExpiryTime.':'.$entryPointKey) !== $nonceTokens[1]) { - new BadCredentialsException(sprintf('Nonce token compromised "%s".', $nonceAsPlainText)); + throw new BadCredentialsException(sprintf('Nonce token compromised "%s".', $nonceAsPlainText)); } } diff --git a/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php b/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php new file mode 100644 index 0000000000..cd64f44e94 --- /dev/null +++ b/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php @@ -0,0 +1,110 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Tests\Http\Firewall; + +use Symfony\Component\Security\Http\Firewall\DigestData; + +class DigestDataTest extends \PHPUnit_Framework_TestCase +{ + public function setUp() + { + class_exists('Symfony\Component\Security\Http\Firewall\DigestAuthenticationListener', true); + } + + public function testGetResponse() + { + $digestAuth = new DigestData( + 'username="user", realm="Welcome, robot!", ' . + 'nonce="MTM0NzMyMTgyMy42NzkzOmRlZjM4NmIzOGNjMjE0OWJiNDU0MDAxNzJmYmM1MmZl", ' . + 'uri="/path/info?p1=5&p2=5", cnonce="MDIwODkz", nc=00000001, qop="auth", ' . + 'response="b52938fc9e6d7c01be7702ece9031b42"' + ); + + $this->assertEquals('b52938fc9e6d7c01be7702ece9031b42', $digestAuth->getResponse()); + } + + public function testGetUsername() + { + $digestAuth = new DigestData( + 'username="user", realm="Welcome, robot!", ' . + 'nonce="MTM0NzMyMTgyMy42NzkzOmRlZjM4NmIzOGNjMjE0OWJiNDU0MDAxNzJmYmM1MmZl", ' . + 'uri="/path/info?p1=5&p2=5", cnonce="MDIwODkz", nc=00000001, qop="auth", ' . + 'response="b52938fc9e6d7c01be7702ece9031b42"' + ); + + $this->assertEquals('user', $digestAuth->getUsername()); + } + + public function testValidateAndDecode() + { + $time = microtime(true); + $key = 'ThisIsAKey'; + $nonce = base64_encode($time . ':' . md5($time . ':' . $key)); + + $digestAuth = new DigestData( + 'username="user", realm="Welcome, robot!", nonce="' . $nonce . '", ' . + 'uri="/path/info?p1=5&p2=5", cnonce="MDIwODkz", nc=00000001, qop="auth", ' . + 'response="b52938fc9e6d7c01be7702ece9031b42"' + ); + + try { + $digestAuth->validateAndDecode($key, 'Welcome, robot!'); + } catch (\Exception $e) { + $this->fail(sprintf('testValidateAndDecode fail with message: %s', $e->getMessage())); + } + } + + public function testCalculateServerDigest() + { + $username = 'user'; + $realm = 'Welcome, robot!'; + $password = 'pass,word=password'; + $time = microtime(true); + $key = 'ThisIsAKey'; + $nonce = base64_encode($time . ':' . md5($time . ':' . $key)); + $nc = '00000001'; + $cnonce = 'MDIwODkz'; + $qop = 'auth'; + $method = 'GET'; + $uri = '/path/info?p1=5&p2=5'; + + $response = md5( + md5($username . ':' . $realm . ':' . $password) . + ':' . $nonce . ':' . $nc . ':' . $cnonce . ':' . $qop . ':' . md5($method . ':' . $uri) + ); + + $digest = sprintf('username="%s", realm="%s", nonce="%s", uri="%s", cnonce="%s", nc="%s", qop="%s", response="%s"', + $username, $realm, $nonce, $uri, $cnonce, $nc, $qop, $response + ); + + $digestAuth = new DigestData($digest); + + $this->assertEquals($digestAuth->getResponse(), $digestAuth->calculateServerDigest($password, $method)); + } + + public function testIsNonceExpired() + { + $time = microtime(true) + 10; + $key = 'ThisIsAKey'; + $nonce = base64_encode($time . ':' . md5($time . ':' . $key)); + + $digestAuth = new DigestData( + 'username="user", realm="Welcome, robot!", nonce="' . $nonce . '", ' . + 'uri="/path/info?p1=5&p2=5", cnonce="MDIwODkz", nc=00000001, qop="auth", ' . + 'response="b52938fc9e6d7c01be7702ece9031b42"' + ); + + $digestAuth->validateAndDecode($key, 'Welcome, robot!'); + + $this->assertFalse($digestAuth->isNonceExpired()); + } +} \ No newline at end of file From 694697dd915247dc29d66674bfe95161c9755ccc Mon Sep 17 00:00:00 2001 From: Vincent Simonin Date: Mon, 17 Sep 2012 17:24:25 +0200 Subject: [PATCH 2/5] [Security] Fixed digest authentication Digest authentication fail if digest parameters contains `=` character or `, ` string. * Support escaped characters --- .../Firewall/DigestAuthenticationListener.php | 2 +- .../Security/Http/Firewall/DigestDataTest.php | 36 ++++++++++++------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php index 8567a0005c..b679f4d251 100644 --- a/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php @@ -141,7 +141,7 @@ class DigestData public function __construct($header) { $this->header = $header; - preg_match_all('/(\w+)=("([^"]+)"|([^\s,$]+))/', $header, $matches, PREG_SET_ORDER); + preg_match_all('/(\w+)=("((?:[^"\\\\]|\\\\.)+)"|([^\s,$]+))/', $header, $matches, PREG_SET_ORDER); $this->elements = array(); foreach ($matches as $match) { if (isset($match[1]) && isset($match[3])) { diff --git a/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php b/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php index cd64f44e94..df96470d3b 100644 --- a/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php +++ b/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php @@ -44,6 +44,18 @@ class DigestDataTest extends \PHPUnit_Framework_TestCase $this->assertEquals('user', $digestAuth->getUsername()); } + public function testGetUsernameWithQuote() + { + $digestAuth = new DigestData( + 'username="\"user\"", realm="Welcome, robot!", ' . + 'nonce="MTM0NzMyMTgyMy42NzkzOmRlZjM4NmIzOGNjMjE0OWJiNDU0MDAxNzJmYmM1MmZl", ' . + 'uri="/path/info?p1=5&p2=5", cnonce="MDIwODkz", nc=00000001, qop="auth", ' . + 'response="b52938fc9e6d7c01be7702ece9031b42"' + ); + + $this->assertEquals('\"user\"', $digestAuth->getUsername()); + } + public function testValidateAndDecode() { $time = microtime(true); @@ -65,24 +77,24 @@ class DigestDataTest extends \PHPUnit_Framework_TestCase public function testCalculateServerDigest() { - $username = 'user'; - $realm = 'Welcome, robot!'; - $password = 'pass,word=password'; + $this->calculateServerDigest('user', 'Welcome, robot!', 'pass,word=password', 'ThisIsAKey', '00000001', 'MDIwODkz', 'auth', 'GET', '/path/info?p1=5&p2=5'); + } + + public function testCalculateServerDigestWithQuote() + { + $this->calculateServerDigest('\"user\"', 'Welcome, \"robot\"!', 'pass,word=password', 'ThisIsAKey', '00000001', 'MDIwODkz', 'auth', 'GET', '/path/info?p1=5&p2=5'); + } + + private function calculateServerDigest($username, $realm, $password, $key, $nc, $cnonce, $qop, $method, $uri) + { $time = microtime(true); - $key = 'ThisIsAKey'; $nonce = base64_encode($time . ':' . md5($time . ':' . $key)); - $nc = '00000001'; - $cnonce = 'MDIwODkz'; - $qop = 'auth'; - $method = 'GET'; - $uri = '/path/info?p1=5&p2=5'; $response = md5( - md5($username . ':' . $realm . ':' . $password) . - ':' . $nonce . ':' . $nc . ':' . $cnonce . ':' . $qop . ':' . md5($method . ':' . $uri) + md5($username . ':' . $realm . ':' . $password) . ':' . $nonce . ':' . $nc . ':' . $cnonce . ':' . $qop . ':' . md5($method . ':' . $uri) ); - $digest = sprintf('username="%s", realm="%s", nonce="%s", uri="%s", cnonce="%s", nc="%s", qop="%s", response="%s"', + $digest = sprintf('username="%s", realm="%s", nonce="%s", uri="%s", cnonce="%s", nc=%s, qop="%s", response="%s"', $username, $realm, $nonce, $uri, $cnonce, $nc, $qop, $response ); From d66b03c8308e5e9d9c654014e36a3a3b77a93745 Mon Sep 17 00:00:00 2001 From: Sebastiaan Stok Date: Sun, 28 Oct 2012 10:56:34 +0100 Subject: [PATCH 3/5] fixed CS --- .../Security/Http/Firewall/DigestDataTest.php | 46 +++++++++---------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php b/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php index df96470d3b..82f3215dea 100644 --- a/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php +++ b/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php @@ -15,11 +15,6 @@ use Symfony\Component\Security\Http\Firewall\DigestData; class DigestDataTest extends \PHPUnit_Framework_TestCase { - public function setUp() - { - class_exists('Symfony\Component\Security\Http\Firewall\DigestAuthenticationListener', true); - } - public function testGetResponse() { $digestAuth = new DigestData( @@ -85,6 +80,28 @@ class DigestDataTest extends \PHPUnit_Framework_TestCase $this->calculateServerDigest('\"user\"', 'Welcome, \"robot\"!', 'pass,word=password', 'ThisIsAKey', '00000001', 'MDIwODkz', 'auth', 'GET', '/path/info?p1=5&p2=5'); } + public function testIsNonceExpired() + { + $time = microtime(true) + 10; + $key = 'ThisIsAKey'; + $nonce = base64_encode($time . ':' . md5($time . ':' . $key)); + + $digestAuth = new DigestData( + 'username="user", realm="Welcome, robot!", nonce="' . $nonce . '", ' . + 'uri="/path/info?p1=5&p2=5", cnonce="MDIwODkz", nc=00000001, qop="auth", ' . + 'response="b52938fc9e6d7c01be7702ece9031b42"' + ); + + $digestAuth->validateAndDecode($key, 'Welcome, robot!'); + + $this->assertFalse($digestAuth->isNonceExpired()); + } + + protected function setUp() + { + class_exists('Symfony\Component\Security\Http\Firewall\DigestAuthenticationListener', true); + } + private function calculateServerDigest($username, $realm, $password, $key, $nc, $cnonce, $qop, $method, $uri) { $time = microtime(true); @@ -102,21 +119,4 @@ class DigestDataTest extends \PHPUnit_Framework_TestCase $this->assertEquals($digestAuth->getResponse(), $digestAuth->calculateServerDigest($password, $method)); } - - public function testIsNonceExpired() - { - $time = microtime(true) + 10; - $key = 'ThisIsAKey'; - $nonce = base64_encode($time . ':' . md5($time . ':' . $key)); - - $digestAuth = new DigestData( - 'username="user", realm="Welcome, robot!", nonce="' . $nonce . '", ' . - 'uri="/path/info?p1=5&p2=5", cnonce="MDIwODkz", nc=00000001, qop="auth", ' . - 'response="b52938fc9e6d7c01be7702ece9031b42"' - ); - - $digestAuth->validateAndDecode($key, 'Welcome, robot!'); - - $this->assertFalse($digestAuth->isNonceExpired()); - } -} \ No newline at end of file +} From 80f6992a4146472211fc72fb348f966b91a6d3e8 Mon Sep 17 00:00:00 2001 From: Sebastiaan Stok Date: Sun, 28 Oct 2012 11:58:35 +0100 Subject: [PATCH 4/5] [Security] added test extra for digest authentication --- .../Security/Http/Firewall/DigestDataTest.php | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php b/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php index 82f3215dea..d201c8f6df 100644 --- a/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php +++ b/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php @@ -51,6 +51,42 @@ class DigestDataTest extends \PHPUnit_Framework_TestCase $this->assertEquals('\"user\"', $digestAuth->getUsername()); } + public function testGetUsernameWithQuoteAndEscape() + { + $digestAuth = new DigestData( + 'username="\"u\\\\\"ser\"", realm="Welcome, robot!", ' . + 'nonce="MTM0NzMyMTgyMy42NzkzOmRlZjM4NmIzOGNjMjE0OWJiNDU0MDAxNzJmYmM1MmZl", ' . + 'uri="/path/info?p1=5&p2=5", cnonce="MDIwODkz", nc=00000001, qop="auth", ' . + 'response="b52938fc9e6d7c01be7702ece9031b42"' + ); + + $this->assertEquals('\"u\\\\\"ser\"', $digestAuth->getUsername()); + } + + public function testGetUsernameWithSingleQuote() + { + $digestAuth = new DigestData( + 'username="\"u\'ser\"", realm="Welcome, robot!", ' . + 'nonce="MTM0NzMyMTgyMy42NzkzOmRlZjM4NmIzOGNjMjE0OWJiNDU0MDAxNzJmYmM1MmZl", ' . + 'uri="/path/info?p1=5&p2=5", cnonce="MDIwODkz", nc=00000001, qop="auth", ' . + 'response="b52938fc9e6d7c01be7702ece9031b42"' + ); + + $this->assertEquals('\"u\'ser\"', $digestAuth->getUsername()); + } + + public function testGetUsernameWithEscape() + { + $digestAuth = new DigestData( + 'username="\"u\\ser\"", realm="Welcome, robot!", ' . + 'nonce="MTM0NzMyMTgyMy42NzkzOmRlZjM4NmIzOGNjMjE0OWJiNDU0MDAxNzJmYmM1MmZl", ' . + 'uri="/path/info?p1=5&p2=5", cnonce="MDIwODkz", nc=00000001, qop="auth", ' . + 'response="b52938fc9e6d7c01be7702ece9031b42"' + ); + + $this->assertEquals('\"u\\ser\"', $digestAuth->getUsername()); + } + public function testValidateAndDecode() { $time = microtime(true); @@ -80,6 +116,17 @@ class DigestDataTest extends \PHPUnit_Framework_TestCase $this->calculateServerDigest('\"user\"', 'Welcome, \"robot\"!', 'pass,word=password', 'ThisIsAKey', '00000001', 'MDIwODkz', 'auth', 'GET', '/path/info?p1=5&p2=5'); } + public function testCalculateServerDigestWithQuoteAndEscape() + { + $this->calculateServerDigest('\"u\\\\\"ser\"', 'Welcome, \"robot\"!', 'pass,word=password', 'ThisIsAKey', '00000001', 'MDIwODkz', 'auth', 'GET', '/path/info?p1=5&p2=5'); + } + + public function testCalculateServerDigestEscape() + { + $this->calculateServerDigest('\"u\\ser\"', 'Welcome, \"robot\"!', 'pass,word=password', 'ThisIsAKey', '00000001', 'MDIwODkz', 'auth', 'GET', '/path/info?p1=5&p2=5'); + $this->calculateServerDigest('\"u\\ser\\\\\"', 'Welcome, \"robot\"!', 'pass,word=password', 'ThisIsAKey', '00000001', 'MDIwODkz', 'auth', 'GET', '/path/info?p1=5&p2=5'); + } + public function testIsNonceExpired() { $time = microtime(true) + 10; From f2cbea3b309c52951267f5f05b3842cf1b502dd4 Mon Sep 17 00:00:00 2001 From: Sebastiaan Stok Date: Thu, 15 Nov 2012 16:54:04 +0100 Subject: [PATCH 5/5] [Security] remove escape charters from username provided by Digest DigestAuthenticationListener --- .../Firewall/DigestAuthenticationListener.php | 2 +- .../Security/Http/Firewall/DigestDataTest.php | 20 +++++++++++++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php b/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php index b679f4d251..2bc4aa550f 100644 --- a/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php +++ b/src/Symfony/Component/Security/Http/Firewall/DigestAuthenticationListener.php @@ -157,7 +157,7 @@ class DigestData public function getUsername() { - return $this->elements['username']; + return strtr($this->elements['username'], array("\\\"" => "\"", "\\\\" => "\\")); } public function validateAndDecode($entryPointKey, $expectedRealm) diff --git a/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php b/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php index d201c8f6df..cfb929cacc 100644 --- a/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php +++ b/tests/Symfony/Tests/Component/Security/Http/Firewall/DigestDataTest.php @@ -48,7 +48,7 @@ class DigestDataTest extends \PHPUnit_Framework_TestCase 'response="b52938fc9e6d7c01be7702ece9031b42"' ); - $this->assertEquals('\"user\"', $digestAuth->getUsername()); + $this->assertEquals('"user"', $digestAuth->getUsername()); } public function testGetUsernameWithQuoteAndEscape() @@ -60,7 +60,7 @@ class DigestDataTest extends \PHPUnit_Framework_TestCase 'response="b52938fc9e6d7c01be7702ece9031b42"' ); - $this->assertEquals('\"u\\\\\"ser\"', $digestAuth->getUsername()); + $this->assertEquals('"u\\"ser"', $digestAuth->getUsername()); } public function testGetUsernameWithSingleQuote() @@ -72,7 +72,19 @@ class DigestDataTest extends \PHPUnit_Framework_TestCase 'response="b52938fc9e6d7c01be7702ece9031b42"' ); - $this->assertEquals('\"u\'ser\"', $digestAuth->getUsername()); + $this->assertEquals('"u\'ser"', $digestAuth->getUsername()); + } + + public function testGetUsernameWithSingleQuoteAndEscape() + { + $digestAuth = new DigestData( + 'username="\"u\\\'ser\"", realm="Welcome, robot!", ' . + 'nonce="MTM0NzMyMTgyMy42NzkzOmRlZjM4NmIzOGNjMjE0OWJiNDU0MDAxNzJmYmM1MmZl", ' . + 'uri="/path/info?p1=5&p2=5", cnonce="MDIwODkz", nc=00000001, qop="auth", ' . + 'response="b52938fc9e6d7c01be7702ece9031b42"' + ); + + $this->assertEquals('"u\\\'ser"', $digestAuth->getUsername()); } public function testGetUsernameWithEscape() @@ -84,7 +96,7 @@ class DigestDataTest extends \PHPUnit_Framework_TestCase 'response="b52938fc9e6d7c01be7702ece9031b42"' ); - $this->assertEquals('\"u\\ser\"', $digestAuth->getUsername()); + $this->assertEquals('"u\\ser"', $digestAuth->getUsername()); } public function testValidateAndDecode()