- Patch #886982 by Berdir, Heine: incomplete verification of assertions.

4 files changed, 265 insertions, 16 deletions

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.');
+  }
+
 }
 
 /**