CVE-2010-3094

@@ -1,5 +1,11 @@
 // $Id$
 
+Drupal 6.18, 2010-08-11
+----------------------
+- Fixed security issues (OpenID authentication bypass, File download access
+  bypass, Comment unpublishing bypass, Actions cross site scripting),
+  see SA-CORE-2010-002.
+
 Drupal 6.17, 2010-06-02
 ----------------------
 - Improved PostgreSQL compatibility

@@ -328,7 +328,7 @@ function actions_synchronize($actions_in_code = array(), $delete_orphans = FALSE
       else {
         // This is a new singleton that we don't have an aid for; assign one.
         db_query("INSERT INTO {actions} (aid, type, callback, parameters, description) VALUES ('%s', '%s', '%s', '%s', '%s')", $callback, $array['type'], $callback, '', $array['description']);
-        watchdog('actions', "Action '%action' added.", array('%action' => filter_xss_admin($array['description'])));
+        watchdog('actions', "Action '%action' added.", array('%action' => $array['description']));
       }
     }
   }
@@ -350,7 +350,7 @@ function actions_synchronize($actions_in_code = array(), $delete_orphans = FALSE
       $results = db_query("SELECT a.aid, a.description FROM {actions} a WHERE callback IN ($placeholders)", $orphaned);
       while ($action = db_fetch_object($results)) {
         actions_delete($action->aid);
-        watchdog('actions', "Removed orphaned action '%action' from database.", array('%action' => filter_xss_admin($action->description)));
+        watchdog('actions', "Removed orphaned action '%action' from database.", array('%action' => $action->description));
       }
     }
     else {

@@ -663,7 +663,7 @@ function comment_access($op, $comment) {
   global $user;
 
   if ($op == 'edit') {
-    return ($user->uid && $user->uid == $comment->uid && comment_num_replies($comment->cid) == 0) || user_access('administer comments');
+    return ($user->uid && $user->uid == $comment->uid && comment_num_replies($comment->cid) == 0 && $comment->status == COMMENT_PUBLISHED) || user_access('administer comments');
   }
 }
 

@@ -66,5 +66,80 @@ 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;
 }
+
+/**
+ * @defgroup updates-6.x-extra Extra openid updates for 6.x
+ * @{
+ */
+
+/**
+ * Add the openid_nonce table.
+ *
+ * Implementation of hook_update_N().
+ */
+function openid_update_6000() {
+  $ret = array();
+
+  $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($ret, 'openid_nonce', $schema['openid_nonce']);
+
+  return $ret;
+}
+
+/**
+ * @} End of "defgroup updates-6.x-extra"
+ * The next series of updates should start at 7000.
+ */

@@ -95,7 +95,7 @@ function openid_form_alter(&$form, $form_state, $form_id) {
       'data' => l(t('Cancel OpenID login'), '#'),
       'class' => 'user-link',
     );
