From 40093b2fa7dde4a5f3c6806aad91b9302c232903 Mon Sep 17 00:00:00 2001 From: webchick Date: Wed, 1 Feb 2012 13:29:51 -0800 Subject: SA-CORE-2012-001 --- modules/openid/openid.inc | 23 ++++++++-- modules/openid/openid.module | 13 ++++-- modules/openid/openid.test | 77 +++++++++++++++++++++++++++++++-- modules/openid/tests/openid_test.module | 23 +++++++--- 4 files changed, 118 insertions(+), 18 deletions(-) (limited to 'modules/openid') diff --git a/modules/openid/openid.inc b/modules/openid/openid.inc index 98af518c7..9b793d368 100644 --- a/modules/openid/openid.inc +++ b/modules/openid/openid.inc @@ -617,18 +617,31 @@ function _openid_get_params($str) { * @param $fallback_prefix * An optional prefix that will be used in case no prefix is found for the * target extension namespace. + * @param $only_signed + * Return only keys that are included in the message signature in openid.sig. + * Unsigned fields may have been modified or added by other parties than the + * OpenID Provider. + * * @return * An associative array containing all the parameters in the response message * that belong to the extension. The keys are stripped from their namespace * prefix. + * * @see http://openid.net/specs/openid-authentication-2_0.html#extensions */ -function openid_extract_namespace($response, $extension_namespace, $fallback_prefix = NULL) { +function openid_extract_namespace($response, $extension_namespace, $fallback_prefix = NULL, $only_signed = FALSE) { + $signed_keys = explode(',', $response['openid.signed']); + // Find the namespace prefix. $prefix = $fallback_prefix; foreach ($response as $key => $value) { if ($value == $extension_namespace && preg_match('/^openid\.ns\.([^.]+)$/', $key, $matches)) { $prefix = $matches[1]; + if ($only_signed && !in_array('ns.' . $matches[1], $signed_keys)) { + // The namespace was defined but was not signed as required. In this + // case we do not fall back to $fallback_prefix. + $prefix = NULL; + } break; } } @@ -641,7 +654,9 @@ function openid_extract_namespace($response, $extension_namespace, $fallback_pre foreach ($response as $key => $value) { if (preg_match('/^openid\.' . $prefix . '\.(.+)$/', $key, $matches)) { $local_key = $matches[1]; - $output[$local_key] = $value; + if (!$only_signed || in_array($prefix . '.' . $local_key, $signed_keys)) { + $output[$local_key] = $value; + } } } @@ -837,8 +852,8 @@ function _openid_invalid_openid_transition($identity, $response) { // Try to extract e-mail address from Simple Registration (SREG) or // Attribute Exchanges (AX) keys. $email = ''; - $sreg_values = openid_extract_namespace($response, OPENID_NS_SREG, 'sreg'); - $ax_values = openid_extract_namespace($response, OPENID_NS_AX, 'ax'); + $sreg_values = openid_extract_namespace($response, OPENID_NS_SREG, 'sreg', TRUE); + $ax_values = openid_extract_namespace($response, OPENID_NS_AX, 'ax', TRUE); if (!empty($sreg_values['email']) && valid_email_address($sreg_values['email'])) { $email = $sreg_values['email']; } diff --git a/modules/openid/openid.module b/modules/openid/openid.module index f2847fc0d..e08d55718 100644 --- a/modules/openid/openid.module +++ b/modules/openid/openid.module @@ -185,10 +185,15 @@ function openid_form_user_register_form_alter(&$form, &$form_state) { $response = $_SESSION['openid']['response']; - // Extract Simple Registration keys from the response. - $sreg_values = openid_extract_namespace($response, OPENID_NS_SREG, 'sreg'); - // Extract Attribute Exchanges keys from the response. - $ax_values = openid_extract_namespace($response, OPENID_NS_AX, 'ax'); + // Extract Simple Registration keys from the response. We only include + // signed keys as required by OpenID Simple Registration Extension 1.0, + // section 4. + $sreg_values = openid_extract_namespace($response, OPENID_NS_SREG, 'sreg', TRUE); + // Extract Attribute Exchanges keys from the response. We only include + // signed keys. This is not required by the specification, but it is + // recommended by Google, see + // http://googlecode.blogspot.com/2011/05/security-advisory-to-websites-using.html + $ax_values = openid_extract_namespace($response, OPENID_NS_AX, 'ax', TRUE); if (!empty($sreg_values['nickname'])) { // Use the nickname returned by Simple Registration if available. diff --git a/modules/openid/openid.test b/modules/openid/openid.test index afb9068c6..9b6b1ad5f 100644 --- a/modules/openid/openid.test +++ b/modules/openid/openid.test @@ -343,17 +343,49 @@ class OpenIDFunctionalTestCase extends OpenIDWebTestCase { // Use a User-supplied Identity that is the URL of an XRDS document. $identity = url('openid-test/yadis/xrds', array('absolute' => TRUE)); - // Do not sign all mandatory fields (e.g. assoc_handle). + // Respond with an invalid signature. + variable_set('openid_test_response', array('openid.sig' => 'this-is-an-invalid-signature')); + $this->submitLoginForm($identity); + $this->assertRaw('OpenID login failed.'); + + // Do not sign the mandatory field openid.assoc_handle. variable_set('openid_test_response', array('openid.signed' => 'op_endpoint,claimed_id,identity,return_to,response_nonce')); $this->submitLoginForm($identity); $this->assertRaw('OpenID login failed.'); - // Sign all mandatory fields and some custom fields. - variable_set('openid_test_response', array('openid.foo' => 'bar', 'openid.signed' => 'op_endpoint,claimed_id,identity,return_to,response_nonce,assoc_handle,foo')); + // Sign all mandatory fields and a custom field. + $keys_to_sign = array('op_endpoint', 'claimed_id', 'identity', 'return_to', 'response_nonce', 'assoc_handle', 'foo'); + $association = new stdClass(); + $association->mac_key = variable_get('mac_key'); + $response = array( + 'openid.op_endpoint' => url('openid-test/endpoint', array('absolute' => TRUE)), + 'openid.claimed_id' => $identity, + 'openid.identity' => $identity, + 'openid.return_to' => url('openid/authenticate', array('absolute' => TRUE)), + 'openid.response_nonce' => _openid_nonce(), + 'openid.assoc_handle' => 'openid-test', + 'openid.foo' => 123, + 'openid.signed' => implode(',', $keys_to_sign), + ); + $response['openid.sig'] = _openid_signature($association, $response, $keys_to_sign); + variable_set('openid_test_response', $response); $this->submitLoginForm($identity); $this->assertNoRaw('OpenID login failed.'); - } + $this->assertFieldByName('name', '', t('No username was supplied by provider.')); + $this->assertFieldByName('mail', '', t('No e-mail address was supplied by provider.')); + // Check that unsigned SREG fields are ignored. + $response = array( + 'openid.signed' => 'op_endpoint,claimed_id,identity,return_to,response_nonce,assoc_handle,sreg.nickname', + 'openid.sreg.nickname' => 'john', + 'openid.sreg.email' => 'john@example.com', + ); + variable_set('openid_test_response', $response); + $this->submitLoginForm($identity); + $this->assertNoRaw('OpenID login failed.'); + $this->assertFieldByName('name', 'john', t('Username was supplied by provider.')); + $this->assertFieldByName('mail', '', t('E-mail address supplied by provider was ignored.')); + } } /** @@ -728,4 +760,41 @@ class OpenIDUnitTest extends DrupalWebTestCase { $this->assertEqual(openid_normalize('http://example.com/path#fragment'), 'http://example.com/path', t('openid_normalize() correctly normalized a URL with a fragment.')); } + + /** + * Test openid_extract_namespace(). + */ + function testOpenidExtractNamespace() { + $response = array( + 'openid.sreg.nickname' => 'john', + 'openid.ns.ext1' => OPENID_NS_SREG, + 'openid.ext1.nickname' => 'george', + 'openid.ext1.email' => 'george@example.com', + 'openid.ns.ext2' => 'http://example.com/ns/ext2', + 'openid.ext2.foo' => '123', + 'openid.ext2.bar' => '456', + 'openid.signed' => 'sreg.nickname,ns.ext1,ext1.email,ext2.foo', + ); + + $values = openid_extract_namespace($response, 'http://example.com/ns/dummy', NULL, FALSE); + $this->assertEqual($values, array(), t('Nothing found for unused namespace.')); + + $values = openid_extract_namespace($response, 'http://example.com/ns/dummy', 'sreg', FALSE); + $this->assertEqual($values, array('nickname' => 'john'), t('Value found for fallback prefix.')); + + $values = openid_extract_namespace($response, OPENID_NS_SREG, 'sreg', FALSE); + $this->assertEqual($values, array('nickname' => 'george', 'email' => 'george@example.com'), t('Namespace takes precedence over fallback prefix.')); + + // ext1.email is signed, but ext1.nickname is not. + $values = openid_extract_namespace($response, OPENID_NS_SREG, 'sreg', TRUE); + $this->assertEqual($values, array('email' => 'george@example.com'), t('Unsigned namespaced fields ignored.')); + + $values = openid_extract_namespace($response, 'http://example.com/ns/ext2', 'sreg', FALSE); + $this->assertEqual($values, array('foo' => '123', 'bar' => '456'), t('Unsigned fields found.')); + + // ext2.foo and ext2.bar are ignored, because ns.ext2 is not signed. The + // fallback prefix is not used, because the namespace is specified. + $values = openid_extract_namespace($response, 'http://example.com/ns/ext2', 'sreg', TRUE); + $this->assertEqual($values, array(), t('Unsigned fields ignored.')); + } } diff --git a/modules/openid/tests/openid_test.module b/modules/openid/tests/openid_test.module index 629dcd335..1b0de4ec5 100644 --- a/modules/openid/tests/openid_test.module +++ b/modules/openid/tests/openid_test.module @@ -324,9 +324,7 @@ function _openid_test_endpoint_authenticate() { // Generate unique identifier for this authentication. $nonce = _openid_nonce(); - // Generate response containing the user's identity. The openid.sreg.xxx - // entries contain profile data stored by the OpenID Provider (see OpenID - // Simple Registration Extension 1.0). + // Generate response containing the user's identity. $response = variable_get('openid_test_response', array()) + array( 'openid.ns' => OPENID_NS_2_0, 'openid.mode' => 'id_res', @@ -336,14 +334,27 @@ function _openid_test_endpoint_authenticate() { 'openid.return_to' => $_REQUEST['openid_return_to'], 'openid.response_nonce' => $nonce, 'openid.assoc_handle' => 'openid-test', - 'openid.signed' => 'op_endpoint,claimed_id,identity,return_to,response_nonce,assoc_handle', ); + if (isset($response['openid.signed'])) { + $keys_to_sign = explode(',', $response['openid.signed']); + } + else { + // Unless openid.signed is explicitly defined, all keys are signed. + $keys_to_sign = array(); + foreach ($response as $key => $value) { + // Strip off the "openid." prefix. + $keys_to_sign[] = substr($key, 7); + } + $response['openid.signed'] = implode(',', $keys_to_sign); + } + // Sign the message using the MAC key that was exchanged during association. $association = new stdClass(); $association->mac_key = variable_get('mac_key'); - $keys_to_sign = explode(',', $response['openid.signed']); - $response['openid.sig'] = _openid_signature($association, $response, $keys_to_sign); + if (!isset($response['openid.sig'])) { + $response['openid.sig'] = _openid_signature($association, $response, $keys_to_sign); + } // Put the signed message into the query string of a URL supplied by the // Relying Party, and redirect the user. -- cgit v1.2.3