From ee301cf5ebff3534b59fcece583b3a0e4f094f15 Mon Sep 17 00:00:00 2001 From: Lee Rowlands Date: Thu, 18 Oct 2018 08:40:19 +1000 Subject: [PATCH 1/2] SA-CORE-2018-006 by alexpott, attilatilman, bkosborne, catch, bonus, Wim Leers, Sam152, Berdir, Damien Tournoud, Dave Reid, Kova101, David_Rothstein, dawehner, dsnopek, samuel.mortenson, stefan.r, tedbow, xjm, timmillwood, pwolanin, njbooher, dyates, effulgentsia, klausi, mlhess, larowlan --- includes/common.inc | 5 ++++- modules/path/path.test | 30 +++++++++++++++++++++++++++++- modules/system/system.mail.inc | 34 +++++++++++++++++++++++++++++++++- 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/includes/common.inc b/includes/common.inc index f61d1eb0f24..a79a2f42ac8 100644 --- a/includes/common.inc +++ b/includes/common.inc @@ -2311,7 +2311,10 @@ function url($path = NULL, array $options = array()) { $language = isset($options['language']) && isset($options['language']->language) ? $options['language']->language : ''; $alias = drupal_get_path_alias($original_path, $language); if ($alias != $original_path) { - $path = $alias; + // Strip leading slashes from internal path aliases to prevent them + // becoming external URLs without protocol. /example.com should not be + // turned into //example.com. + $path = ltrim($alias, '/'); } } diff --git a/modules/path/path.test b/modules/path/path.test index edecff5cbb5..f6131ce62bf 100644 --- a/modules/path/path.test +++ b/modules/path/path.test @@ -21,7 +21,7 @@ class PathTestCase extends DrupalWebTestCase { parent::setUp('path'); // Create test user and login. - $web_user = $this->drupalCreateUser(array('create page content', 'edit own page content', 'administer url aliases', 'create url aliases')); + $web_user = $this->drupalCreateUser(array('create page content', 'edit own page content', 'administer url aliases', 'create url aliases', 'access content overview')); $this->drupalLogin($web_user); } @@ -160,6 +160,34 @@ class PathTestCase extends DrupalWebTestCase { $this->drupalGet($edit['path[alias]']); $this->assertNoText($node1->title, 'Alias was successfully deleted.'); $this->assertResponse(404); + + // Create third test node. + $node3 = $this->drupalCreateNode(); + + // Create an invalid alias with a leading slash and verify that the slash + // is removed when the link is generated. This ensures that URL aliases + // cannot be used to inject external URLs. + // @todo The user interface should either display an error message or + // automatically trim these invalid aliases, rather than allowing them to + // be silently created, at which point the functional aspects of this + // test will need to be moved elsewhere and switch to using a + // programmatically-created alias instead. + $alias = $this->randomName(8); + $edit = array('path[alias]' => '/' . $alias); + $this->drupalPost('node/' . $node3->nid . '/edit', $edit, t('Save')); + $this->drupalGet('admin/content'); + // This checks the link href before clicking it, rather than using + // DrupalWebTestCase::assertUrl() after clicking it, because the test + // browser does not always preserve the correct number of slashes in the + // URL when it visits internal links; using DrupalWebTestCase::assertUrl() + // would actually make the test pass unconditionally on the testbot (or + // anywhere else where Drupal is installed in a subdirectory). + $link_xpath = $this->xpath('//a[normalize-space(text())=:label]', array(':label' => $node3->title)); + $link_href = (string) $link_xpath[0]['href']; + $link_prefix = base_path() . (variable_get('clean_url', 0) ? '' : '?q='); + $this->assertEqual($link_href, $link_prefix . $alias); + $this->clickLink($node3->title); + $this->assertResponse(404); } /** diff --git a/modules/system/system.mail.inc b/modules/system/system.mail.inc index 443e5740013..9a17f55f6f5 100644 --- a/modules/system/system.mail.inc +++ b/modules/system/system.mail.inc @@ -70,7 +70,9 @@ class DefaultMailSystem implements MailSystemInterface { // hosts. The return value of this method will still indicate whether mail // was sent successfully. if (!isset($_SERVER['WINDIR']) && strpos($_SERVER['SERVER_SOFTWARE'], 'Win32') === FALSE) { - if (isset($message['Return-Path']) && !ini_get('safe_mode')) { + // We validate the return path, unless it is equal to the site mail, which + // we assume to be safe. + if (isset($message['Return-Path']) && !ini_get('safe_mode') && (variable_get('site_mail', ini_get('sendmail_from')) === $message['Return-Path'] || self::_isShellSafe($message['Return-Path']))) { // On most non-Windows systems, the "-f" option to the sendmail command // is used to set the Return-Path. There is no space between -f and // the value of the return path. @@ -109,6 +111,36 @@ class DefaultMailSystem implements MailSystemInterface { } return $mail_result; } + + /** + * Disallows potentially unsafe shell characters. + * + * Functionally similar to PHPMailer::isShellSafe() which resulted from + * CVE-2016-10045. Note that escapeshellarg and escapeshellcmd are inadequate + * for this purpose. + * + * @param string $string + * The string to be validated. + * + * @return bool + * True if the string is shell-safe. + * + * @see https://github.com/PHPMailer/PHPMailer/issues/924 + * @see https://github.com/PHPMailer/PHPMailer/blob/v5.2.21/class.phpmailer.php#L1430 + * + * @todo Rename to ::isShellSafe() and/or discuss whether this is the correct + * location for this helper. + */ + protected static function _isShellSafe($string) { + if (escapeshellcmd($string) !== $string || !in_array(escapeshellarg($string), array("'$string'", "\"$string\""))) { + return FALSE; + } + if (preg_match('/[^a-zA-Z0-9@_\-.]/', $string) !== 0) { + return FALSE; + } + return TRUE; + } + } /** From 08b4bce5841b938a3bdb24e3f4b246ae56ff91e7 Mon Sep 17 00:00:00 2001 From: Lee Rowlands Date: Thu, 18 Oct 2018 08:40:19 +1000 Subject: [PATCH 2/2] Drupal 7.60 --- CHANGELOG.txt | 3 +++ includes/bootstrap.inc | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 0929559d1fb..fa4f2091a29 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -1,3 +1,6 @@ +Drupal 7.60, 2018-10-18 +------------------------ +- Fixed security issues. See SA-CORE-2018-006. Drupal 7.59, 2018-04-25 ----------------------- diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc index a91c398f4ac..2d8b820fcbd 100644 --- a/includes/bootstrap.inc +++ b/includes/bootstrap.inc @@ -8,7 +8,7 @@ /** * The current system version. */ -define('VERSION', '7.59'); +define('VERSION', '7.60'); /** * Core API compatibility.