-    
+
     $form['openid_links'] = array(
       '#value' => theme('item_list', $items),
       '#weight' => 1,
@@ -220,12 +220,13 @@ function openid_begin($claimed_id, $return_to = '', $form_values = array()) {
  *   $response['status'] set to one of 'success', 'failed' or 'cancel'.
  */
 function openid_complete($response = array()) {
+  global $base_url;
   module_load_include('inc', 'openid');
 
   if (count($response) == 0) {
     $response = _openid_response();
   }
-  
+
   // Default to failed response
   $response['status'] = 'failed';
   if (isset($_SESSION['openid']['service']['uri']) && isset($_SESSION['openid']['claimed_id'])) {
@@ -238,7 +239,7 @@ function openid_complete($response = array()) {
         $response['status'] = 'cancel';
       }
       else {
-        if (openid_verify_assertion($service['uri'], $response)) {
+        if (openid_verify_assertion($service, $response)) {
           // If the returned claimed_id is different from the session claimed_id,
           // then we need to do discovery and make sure the op_endpoint matches.
           if ($service['version'] == 2 && $response['openid.claimed_id'] != $claimed_id) {
@@ -250,6 +251,31 @@ function openid_complete($response = array()) {
           else {
             $response['openid.claimed_id'] = $claimed_id;
           }
+          // 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.
+          $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 $response;
+          }
+          // 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 $response;
+            }
+          }
           $response['status'] = 'success';
         }
       }
@@ -502,33 +528,39 @@ 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 repsonse values from the provider.
+ * @param $service
+ *   Array describing the OpenID provider.
+ * @param $response
+ *   Array of response values from the provider.
  *
  * @return boolean
  */
-function openid_verify_assertion($op_endpoint, $response) {
+function openid_verify_assertion($service, $response) {
   module_load_include('inc', 'openid');
 
-  $valid = FALSE;
+  // 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.4
+  // Verify the signatures.
+  $valid = FALSE;
   $association = db_fetch_object(db_query("SELECT * FROM {openid_association} WHERE assoc_handle = '%s'", $response['openid.assoc_handle']));
   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 {
+    // http://openid.net/specs/openid-authentication-2_0.html#rfc.section.11.4.3
+    // Direct verification.
     $request = $response;
     $request['openid.mode'] = 'check_authentication';
     $message = _openid_create_message($request);
     $headers = array('Content-Type' => 'application/x-www-form-urlencoded; charset=utf-8');
-    $result = drupal_http_request($op_endpoint, $headers, 'POST', _openid_encode_message($message));
+    $result = drupal_http_request($service['uri'], $headers, 'POST', _openid_encode_message($message));
     if (!isset($result->error)) {
       $response = _openid_parse_message($result->data);
       if (strtolower(trim($response['is_valid'])) == 'true') {
@@ -541,3 +573,101 @@ 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_query("INSERT INTO {openid_nonce} (idp_endpoint_uri, nonce, expires) VALUES ('%s', '%s', %d)", $service['uri'], $response['openid.response_nonce'], $nonce_timestamp + $expiry);
+
+  // Count the number of times this nonce was used.
+  $count_used = db_result(db_query("SELECT COUNT(*) FROM {openid_nonce} WHERE nonce = '%s' AND idp_endpoint_uri = '%s'", $response['openid.response_nonce'], $service['uri']));
+
+  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;
+  }
+}
+
+/**
+ * Remove expired nonces from the database.
+ *
+ * Implementation of hook_cron().
+ */
+function openid_cron() {
+  db_query("DELETE FROM {openid_nonce} WHERE expires < %d", time());
+}

@@ -9,7 +9,7 @@
 /**
  * The current system version.
  */
-define('VERSION', '6.17');
+define('VERSION', '6.18');
 
 /**
  * Core API compatibility.
@@ -1388,7 +1388,7 @@ function system_actions_manage() {
   while ($action = db_fetch_object($result)) {
     $row[] = array(
       array('data' => $action->type),
-      array('data' => $action->description),
+      array('data' => filter_xss_admin($action->description)),
       array('data' => $action->parameters ? l(t('configure'), "admin/settings/actions/configure/$action->aid") : ''),
       array('data' => $action->parameters ? l(t('delete'), "admin/settings/actions/delete/$action->aid") : '')
     );
@@ -1594,9 +1594,8 @@ function system_actions_delete_form_submit($form, &$form_state) {
   $aid = $form_state['values']['aid'];
   $action = actions_load($aid);
   actions_delete($aid);
-  $description = check_plain($action->description);
-  watchdog('user', 'Deleted action %aid (%action)', array('%aid' => $aid, '%action' => $description));
-  drupal_set_message(t('Action %action was deleted', array('%action' => $description)));
+  watchdog('user', 'Deleted action %aid (%action)', array('%aid' => $aid, '%action' => $action->description));
+  drupal_set_message(t('Action %action was deleted', array('%action' => $action->description)));
   $form_state['redirect'] = 'admin/settings/actions/manage';
 }
 
@@ -1796,7 +1795,7 @@ function system_mail($key, &$message, $params) {
     );
   }
   $subject = strtr($context['subject'], $variables);
-  $body = strtr($context['message'], $variables);
+  $body = strtr(filter_xss_admin($context['message']), $variables);
   $message['subject'] .= str_replace(array("\r", "\n"), '', $subject);
   $message['body'][] = drupal_html_to_text($body);
 }
@@ -1845,11 +1844,11 @@ function system_message_action(&$object, $context = array()) {
     case 'taxonomy':
       $vocabulary = taxonomy_vocabulary_load($object->vid);
       $variables = array_merge($variables, array(
-        '%term_name' => $object->name,
-        '%term_description' => $object->description,
+        '%term_name' => check_plain($object->name),
+        '%term_description' => filter_xss_admin($object->description),
         '%term_id' => $object->tid,
-        '%vocabulary_name' => $vocabulary->name,
-        '%vocabulary_description' => $vocabulary->description,
+        '%vocabulary_name' => check_plain($vocabulary->name),
+        '%vocabulary_description' => filter_xss_admin($vocabulary->description),
         '%vocabulary_id' => $vocabulary->vid,
         )
       );
@@ -1864,13 +1863,13 @@ function system_message_action(&$object, $context = array()) {
       '%uid' => $node->uid,
       '%node_url' => url('node/'. $node->nid, array('absolute' => TRUE)),
       '%node_type' => check_plain(node_get_types('name', $node)),
-      '%title' => filter_xss($node->title),
-      '%teaser' => filter_xss($node->teaser),
-      '%body' => filter_xss($node->body),
+      '%title' => check_plain($node->title),
+      '%teaser' => check_markup($node->teaser, $node->format, FALSE),
+      '%body' => check_markup($node->body, $node->format, FALSE),
       )
     );
   }
-  $context['message'] = strtr($context['message'], $variables);
+  $context['message'] = strtr(filter_xss_admin($context['message']), $variables);
   drupal_set_message($context['message']);
 }
 

@@ -84,7 +84,7 @@ function trigger_unassign_submit($form, &$form_state) {
     $aid = actions_function_lookup($form_values['aid']);
     db_query("DELETE FROM {trigger_assignments} WHERE hook = '%s' AND op = '%s' AND aid = '%s'", $form_values['hook'], $form_values['operation'], $aid);
     $actions = actions_get_all_actions();
-    watchdog('actions', 'Action %action has been unassigned.',  array('%action' => check_plain($actions[$aid]['description'])));
+    watchdog('actions', 'Action %action has been unassigned.',  array('%action' => $actions[$aid]['description']));
     drupal_set_message(t('Action %action has been unassigned.', array('%action' => $actions[$aid]['description'])));
     $hook = $form_values['hook'] == 'nodeapi' ? 'node' : $form_values['hook'];
     $form_state['redirect'] = 'admin/build/trigger/'. $hook;
@@ -239,7 +239,7 @@ function theme_trigger_display($element) {
     $rows = array();
     foreach ($element['assigned']['#value'] as $aid => $info) {
       $rows[] = array(
-        $info['description'],
+        filter_xss_admin($info['description']),
         $info['link']
       );
     }

@@ -147,7 +147,13 @@ function _upload_file_limits($user) {
 function upload_file_download($filepath) {
   $filepath = file_create_path($filepath);
   $result = db_query("SELECT f.*, u.nid FROM {files} f INNER JOIN {upload} u ON f.fid = u.fid WHERE filepath = '%s'", $filepath);
-  if ($file = db_fetch_object($result)) {
+  while ($file = db_fetch_object($result)) {
+    if ($filepath !== $file->filepath) {
+      // Since some database servers sometimes use a case-insensitive
+      // comparison by default, double check that the filename is an exact
+      // match.
+      continue;
+    }
     if (user_access('view uploaded files') && ($node = node_load($file->nid)) && node_access('view', $node)) {
       return array(
         'Content-Type: ' . $file->filemime,