From 80befb6c7e27d32b3e91cfce09bfe8e71b2c6203 Mon Sep 17 00:00:00 2001
From: Dries Buytaert <dries@buytaert.net>
Date: Sun, 22 Aug 2010 22:00:16 +0000
Subject: [PATCH] - 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(-)
diff --git a/modules/openid/openid.inc b/modules/openid/openid.inc
index 4d4163805a64..5a9326905a4c 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 0cd900bd419f..404cb2fdd2c0 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 b51e13acab2f..3412db45d76e 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 68313ae7ea92..8937576c7978 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.');
+ }
+
}
/**
--
GitLab