From 80befb6c7e27d32b3e91cfce09bfe8e71b2c6203 Mon Sep 17 00:00:00 2001 From: Dries Buytaert Date: Sun, 22 Aug 2010 22:00:16 +0000 Subject: - Patch #886982 by Berdir, Heine: incomplete verification of assertions. --- modules/openid/openid.inc | 4 +- modules/openid/openid.install | 69 +++++++++++++++ modules/openid/openid.module | 189 ++++++++++++++++++++++++++++++++++++++---- modules/openid/openid.test | 19 +++++ 4 files changed, 265 insertions(+), 16 deletions(-) (limited to 'modules/openid') diff --git a/modules/openid/openid.inc b/modules/openid/openid.inc index 4d4163805..5a9326905 100644 --- a/modules/openid/openid.inc +++ b/modules/openid/openid.inc @@ -359,8 +359,8 @@ function _openid_parse_message($message) { * Return a nonce value - formatted per OpenID spec. */ function _openid_nonce() { - // YYYY-MM-DDThh:mm:ssTZD UTC, plus some optional extra unique chars - return gmstrftime('%Y-%m-%dT%H:%M:%S%Z') . + // YYYY-MM-DDThh:mm:ssZ, plus some optional extra unique characters. + return gmdate('Y-m-d\TH:i:s\Z') . chr(mt_rand(0, 25) + 65) . chr(mt_rand(0, 25) + 65) . chr(mt_rand(0, 25) + 65) . diff --git a/modules/openid/openid.install b/modules/openid/openid.install index 0cd900bd4..404cb2fdd 100644 --- a/modules/openid/openid.install +++ b/modules/openid/openid.install @@ -55,6 +55,32 @@ function openid_schema() { 'primary key' => array('assoc_handle'), ); + $schema['openid_nonce'] = array( + 'description' => 'Stores received openid.response_nonce per OpenID endpoint URL to prevent replay attacks.', + 'fields' => array( + 'idp_endpoint_uri' => array( + 'type' => 'varchar', + 'length' => 255, + 'description' => 'URI of the OpenID Provider endpoint.', + ), + 'nonce' => array( + 'type' => 'varchar', + 'length' => 255, + 'description' => 'The value of openid.response_nonce.', + ), + 'expires' => array( + 'type' => 'int', + 'not null' => TRUE, + 'default' => 0, + 'description' => 'A Unix timestamp indicating when the entry should expire.', + ), + ), + 'indexes' => array( + 'nonce' => array('nonce'), + 'expires' => array('expires'), + ), + ); + return $schema; } @@ -84,3 +110,46 @@ function openid_requirements($phase) { return $requirements; } + +/** + * @defgroup updates-6.x-extra Extra openid updates for 6.x + * @{ + */ + +/** + * Add a table to store nonces. + */ +function openid_update_6000() { + $schema['openid_nonce'] = array( + 'description' => 'Stores received openid.response_nonce per OpenID endpoint URL to prevent replay attacks.', + 'fields' => array( + 'idp_endpoint_uri' => array( + 'type' => 'varchar', + 'length' => 255, + 'description' => 'URI of the OpenID Provider endpoint.', + ), + 'nonce' => array( + 'type' => 'varchar', + 'length' => 255, + 'description' => 'The value of openid.response_nonce' + ), + 'expires' => array( + 'type' => 'int', + 'not null' => TRUE, + 'default' => 0, + 'description' => 'A Unix timestamp indicating when the entry should expire.', + ), + ), + 'indexes' => array( + 'nonce' => array('nonce'), + 'expires' => array('expires'), + ), + ); + + db_create_table('openid_nonce', $schema['openid_nonce']); +} + +/** + * @} End of "defgroup updates-6.x-extra" + * The next series of updates should start at 7000. + */ diff --git a/modules/openid/openid.module b/modules/openid/openid.module index b51e13aca..3412db45d 100644 --- a/modules/openid/openid.module +++ b/modules/openid/openid.module @@ -332,7 +332,7 @@ function openid_complete($response = array()) { $response['status'] = 'cancel'; } else { - if (openid_verify_assertion($service['uri'], $response)) { + if (openid_verify_assertion($service, $response)) { // OpenID Authentication, section 7.3.2.3 and Appendix A.5: // The CanonicalID specified in the XRDS document must be used as the // account key. We rely on the XRI proxy resolver to verify that the @@ -726,15 +726,31 @@ function openid_authentication_request($claimed_id, $identity, $return_to = '', /** * Attempt to verify the response received from the OpenID Provider. * - * @param $op_endpoint The OpenID Provider URL. - * @param $response Array of response values from the provider. + * @param $service + * Array describing the OpenID provider. + * @param $response + * Array of response values from the provider. * * @return boolean * @see http://openid.net/specs/openid-authentication-2_0.html#rfc.section.11.4 */ -function openid_verify_assertion($op_endpoint, $response) { +function openid_verify_assertion($service, $response) { module_load_include('inc', 'openid'); + // http://openid.net/specs/openid-authentication-2_0.html#rfc.section.11.3 + // Check the Nonce to protect against replay attacks. + if (!openid_verify_assertion_nonce($service, $response)) { + return FALSE; + } + + // http://openid.net/specs/openid-authentication-2_0.html#rfc.section.11.1 + // Verifying the return URL. + if (!openid_verify_assertion_return_url($service, $response)) { + return FALSE; + } + + // http://openid.net/specs/openid-authentication-2_0.html#rfc.section.11.4 + // Verify the signatures. $valid = FALSE; $association = FALSE; @@ -746,17 +762,13 @@ function openid_verify_assertion($op_endpoint, $response) { } if ($association && isset($association->session_type)) { - $keys_to_sign = explode(',', $response['openid.signed']); - $self_sig = _openid_signature($association, $response, $keys_to_sign); - if ($self_sig == $response['openid.sig']) { - $valid = TRUE; - } - else { - $valid = FALSE; - } + // http://openid.net/specs/openid-authentication-2_0.html#rfc.section.11.4.2 + // Verification using an association. + $valid = openid_verify_assertion_signature($service, $association, $response); } else { - // See http://openid.net/specs/openid-authentication-2_0.html#rfc.section.11.4.2.1 + // http://openid.net/specs/openid-authentication-2_0.html#rfc.section.11.4.2 + // Direct verification. // The verification requests contain all the fields from the response, // except openid.mode. $request = $response; @@ -767,7 +779,7 @@ function openid_verify_assertion($op_endpoint, $response) { 'method' => 'POST', 'data' => _openid_encode_message($message), ); - $result = drupal_http_request($op_endpoint, $options); + $result = drupal_http_request($service['uri'], $options); if (!isset($result->error)) { $response = _openid_parse_message($result->data); @@ -789,3 +801,152 @@ function openid_verify_assertion($op_endpoint, $response) { } return $valid; } + + +/** + * Verify the signature of the response received from the OpenID provider. + * + * @param $service + * Array describing the OpenID provider. + * @param $association + * Information on the association with the OpenID provider. + * @param $response + * Array of response values from the provider. + * + * @return + * TRUE if the signature is valid and covers all fields required to be signed. + * @see http://openid.net/specs/openid-authentication-2_0.html#rfc.section.11.4 + */ +function openid_verify_assertion_signature($service, $association, $response) { + if ($service['version'] == 2) { + // OpenID Authentication 2.0, section 10.1: + // These keys must always be signed. + $mandatory_keys = array('op_endpoint', 'return_to', 'response_nonce', 'assoc_handle'); + if (isset($response['openid.claimed_id'])) { + // If present, these two keys must also be signed. According to the spec, + // they are either both present or both absent. + $mandatory_keys[] = 'claimed_id'; + $mandatory_keys[] = 'identity'; + } + } + else { + // OpenID Authentication 1.1. section 4.3.3. + $mandatory_keys = array('identity', 'return_to'); + } + + $keys_to_sign = explode(',', $response['openid.signed']); + + if (count(array_diff($mandatory_keys, $keys_to_sign)) > 0) { + return FALSE; + } + + return _openid_signature($association, $response, $keys_to_sign) === $response['openid.sig']; +} + +/** + * Verify that the nonce has not been used in earlier assertions from the same OpenID provider. + * + * @param $service + * Array describing the OpenID provider. + * @param $response + * Array of response values from the provider. + * + * @return + * TRUE if the nonce has not expired and has not been used earlier. + */ +function openid_verify_assertion_nonce($service, $response) { + if ($service['version'] != 2) { + return TRUE; + } + + if (preg_match('/^(\d{4})-(\d{2})-(\d{2})T(\d{2}):(\d{2}):(\d{2})Z/', $response['openid.response_nonce'], $matches)) { + list(, $year, $month, $day, $hour, $minutes, $seconds) = $matches; + $nonce_timestamp = gmmktime($hour, $minutes, $seconds, $month, $day, $year); + } + else { + watchdog('openid', 'Nonce from @endpoint rejected because it is not correctly formatted, nonce: @nonce.', array('@endpoint' => $service['uri'], '@nonce' => $response['openid.response_nonce']), WATCHDOG_WARNING); + return FALSE; + } + + // A nonce with a timestamp to far in the past or future will already have + // been removed and cannot be checked for single use anymore. + $time = time(); + $expiry = 900; + if ($nonce_timestamp <= $time - $expiry || $nonce_timestamp >= $time + $expiry) { + watchdog('openid', 'Nonce received from @endpoint is out of range (time difference: @intervals). Check possible clock skew.', array('@endpoint' => $service['uri'], '@interval' => $time - $nonce_timestamp), WATCHDOG_WARNING); + return FALSE; + } + + // Record that this nonce was used. + db_insert('openid_nonce') + ->fields(array( + 'idp_endpoint_uri' => $service['uri'], + 'nonce' => $response['openid.response_nonce'], + 'expires' => $nonce_timestamp + $expiry, + )) + ->execute(); + + // Count the number of times this nonce was used. + $count_used = db_query("SELECT COUNT(*) FROM {openid_nonce} WHERE nonce = :nonce AND idp_endpoint_uri = :idp_endpoint_uri", array( + ':nonce' => $response['openid.response_nonce'], + ':idp_endpoint_uri' => $service['uri'], + ))->fetchField(); + + if ($count_used == 1) { + return TRUE; + } + else { + watchdog('openid', 'Nonce replay attempt blocked from @ip, nonce: @nonce.', array('@ip' => ip_address(), '@nonce' => $response['openid.response_nonce']), WATCHDOG_CRITICAL); + return FALSE; + } +} + + +/** + * Verify that openid.return_to matches the current URL. + * + * See OpenID Authentication 2.0, section 11.1. While OpenID Authentication + * 1.1, section 4.3 does not mandate return_to verification, the received + * return_to should still match these constraints. + * + * @param $service + * Array describing the OpenID provider. + * @param $response + * Array of response values from the provider. + * + * @return + * TRUE if return_to is valid, FALSE otherwise. + */ +function openid_verify_assertion_return_url($service, $response) { + global $base_url; + + $return_to_parts = parse_url($response['openid.return_to']); + + $base_url_parts = parse_url($base_url); + $current_parts = parse_url($base_url_parts['scheme'] .'://'. $base_url_parts['host'] . request_uri()); + + if ($return_to_parts['scheme'] != $current_parts['scheme'] || $return_to_parts['host'] != $current_parts['host'] || $return_to_parts['path'] != $current_parts['path']) { + return FALSE; + } + // Verify that all query parameters in the openid.return_to URL have + // the same value in the current URL. In addition, the current URL + // contains a number of other parameters added by the OpenID Provider. + parse_str(isset($return_to_parts['query']) ? $return_to_parts['query'] : '', $return_to_query_parameters); + foreach ($return_to_query_parameters as $name => $value) { + if (!array_key_exists($name, $_GET) || $_GET[$name] != $value) { + return FALSE; + } + } + return TRUE; +} + +/** + * Remove expired nonces from the database. + * + * Implementation of hook_cron(). + */ +function openid_cron() { + db_delete('openid_nonce') + ->condition('expires', REQUEST_TIME, '<') + ->execute(); +} diff --git a/modules/openid/openid.test b/modules/openid/openid.test index 68313ae7e..8937576c7 100644 --- a/modules/openid/openid.test +++ b/modules/openid/openid.test @@ -264,6 +264,25 @@ class OpenIDFunctionalTestCase extends OpenIDWebTestCase { } $this->assertRaw(t('Successfully added %identity', array('%identity' => $claimed_id)), t('Identity %identity was added.', array('%identity' => $identity))); } + + /** + * Tests that openid.signed is verified. + */ + function testSignatureValidation() { + // 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). + 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')); + $this->submitLoginForm($identity); + $this->assertNoRaw('OpenID login failed.'); + } + } /** -- cgit v1.2.3