From c06c89d3b5d8266e082cc11b2f60819c088f3486 Mon Sep 17 00:00:00 2001 From: Daniel Wehner Date: Fri, 20 Mar 2015 09:45:29 +0100 Subject: [PATCH 1/3] Update pressflow to 6.35 --- includes/bootstrap.inc | 76 +++++++++++++++++++++++++++++++++++- includes/common.inc | 33 ++++++++++++---- includes/menu.inc | 10 ++++- modules/system/system.module | 2 +- modules/user/user.module | 28 +++++++++++-- modules/user/user.pages.inc | 2 +- 6 files changed, 137 insertions(+), 14 deletions(-) diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc index bf14d1bb049..886387e6ab1 100644 --- a/includes/bootstrap.inc +++ b/includes/bootstrap.inc @@ -1858,7 +1858,35 @@ function _drupal_bootstrap($phase) { case DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE: // Initialize configuration variables, using values from settings.php if available. $conf = variable_init(isset($conf) ? $conf : array()); - + + // Sanitize the destination parameter (which is often used for redirects) + // to prevent open redirect attacks leading to other domains. Sanitize + // both $_GET['destination'] and $_REQUEST['destination'] to protect code + // that relies on either, but do not sanitize $_POST to avoid interfering + // with unrelated form submissions. $_REQUEST['edit']['destination'] is + // also sanitized since drupal_goto() will sometimes rely on it, and + // other code might therefore use it too. The sanitization happens here + // because menu_path_is_external() requires the variable system to be + // available. + if (isset($_GET['destination']) || isset($_REQUEST['destination']) || isset($_REQUEST['edit']['destination'])) { + require_once './includes/menu.inc'; + drupal_load('module', 'filter'); + // If the destination is an external URL, remove it. + if (isset($_GET['destination']) && menu_path_is_external($_GET['destination'])) { + unset($_GET['destination']); + unset($_REQUEST['destination']); + } + // If there's still something in $_REQUEST['destination'] that didn't + // come from $_GET, check it too. + if (isset($_REQUEST['destination']) && (!isset($_GET['destination']) || $_REQUEST['destination'] != $_GET['destination']) && menu_path_is_external($_REQUEST['destination'])) { + unset($_REQUEST['destination']); + } + // Check $_REQUEST['edit']['destination'] separately. + if (isset($_REQUEST['edit']['destination']) && menu_path_is_external($_REQUEST['edit']['destination'])) { + unset($_REQUEST['edit']['destination']); + } + } + $cache_mode = variable_get('cache', CACHE_DISABLED); // Get the page from the cache, unless the cache is disabled or external. if ($cache_mode != CACHE_DISABLED && $cache_mode != CACHE_EXTERNAL) { @@ -2380,3 +2408,49 @@ function drupal_random_bytes($count) { return $output; } +/** + * Calculates a hexadecimal encoded sha-1 hmac. + * + * @param string $data + * String to be validated with the hmac. + * @param string $key + * A secret string key. + * + * See RFC2104 (http://www.ietf.org/rfc/rfc2104.txt). Note, the result of this + * must be identical to using hash_hmac('sha1', $data, $key); We don't use + * that function since PHP can be missing it if it was compiled with the + * --disable-hash switch. + * + * @return string + * A hexadecimal encoded sha-1 hmac. + */ +function drupal_hash_hmac_sha1($data, $key) { + // Keys longer than the 64 byte block size must be hashed first. + if (strlen($key) > 64) { + $key = pack("H*", sha1($key)); + } + return sha1((str_pad($key, 64, chr(0x00)) ^ (str_repeat(chr(0x5c), 64))) . pack("H*", sha1((str_pad($key, 64, chr(0x00)) ^ (str_repeat(chr(0x36), 64))) . $data))); +} + +/** + * Calculates a base-64 encoded, URL-safe sha-1 hmac. + * + * @param string $data + * String to be validated with the hmac. + * @param string $key + * A secret string key. + * + * @return string + * A base-64 encoded sha-1 hmac, with + replaced with -, / with _ and + * any = padding characters removed. + */ +function drupal_hmac_base64($data, $key) { + // Casting $data and $key to strings here is necessary to avoid empty string + // results of the hash function if they are not scalar values. As this + // function is used in security-critical contexts like token validation it is + // important that it never returns an empty string. + $hmac = base64_encode(pack("H*", drupal_hash_hmac_sha1((string) $data, (string) $key))); + // Modify the hmac so it's safe to use in URLs. + return strtr($hmac, array('+' => '-', '/' => '_', '=' => '')); +} + diff --git a/includes/common.inc b/includes/common.inc index 7152476f46f..c909425735b 100644 --- a/includes/common.inc +++ b/includes/common.inc @@ -328,7 +328,7 @@ function drupal_goto($path = '', $query = NULL, $fragment = NULL, $http_response if ($destination) { // Do not redirect to an absolute URL originating from user input. $colonpos = strpos($destination, ':'); - $absolute = ($colonpos !== FALSE && !preg_match('![/?#]!', substr($destination, 0, $colonpos))); + $absolute = strpos($destination, '//') === 0 || ($colonpos !== FALSE && !preg_match('![/?#]!', substr($destination, 0, $colonpos))); if (!$absolute) { extract(parse_url(urldecode($destination))); } @@ -380,7 +380,10 @@ function drupal_not_found() { // Keep old path for reference, and to allow forms to redirect to it. if (!isset($_REQUEST['destination'])) { - $_REQUEST['destination'] = $_GET['q']; + // Make sure that the current path is not interpreted as external URL. + if (!menu_path_is_external($_GET['q'])) { + $_REQUEST['destination'] = $_GET['q']; + } } $path = drupal_get_normal_path(variable_get('site_404', '')); @@ -410,7 +413,10 @@ function drupal_access_denied() { // Keep old path for reference, and to allow forms to redirect to it. if (!isset($_REQUEST['destination'])) { - $_REQUEST['destination'] = $_GET['q']; + // Make sure that the current path is not interpreted as external URL. + if (!menu_path_is_external($_GET['q'])) { + $_REQUEST['destination'] = $_GET['q']; + } } $path = drupal_get_normal_path(variable_get('site_403', '')); @@ -1468,12 +1474,20 @@ function url($path = NULL, $options = array()) { 'alias' => FALSE, 'prefix' => '' ); + // A duplicate of the code from menu_path_is_external() to avoid needing + // another function call, since performance inside url() is critical. if (!isset($options['external'])) { - // Return an external link if $path contains an allowed absolute URL. - // Only call the slow filter_xss_bad_protocol if $path contains a ':' before - // any / ? or #. + // Return an external link if $path contains an allowed absolute URL. Avoid + // calling filter_xss_bad_protocol() if there is any slash (/), hash (#) or + // question_mark (?) before the colon (:) occurrence - if any - as this + // would clearly mean it is not a URL. If the path starts with 2 slashes + // then it is always considered an external URL without an explicit protocol + // part. $colonpos = strpos($path, ':'); - $options['external'] = ($colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && filter_xss_bad_protocol($path, FALSE) == check_plain($path)); + $options['external'] = (strpos($path, '//') === 0) + || ($colonpos !== FALSE + && !preg_match('![/?#]!', substr($path, 0, $colonpos)) + && filter_xss_bad_protocol($path, FALSE) == check_plain($path)); } // May need language dependent rewriting if language.inc is present. @@ -1503,6 +1517,11 @@ function url($path = NULL, $options = array()) { return $path . $options['fragment']; } + // Strip leading slashes from internal paths to prevent them becoming external + // URLs without protocol. /example.com should not be turned into + // //example.com. + $path = ltrim($path, '/'); + global $base_url; static $script; diff --git a/includes/menu.inc b/includes/menu.inc index 0d377d0d387..ec5a2d8dbc3 100644 --- a/includes/menu.inc +++ b/includes/menu.inc @@ -2486,8 +2486,16 @@ function _menu_router_build($callbacks) { * Returns TRUE if a path is external (e.g. http://example.com). */ function menu_path_is_external($path) { + // Avoid calling filter_xss_bad_protocol() if there is any slash (/), + // hash (#) or question_mark (?) before the colon (:) occurrence - if any - as + // this would clearly mean it is not a URL. If the path starts with 2 slashes + // then it is always considered an external URL without an explicit protocol + // part. $colonpos = strpos($path, ':'); - return $colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && filter_xss_bad_protocol($path, FALSE) == check_plain($path); + return (strpos($path, '//') === 0) + || ($colonpos !== FALSE + && !preg_match('![/?#]!', substr($path, 0, $colonpos)) + && filter_xss_bad_protocol($path, FALSE) == check_plain($path)); } /** diff --git a/modules/system/system.module b/modules/system/system.module index 8803ce91def..2935c16090f 100644 --- a/modules/system/system.module +++ b/modules/system/system.module @@ -8,7 +8,7 @@ /** * The current system version. */ -define('VERSION', '6.34'); +define('VERSION', '6.35'); /** * Core API compatibility. diff --git a/modules/user/user.module b/modules/user/user.module index 5a7155936ba..e9bfdc2c5f3 100644 --- a/modules/user/user.module +++ b/modules/user/user.module @@ -1592,11 +1592,33 @@ function user_external_login_register($name, $module) { */ function user_pass_reset_url($account) { $timestamp = time(); - return url("user/reset/$account->uid/$timestamp/". user_pass_rehash($account->pass, $timestamp, $account->login), array('absolute' => TRUE)); + return url("user/reset/$account->uid/$timestamp/". user_pass_rehash($account->pass, $timestamp, $account->login, $account->uid), array('absolute' => TRUE)); } -function user_pass_rehash($password, $timestamp, $login) { - return md5($timestamp . $password . $login); +function user_pass_rehash($password, $timestamp, $login, $uid) { + // Backwards compatibility: Try to determine a $uid if one was not passed. + // (Since $uid is a required parameter to this function, a PHP warning will + // be generated if it's not provided, which is an indication that the calling + // code should be updated. But the code below will try to generate a correct + // hash in the meantime.) + if (!isset($uid)) { + $uids = array(); + $result = db_query_range("SELECT uid FROM {users} WHERE pass = '%s' AND login = '%s' AND uid > 0", $password, $login, 0, 2); + while ($row = db_fetch_array($result)) { + $uids[] = $row['uid']; + } + // If exactly one user account matches the provided password and login + // timestamp, proceed with that $uid. + if (count($uids) == 1) { + $uid = reset($uids); + } + // Otherwise there is no safe hash to return, so return a random string + // that will never be treated as a valid token. + else { + return drupal_random_key(); + } + } + return drupal_hmac_base64($timestamp . $login . $uid, drupal_get_private_key() . $password); } function user_edit_form(&$form_state, $uid, $edit, $register = FALSE) { diff --git a/modules/user/user.pages.inc b/modules/user/user.pages.inc index e5f7e5b561c..64cbd1e775a 100644 --- a/modules/user/user.pages.inc +++ b/modules/user/user.pages.inc @@ -106,7 +106,7 @@ function user_pass_reset(&$form_state, $uid, $timestamp, $hashed_pass, $action = drupal_set_message(t('You have tried to use a one-time login link that has expired. Please request a new one using the form below.')); drupal_goto('user/password'); } - else if ($account->uid && $timestamp > $account->login && $timestamp < $current && $hashed_pass == user_pass_rehash($account->pass, $timestamp, $account->login)) { + else if ($account->uid && $timestamp > $account->login && $timestamp < $current && $hashed_pass == user_pass_rehash($account->pass, $timestamp, $account->login, $account->uid)) { // First stage is a confirmation form, then login if ($action == 'login') { watchdog('user', 'User %name used one-time login link at time %timestamp.', array('%name' => $account->name, '%timestamp' => $timestamp)); From 5c3651685dea3676052e156297d0ee3448e53796 Mon Sep 17 00:00:00 2001 From: Daniel Wehner Date: Fri, 20 Mar 2015 10:00:20 +0100 Subject: [PATCH 2/3] fixed changelog.txt --- CHANGELOG.txt | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 99f2fc62ceb..c6f99a349b6 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -1,18 +1,6 @@ -Drupal 6.30, 2014-01-15 ----------------------- -- Fixed security issues (multiple vulnerabilities), see SA-CORE-2014-001. - -Drupal 6.29, 2013-11-20 ----------------------- -- Fixed security issues (multiple vulnerabilities), see SA-CORE-2013-003. - -Drupal 6.31, 2014-04-16 +Drupal 6.35, 2015-03-18 ---------------------- -- Fixed security issues (information disclosure). See SA-CORE-2014-002. - -Drupal 6.30, 2014-01-15 ----------------------- -- Fixed security issues (multiple vulnerabilities), see SA-CORE-2014-001. +- Fixed security issues (multiple vulnerabilities). See SA-CORE-2015-001. Drupal 6.34, 2014-11-19 ---------------------- From 25e43a41d902e54d3f118ceef43a9976c664bd22 Mon Sep 17 00:00:00 2001 From: Daniel Wehner Date: Wed, 29 Jul 2015 22:38:06 +0200 Subject: [PATCH 3/3] Update pressflow to 6.36 --- CHANGELOG.txt | 4 ++++ modules/openid/openid.module | 32 ++++++++++++++++++++++++++++---- modules/system/system.module | 2 +- 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index c6f99a349b6..bfefb91bb43 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -1,3 +1,7 @@ +Drupal 6.36, 2015-06-17 +----------------------- +- Fixed security issues (OpenID impersonation). See SA-CORE-2015-002. + Drupal 6.35, 2015-03-18 ---------------------- - Fixed security issues (multiple vulnerabilities). See SA-CORE-2015-001. diff --git a/modules/openid/openid.module b/modules/openid/openid.module index 2271464e81c..c27750768ba 100644 --- a/modules/openid/openid.module +++ b/modules/openid/openid.module @@ -243,10 +243,34 @@ function openid_complete($response = array()) { 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) { - $disco = openid_discovery($response['openid.claimed_id']); - if ($disco[0]['uri'] != $service['uri']) { - return $response; + if ($service['version'] == 2) { + // Returned Claimed Identifier could contain unique fragment + // identifier to allow identifier recycling so we need to preserve + // it in the response. + $response_claimed_id = _openid_normalize($response['openid.claimed_id']); + + if ($response_claimed_id != $claimed_id || $response_claimed_id != $response['openid.identity']) { + $disco = openid_discovery($response['openid.claimed_id']); + + if ($disco[0]['uri'] != $service['uri']) { + return $response; + } + + if (!empty($disco[0]['localid'])) { + $identity = $disco[0]['localid']; + } + else if (!empty($disco[0]['delegate'])) { + $identity = $disco[0]['delegate']; + } + else { + $identity = FALSE; + } + + // The OP-Local Identifier (if different than the Claimed + // Identifier) must be present in the XRDS document. + if ($response_claimed_id != $response['openid.identity'] && (!$identity || $identity != $response['openid.identity'])) { + return $response; + } } } else { diff --git a/modules/system/system.module b/modules/system/system.module index 2935c16090f..2a5f244263b 100644 --- a/modules/system/system.module +++ b/modules/system/system.module @@ -8,7 +8,7 @@ /** * The current system version. */ -define('VERSION', '6.35'); +define('VERSION', '6.36'); /** * Core API compatibility.