diff --git a/CHANGELOG.txt b/CHANGELOG.txt
index fbf37ef34d7bd9a6cfc4490112170c63ec6b1246..6af2df3801730efac9160dec26f041e563c0ba59 100644
--- a/CHANGELOG.txt
+++ b/CHANGELOG.txt
@@ -1,7 +1,14 @@
// $Id$
-Drupal 6.18-dev, xxxx-xx-xx (development release)
+Drupal 6.19, 2010-08-11
----------------------
+- Fixed a variety of small bugs, improved code documentation.
+
+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
----------------------
@@ -230,6 +237,11 @@ Drupal 6.0, 2008-02-13
- Removed old system updates. Updates from Drupal versions prior to 5.x will
require upgrading to 5.x before upgrading to 6.x.
+Drupal 5.23, 2010-08-11
+-----------------------
+- Fixed security issues (File download access bypass, Comment unpublishing
+ bypass), see SA-CORE-2010-002.
+
Drupal 5.22, 2010-03-03
-----------------------
- Fixed security issues (Open redirection, Locale module cross site scripting,
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/includes/common.inc b/includes/common.inc
index 76d440c39c8ba5a55135d301e70c28d4a8dd1bc2..ef4e23581f59299269f49c0cc0eed59adfc80cc9 100644
--- a/includes/common.inc
+++ b/includes/common.inc
@@ -631,7 +631,7 @@ function drupal_error_handler($errno, $message, $filename, $line, $context) {
return;
}
- if ($errno & (E_ALL ^ E_DEPRECATED)) {
+ if ($errno & (E_ALL ^ E_DEPRECATED ^ E_NOTICE)) {
$types = array(1 => 'error', 2 => 'warning', 4 => 'parse error', 8 => 'notice', 16 => 'core error', 32 => 'core warning', 64 => 'compile error', 128 => 'compile warning', 256 => 'user error', 512 => 'user warning', 1024 => 'user notice', 2048 => 'strict warning', 4096 => 'recoverable fatal error');
// For database errors, we want the line number/file name of the place that
@@ -2891,7 +2891,7 @@ function drupal_alter($type, &$data) {
* Recursively iterates over each of the array elements, generating HTML code.
* This function is usually called from within another function, like
* drupal_get_form() or node_view().
- *
+ *
* drupal_render() flags each element with a '#printed' status to indicate that
* the element has been rendered, which allows individual elements of a given
* array to be rendered independently. This prevents elements from being
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 fee578df973f08ec5e835fe14595eb46717b8f6b..7ad94f92c344054a3e258c9cb45ff59a7c76adb3 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 b805c02d1de7a1a88a49faefc13b580cb5dc490e..c9d15c467aec619e7a2d7e6c9a1a5691c14439e2 100644
--- a/modules/system/system.module
+++ b/modules/system/system.module
@@ -9,7 +9,7 @@
/**
* The current system version.
*/
-define('VERSION', '6.18-dev');
+define('VERSION', '6.19');
/**
* 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,