diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 02a239e1006d50c92e90a928b772f6660b9f8058..d14d20bd98a5e779fe61bc2b366f1c5591ca3d9a 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -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 diff --git a/includes/actions.inc b/includes/actions.inc index d9b3f1ffd9d0977aebf1fd50dcb563e6c804eb3d..a06a19615ddc2d3b041bdd0916c6eb32b4d1c97f 100644 --- a/includes/actions.inc +++ b/includes/actions.inc @@ -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 { diff --git a/modules/comment/comment.module b/modules/comment/comment.module index 76000d9f8cf853def0c21951d5308a36377311fc..0b8ce23b461886deaa24dd7890ed0a3d88c983cf 100644 --- a/modules/comment/comment.module +++ b/modules/comment/comment.module @@ -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'); } } diff --git a/modules/openid/openid.install b/modules/openid/openid.install index 9e482fc50d965d4e7abaf24ebd5d1ccb44159060..d430258251eef86c8635d6772e0f227d8afed8eb 100644 --- a/modules/openid/openid.install +++ b/modules/openid/openid.install @@ -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. + */ diff --git a/modules/openid/openid.module b/modules/openid/openid.module index 1c0bb8f3469790e90d15b8324657d2c6ead3962e..79c020f4e85b4963539b68112606bd5c8157faca 100644 --- a/modules/openid/openid.module +++ b/modules/openid/openid.module @@ -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()); +} diff --git a/modules/system/system.module b/modules/system/system.module index 9c54a604661e19d8fbfea4da135f76c6f8760f08..30d6efa87d5ff3b9babb19322fe1806eae52ff6e 100644 --- a/modules/system/system.module +++ b/modules/system/system.module @@ -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']); } diff --git a/modules/trigger/trigger.admin.inc b/modules/trigger/trigger.admin.inc index 217bfd5c3c92cc5512fa358b62aba9d024a0017b..61123fa978a0dbd9664680a65cfcc559f504bac7 100644 --- a/modules/trigger/trigger.admin.inc +++ b/modules/trigger/trigger.admin.inc @@ -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'] ); } diff --git a/modules/upload/upload.module b/modules/upload/upload.module index 53d79c74186aa506bfb807820df25e8404e0fe5f..0efe0993c8391a4044aae935d6a36aced4f40ed5 100644 --- a/modules/upload/upload.module +++ b/modules/upload/upload.module @@ -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,