Skip to content
This repository has been archived by the owner on Dec 7, 2019. It is now read-only.

Commit

Permalink
Security fix for OpenID
Browse files Browse the repository at this point in the history
  • Loading branch information
ezimuel authored and weierophinney committed Mar 5, 2014
1 parent cde674b commit 709789c
Show file tree
Hide file tree
Showing 6 changed files with 165 additions and 24 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
composer.lock
vendor/
8 changes: 4 additions & 4 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@
],
"require": {
"php": ">=5.3.3",
"zendframework/zend-http": ">=2.0.0",
"zendframework/zend-session": ">=2.0.0",
"zendframework/zend-eventmanager": ">=2.0.0"
"zendframework/zend-http": "~2.0",
"zendframework/zend-session": "~2.0",
"zendframework/zend-eventmanager": "~2.0"
},
"require-dev": {
"zendframework/zendframework": ">=2.0.0"
"zendframework/zendframework": "~2.0"
},
"extra": {
"branch-alias": {
Expand Down
23 changes: 23 additions & 0 deletions library/ZendOpenId/Consumer/GenericConsumer.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@
class GenericConsumer
{

/**
* Parameters required for signature
*/
protected $_signParams = array('op_endpoint', 'return_to', 'response_nonce', 'assoc_handle');

/**
* Reference to an implementation of storage object
*
Expand Down Expand Up @@ -265,7 +270,25 @@ public function verify($params, &$identity = "", $extensions = null)
$macFunc,
$secret,
$expires)) {
// Security fix - check the association bewteen op_endpoint and assoc_handle
if (isset($params['openid_op_endpoint']) && $url !== $params['openid_op_endpoint']) {
$this->_setError("The op_endpoint URI is not the same of URI associated with the assoc_handle");
return false;
}
$signed = explode(',', $params['openid_signed']);
// Check the parameters for the signature
// @see https://openid.net/specs/openid-authentication-2_0.html#positive_assertions
$toCheck = $this->_signParams;
if (isset($params['openid_claimed_id']) && isset($params['openid_identity'])) {
$toCheck = array_merge($toCheck, array('claimed_id', 'identity'));
}
foreach ($toCheck as $param) {
if (!in_array($param, $signed, true)) {
$this->_setError("The required parameter $param is missing in the signed");
return false;
}
}

$data = '';
foreach ($signed as $key) {
$data .= $key . ':' . $params['openid_' . strtr($key,'.','_')] . "\n";
Expand Down
78 changes: 58 additions & 20 deletions tests/ZendOpenId/ConsumerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ public function testHttpRequest()
$this->assertSame( "GET / HTTP/1.1\r\n" .
"Host: www.myopenid.com\r\n" .
"Connection: close\r\n" .
"Accept-encoding: gzip, deflate\r\n" .
"Accept-Encoding: gzip, deflate\r\n" .
"User-Agent: Zend_OpenId\r\n\r\n",
$http->getLastRawRequest() );

Expand All @@ -304,7 +304,7 @@ public function testHttpRequest()
$this->assertSame( "POST / HTTP/1.1\r\n" .
"Host: www.myopenid.com\r\n" .
"Connection: close\r\n" .
"Accept-encoding: gzip, deflate\r\n" .
"Accept-Encoding: gzip, deflate\r\n" .
"User-Agent: Zend_OpenId\r\n" .
"Content-Type: application/x-www-form-urlencoded\r\n\r\n",
$http->getLastRawRequest() );
Expand All @@ -314,7 +314,7 @@ public function testHttpRequest()
$this->assertSame( "GET /test.php?a=b&c=d HTTP/1.1\r\n" .
"Host: www.myopenid.com\r\n" .
"Connection: close\r\n" .
"Accept-encoding: gzip, deflate\r\n" .
"Accept-Encoding: gzip, deflate\r\n" .
"User-Agent: Zend_OpenId\r\n\r\n",
$http->getLastRawRequest() );

Expand All @@ -323,7 +323,7 @@ public function testHttpRequest()
$this->assertSame( "POST /test.php HTTP/1.1\r\n" .
"Host: www.myopenid.com\r\n" .
"Connection: close\r\n" .
"Accept-encoding: gzip, deflate\r\n" .
"Accept-Encoding: gzip, deflate\r\n" .
"User-Agent: Zend_OpenId\r\n" .
"Content-Type: application/x-www-form-urlencoded\r\n" .
"Content-Length: 7\r\n\r\n" .
Expand All @@ -335,7 +335,7 @@ public function testHttpRequest()
$this->assertSame( "GET /test.php?a=b&c=x+y HTTP/1.1\r\n" .
"Host: www.myopenid.com\r\n" .
"Connection: close\r\n" .
"Accept-encoding: gzip, deflate\r\n" .
"Accept-Encoding: gzip, deflate\r\n" .
"User-Agent: Zend_OpenId\r\n\r\n",
$http->getLastRawRequest() );

Expand All @@ -344,7 +344,7 @@ public function testHttpRequest()
$this->assertSame( "POST /test.php?a=b HTTP/1.1\r\n" .
"Host: www.myopenid.com\r\n" .
"Connection: close\r\n" .
"Accept-encoding: gzip, deflate\r\n" .
"Accept-Encoding: gzip, deflate\r\n" .
"User-Agent: Zend_OpenId\r\n" .
"Content-Type: application/x-www-form-urlencoded\r\n" .
"Content-Length: 5\r\n\r\n" .
Expand Down Expand Up @@ -378,7 +378,7 @@ public function testAssociate()
$this->assertSame( "POST / HTTP/1.1\r\n" .
"Host: www.myopenid.com\r\n" .
"Connection: close\r\n" .
"Accept-encoding: gzip, deflate\r\n" .
"Accept-Encoding: gzip, deflate\r\n" .
"User-Agent: Zend_OpenId\r\n" .
"Content-Type: application/x-www-form-urlencoded\r\n" .
"Content-Length: 510\r\n\r\n" .
Expand All @@ -396,7 +396,7 @@ public function testAssociate()
$this->assertSame( "POST / HTTP/1.1\r\n" .
"Host: www.myopenid.com\r\n" .
"Connection: close\r\n" .
"Accept-encoding: gzip, deflate\r\n" .
"Accept-Encoding: gzip, deflate\r\n" .
"User-Agent: Zend_OpenId\r\n" .
"Content-Type: application/x-www-form-urlencoded\r\n" .
"Content-Length: 567\r\n\r\n" .
Expand Down Expand Up @@ -774,8 +774,9 @@ public function testVerify()
"openid_identity" => self::REAL_ID,
"openid_response_nonce" => "2007-08-14T12:52:33Z46c1a59124ffe",
"openid_mode" => "id_res",
"openid_signed" => "assoc_handle,return_to,claimed_id,identity,response_nonce,mode,signed",
"openid_sig" => "rMiVhEmHVcIHoY2uzPNb7udWqa4lruvjnwZfujct0TE="
"openid_op_endpoint" => self::SERVER,
"openid_signed" => "op_endpoint,assoc_handle,return_to,claimed_id,identity,response_nonce,mode,signed",
"openid_sig" => "4KFaoZApYmYq6aFdIGzzgbsIiaA="
);
$storage->delAssociation(self::SERVER);
$storage->addAssociation(self::SERVER, self::HANDLE, "sha256", pack("H*", "ed901bc561c29fd7bb42862e5f09fa37e7944a7ee72142322f34a21bfe1384b8"), $expiresIn);
Expand All @@ -793,8 +794,8 @@ public function testVerify()
"openid_identity" => self::REAL_ID,
"openid_response_nonce" => "2007-08-14T12:52:33Z46c1a59124fff",
"openid_mode" => "id_res",
"openid_signed" => "assoc_handle,return_to,claimed_id,identity,response_nonce,mode,signed",
"openid_sig" => "h/5AFD25NpzSok5tzHEGCVUkQSw="
"openid_signed" => "op_endpoint,assoc_handle,return_to,claimed_id,identity,response_nonce,mode,signed",
"openid_sig" => "O1ycNUA75AiVnoIcdBrx/nx462lLRv4f7xO9IPRiHqU="
);
$storage->delAssociation(self::SERVER);
$storage->addAssociation(self::SERVER, self::HANDLE, "sha1", pack("H*", "8382aea922560ece833ba55fa53b7a975f597370"), $expiresIn);
Expand Down Expand Up @@ -855,10 +856,11 @@ public function testVerifyDumb()
$storage->delAssociation(self::SERVER);
$params = array(
"openid_return_to" => "http://www.zf-test.com/test.php",
"openid_op_endpoint" => self::SERVER,
"openid_assoc_handle" => self::HANDLE,
"openid_response_nonce" => "2007-08-14T12:52:33Z46c1a59124ffe",
"openid_mode" => "id_res",
"openid_signed" => "assoc_handle,return_to,response_nonce,mode,signed",
"openid_signed" => "op_endpoint,assoc_handle,return_to,claimed_id,identity,response_nonce,mode,signed",
"openid_sig" => "h/5AFD25NpzSok5tzHEGCVUkQSw="
);
$storage->purgeNonces();
Expand All @@ -869,31 +871,33 @@ public function testVerifyDumb()
$storage->delAssociation(self::SERVER);
$params = array(
"openid_return_to" => "http://www.zf-test.com/test.php",
"openid_op_endpoint" => self::SERVER,
"openid_assoc_handle" => self::HANDLE,
"openid_claimed_id" => self::ID,
"openid_identity" => self::REAL_ID,
"openid_response_nonce" => "2007-08-14T12:52:33Z46c1a59124ffe",
"openid_mode" => "id_res",
"openid_signed" => "assoc_handle,return_to,claimed_id,identity,response_nonce,mode,signed",
"openid_sig" => "h/5AFD25NpzSok5tzHEGCVUkQSw="
"openid_signed" => "op_endpoint,assoc_handle,return_to,claimed_id,identity,response_nonce,mode,signed",
"openid_sig" => "4KFaoZApYmYq6aFdIGzzgbsIiaA="
);
$storage->purgeNonces();
$this->assertFalse( $consumer->verify($params) );
$this->assertSame( "POST / HTTP/1.1\r\n" .
"Host: www.myopenid.com\r\n" .
"Connection: close\r\n" .
"Accept-encoding: gzip, deflate\r\n" .
"Accept-Encoding: gzip, deflate\r\n" .
"User-Agent: Zend_OpenId\r\n" .
"Content-Type: application/x-www-form-urlencoded\r\n" .
"Content-Length: 445\r\n\r\n" .
"Content-Length: 509\r\n\r\n" .
"openid.return_to=http%3A%2F%2Fwww.zf-test.com%2Ftest.php&" .
"openid.op_endpoint=" . urlencode(self::SERVER) . '&' .
"openid.assoc_handle=d41d8cd98f00b204e9800998ecf8427e&" .
"openid.claimed_id=http%3A%2F%2Fid.myopenid.com%2F&" .
"openid.identity=http%3A%2F%2Freal_id.myopenid.com%2F&" .
"openid.response_nonce=2007-08-14T12%3A52%3A33Z46c1a59124ffe&" .
"openid.mode=check_authentication&" .
"openid.signed=assoc_handle%2Creturn_to%2Cclaimed_id%2Cidentity%2Cresponse_nonce%2Cmode%2Csigned&" .
"openid.sig=h%2F5AFD25NpzSok5tzHEGCVUkQSw%3D",
"openid.signed=op_endpoint%2Cassoc_handle%2Creturn_to%2Cclaimed_id%2Cidentity%2Cresponse_nonce%2Cmode%2Csigned&" .
"openid.sig=4KFaoZApYmYq6aFdIGzzgbsIiaA%3D",
$http->getLastRawRequest() );

$test->setResponse("HTTP/1.1 200 OK\r\n\r\nis_valid:true");
Expand Down Expand Up @@ -936,7 +940,7 @@ public function testVerifyDumb()
$this->assertSame( "POST / HTTP/1.1\r\n" .
"Host: www.myopenid.com\r\n" .
"Connection: close\r\n" .
"Accept-encoding: gzip, deflate\r\n" .
"Accept-Encoding: gzip, deflate\r\n" .
"User-Agent: Zend_OpenId\r\n" .
"Content-Type: application/x-www-form-urlencoded\r\n" .
"Content-Length: 672\r\n\r\n" .
Expand Down Expand Up @@ -975,4 +979,38 @@ public function testVerifyDumb()
$this->assertFalse( $consumer->verify($params) );
$this->assertFalse( $storage->getAssociation(self::SERVER."1", $handle, $func, $secret, $expires) );
}

/**
* Test the required parameters for the signature
* @see https://openid.net/specs/openid-authentication-2_0.html#positive_assertions
*/
public function testSignedParams()
{
$expiresIn = time() + 600;
$_SERVER['SCRIPT_URI'] = "http://www.zf-test.com/test.php";
$storage = new Storage\File(__DIR__ . "/_files/consumer");
$consumer = new TestAsset\ConsumerHelper($storage);

$storage->addDiscoveryInfo(self::ID, self::REAL_ID, self::SERVER, 1.1, $expiresIn);

// Wrong arguments
$this->assertFalse( $consumer->verify(array()) );
// HMAC-SHA1
$consumer->clearAssociation();
$params = array(
"openid_return_to" => "http://www.zf-test.com/test.php",
"openid_assoc_handle" => self::HANDLE,
"openid_claimed_id" => self::ID,
"openid_identity" => self::REAL_ID,
"openid_response_nonce" => "2007-08-14T12:52:33Z46c1a59124ffe",
"openid_mode" => "id_res",
"openid_signed" => "assoc_handle,return_to,claimed_id,identity,response_nonce,mode,signed",
"openid_sig" => "h/5AFD25NpzSok5tzHEGCVUkQSw="
);
$storage->delAssociation(self::SERVER);
$storage->addAssociation(self::SERVER, self::HANDLE, "sha1", pack("H*", "8382aea922560ece833ba55fa53b7a975f597370"), $expiresIn);
$storage->purgeNonces();
$this->assertFalse( $consumer->verify($params) );
$this->assertEquals( "The required parameter op_endpoint is missing in the signed", $consumer->getError());
}
}
2 changes: 2 additions & 0 deletions tests/ZendOpenId/TestAsset/ConsumerHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
use Zend\Session\Container as SessionContainer;
use ZendTest\Session\TestAsset\TestManager as SessionManager;

require_once __DIR__ . '/TestManager.php';

class ConsumerHelper extends Consumer
{
public function __construct(Storage\AbstractStorage $storage = null,
Expand Down
77 changes: 77 additions & 0 deletions tests/ZendOpenId/TestAsset/TestManager.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<?php
/**
* Zend Framework (http://framework.zend.com/)
*
* @link http://github.com/zendframework/zf2 for the canonical source repository
* @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
* @license http://framework.zend.com/license/new-bsd New BSD License
*/

namespace ZendTest\Session\TestAsset;

use Zend\EventManager\EventManagerInterface;
use Zend\Session\AbstractManager;

class TestManager extends AbstractManager
{
public $started = false;

protected $configDefaultClass = 'Zend\\Session\\Configuration\\StandardConfig';
protected $storageDefaultClass = 'Zend\\Session\\Storage\\ArrayStorage';

public function start()
{
$this->started = true;
}

public function destroy()
{
$this->started = false;
}

public function stop()
{}

public function writeClose()
{
$this->started = false;
}

public function getName()
{}

public function setName($name)
{}

public function getId()
{}

public function setId($id)
{}

public function regenerateId()
{}

public function rememberMe($ttl = null)
{}

public function forgetMe()
{}


public function setValidatorChain(EventManagerInterface $chain)
{}

public function getValidatorChain()
{}

public function isValid()
{}


public function sessionExists()
{}

public function expireSessionCookie()
{}
}

0 comments on commit 709789c

Please sign in to comment.