summaryrefslogtreecommitdiffstats
path: root/core/lib/Drupal
diff options
context:
space:
mode:
Diffstat (limited to 'core/lib/Drupal')
-rw-r--r--core/lib/Drupal/Component/Utility/UrlHelper.php10
-rw-r--r--core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php32
-rw-r--r--core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php48
-rw-r--r--core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php9
-rw-r--r--core/lib/Drupal/Core/Routing/UrlGenerator.php5
-rw-r--r--core/lib/Drupal/Core/Security/RequestSanitizer.php13
6 files changed, 83 insertions, 34 deletions
diff --git a/core/lib/Drupal/Component/Utility/UrlHelper.php b/core/lib/Drupal/Component/Utility/UrlHelper.php
index 1d133c9..9e2365c 100644
--- a/core/lib/Drupal/Component/Utility/UrlHelper.php
+++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
@@ -248,6 +248,16 @@ class UrlHelper {
* Exception thrown when a either $url or $bath_url are not fully qualified.
*/
public static function externalIsLocal($url, $base_url) {
+ // Some browsers treat \ as / so normalize to forward slashes.
+ $url = str_replace('\\', '/', $url);
+
+ // Leading control characters may be ignored or mishandled by browsers, so
+ // assume such a path may lead to an non-local location. The \p{C} character
+ // class matches all UTF-8 control, unassigned, and private characters.
+ if (preg_match('/^\p{C}/u', $url) !== 0) {
+ return FALSE;
+ }
+
$url_parts = parse_url($url);
$base_parts = parse_url($base_url);
diff --git a/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
index 8397bde..67a4aae 100644
--- a/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
@@ -8,7 +8,6 @@ use Drupal\Core\Routing\LocalRedirectResponse;
use Drupal\Core\Routing\RequestContext;
use Drupal\Core\Utility\UnroutedUrlAssemblerInterface;
use Symfony\Component\HttpFoundation\Response;
-use Symfony\Component\HttpKernel\Event\GetResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
use Symfony\Component\HttpFoundation\RedirectResponse;
@@ -130,36 +129,6 @@ class RedirectResponseSubscriber implements EventSubscriberInterface {
}
/**
- * Sanitize the destination parameter to prevent open redirect attacks.
- *
- * @param \Symfony\Component\HttpKernel\Event\GetResponseEvent $event
- * The Event to process.
- */
- public function sanitizeDestination(GetResponseEvent $event) {
- $request = $event->getRequest();
- // 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. The sanitization happens here because
- // url_is_external() requires the variable system to be available.
- $query_info = $request->query;
- $request_info = $request->request;
- if ($query_info->has('destination') || $request_info->has('destination')) {
- // If the destination is an external URL, remove it.
- if ($query_info->has('destination') && UrlHelper::isExternal($query_info->get('destination'))) {
- $query_info->remove('destination');
- $request_info->remove('destination');
- }
- // If there's still something in $_REQUEST['destination'] that didn't come
- // from $_GET, check it too.
- if ($request_info->has('destination') && (!$query_info->has('destination') || $request_info->get('destination') != $query_info->get('destination')) && UrlHelper::isExternal($request_info->get('destination'))) {
- $request_info->remove('destination');
- }
- }
- }
-
- /**
* Registers the methods in this class that should be listeners.
*
* @return array
@@ -167,7 +136,6 @@ class RedirectResponseSubscriber implements EventSubscriberInterface {
*/
public static function getSubscribedEvents() {
$events[KernelEvents::RESPONSE][] = ['checkRedirectUrl'];
- $events[KernelEvents::REQUEST][] = ['sanitizeDestination', 100];
return $events;
}
diff --git a/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php b/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php
index e536149..27e5c76 100644
--- a/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php
+++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php
@@ -19,6 +19,20 @@ use Drupal\Core\Site\Settings;
class PhpMail implements MailInterface {
/**
+ * The configuration factory.
+ *
+ * @var \Drupal\Core\Config\ConfigFactoryInterface
+ */
+ protected $configFactory;
+
+ /**
+ * PhpMail constructor.
+ */
+ public function __construct() {
+ $this->configFactory = \Drupal::configFactory();
+ }
+
+ /**
* Concatenates and wraps the email body for plain-text mails.
*
* @param array $message
@@ -86,7 +100,10 @@ class PhpMail implements MailInterface {
// 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.
- $additional_headers = isset($message['Return-Path']) ? '-f' . $message['Return-Path'] : '';
+ // We validate the return path, unless it is equal to the site mail, which
+ // we assume to be safe.
+ $site_mail = $this->configFactory->get('system.site')->get('mail');
+ $additional_headers = isset($message['Return-Path']) && ($site_mail === $message['Return-Path'] || static::_isShellSafe($message['Return-Path'])) ? '-f' . $message['Return-Path'] : '';
$mail_result = @mail(
$message['to'],
$mail_subject,
@@ -112,4 +129,33 @@ class PhpMail implements MailInterface {
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), ["'$string'", "\"$string\""])) {
+ return FALSE;
+ }
+ if (preg_match('/[^a-zA-Z0-9@_\-.]/', $string) !== 0) {
+ return FALSE;
+ }
+ return TRUE;
+ }
+
}
diff --git a/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php b/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
index b85737f..d0690fe 100644
--- a/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
+++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
@@ -43,6 +43,15 @@ class PathProcessorAlias implements InboundPathProcessorInterface, OutboundPathP
if (empty($options['alias'])) {
$langcode = isset($options['language']) ? $options['language']->getId() : NULL;
$path = $this->aliasManager->getAliasByPath($path, $langcode);
+ // Ensure the resulting path has at most one leading slash, to prevent it
+ // becoming an external URL without a protocol like //example.com. This
+ // is done in \Drupal\Core\Routing\UrlGenerator::generateFromRoute()
+ // also, to protect against this problem in arbitrary path processors,
+ // but it is duplicated here to protect any other URL generation code
+ // that might call this method separately.
+ if (strpos($path, '//') === 0) {
+ $path = '/' . ltrim($path, '/');
+ }
}
return $path;
}
diff --git a/core/lib/Drupal/Core/Routing/UrlGenerator.php b/core/lib/Drupal/Core/Routing/UrlGenerator.php
index 3b5c8d2..853a5f7 100644
--- a/core/lib/Drupal/Core/Routing/UrlGenerator.php
+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -297,6 +297,11 @@ class UrlGenerator implements UrlGeneratorInterface {
if ($options['path_processing']) {
$path = $this->processPath($path, $options, $generated_url);
}
+ // Ensure the resulting path has at most one leading slash, to prevent it
+ // becoming an external URL without a protocol like //example.com.
+ if (strpos($path, '//') === 0) {
+ $path = '/' . ltrim($path, '/');
+ }
// The contexts base URL is already encoded
// (see Symfony\Component\HttpFoundation\Request).
$path = str_replace($this->decodedChars[0], $this->decodedChars[1], rawurlencode($path));
diff --git a/core/lib/Drupal/Core/Security/RequestSanitizer.php b/core/lib/Drupal/Core/Security/RequestSanitizer.php
index 3f48f3d..e1626ed 100644
--- a/core/lib/Drupal/Core/Security/RequestSanitizer.php
+++ b/core/lib/Drupal/Core/Security/RequestSanitizer.php
@@ -90,7 +90,8 @@ class RequestSanitizer {
}
if ($bag->has('destination')) {
- $destination_dangerous_keys = static::checkDestination($bag->get('destination'), $whitelist);
+ $destination = $bag->get('destination');
+ $destination_dangerous_keys = static::checkDestination($destination, $whitelist);
if (!empty($destination_dangerous_keys)) {
// The destination is removed rather than sanitized because the URL
// generator service is not available and this method is called very
@@ -101,6 +102,16 @@ class RequestSanitizer {
trigger_error(sprintf('Potentially unsafe destination removed from %s parameter bag because it contained the following keys: %s', $bag_name, implode(', ', $destination_dangerous_keys)));
}
}
+ // Sanitize the destination parameter (which is often used for redirects)
+ // to prevent open redirect attacks leading to other domains.
+ if (UrlHelper::isExternal($destination)) {
+ // The destination is removed because it is an external URL.
+ $bag->remove('destination');
+ $sanitized = TRUE;
+ if ($log_sanitized_keys) {
+ trigger_error(sprintf('Potentially unsafe destination removed from %s parameter bag because it points to an external URL.', $bag_name));
+ }
+ }
}
return $sanitized;
}