From 3724634d7440233d7b37192059d210264643c82c Mon Sep 17 00:00:00 2001
From: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date: Fri, 17 Jul 2015 11:56:19 +0100
Subject: [PATCH] Issue #2504141 by alexpott, tim.plunkett, larowlan,
David_Rothstein, dawehner: Information disclosure/open redirect vulnerability
via blocks that contain a form
---
core/core.services.yml | 4 +
.../RedirectLeadingSlashesSubscriber.php | 54 +++++++++
core/lib/Drupal/Core/Form/FormBuilder.php | 9 +-
.../src/Tests/Form/ExternalFormUrlTest.php | 108 ++++++++++++++++++
.../system/src/Tests/Routing/RouterTest.php | 20 ++++
5 files changed, 194 insertions(+), 1 deletion(-)
create mode 100644 core/lib/Drupal/Core/EventSubscriber/RedirectLeadingSlashesSubscriber.php
create mode 100644 core/modules/system/src/Tests/Form/ExternalFormUrlTest.php
diff --git a/core/core.services.yml b/core/core.services.yml
index 54ef20c0c834..e1ef8d2875ab 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -1021,6 +1021,10 @@ services:
arguments: ['@url_generator', '@router.request_context']
tags:
- { name: event_subscriber }
+ redirect_leading_slashes_subscriber:
+ class: Drupal\Core\EventSubscriber\RedirectLeadingSlashesSubscriber
+ tags:
+ - { name: event_subscriber }
request_close_subscriber:
class: Drupal\Core\EventSubscriber\RequestCloseSubscriber
tags:
diff --git a/core/lib/Drupal/Core/EventSubscriber/RedirectLeadingSlashesSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/RedirectLeadingSlashesSubscriber.php
new file mode 100644
index 000000000000..51ebbc22d68f
--- /dev/null
+++ b/core/lib/Drupal/Core/EventSubscriber/RedirectLeadingSlashesSubscriber.php
@@ -0,0 +1,54 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\EventSubscriber\RedirectLeadingSlashesSubscriber.
+ */
+
+namespace Drupal\Core\EventSubscriber;
+
+use Symfony\Component\HttpFoundation\RedirectResponse;
+use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+use Symfony\Component\HttpKernel\KernelEvents;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+
+/**
+ * Redirects paths starting with multiple slashes to a single slash.
+ */
+class RedirectLeadingSlashesSubscriber implements EventSubscriberInterface {
+
+ /**
+ * Redirects paths starting with multiple slashes to a single slash.
+ *
+ * @param \Symfony\Component\HttpKernel\Event\GetResponseEvent $event
+ * The GetResponseEvent to process.
+ */
+ public function redirect(GetResponseEvent $event) {
+ $request = $event->getRequest();
+ // Get the requested path minus the base path.
+ $path = $request->getPathInfo();
+
+ // It is impossible to create a link or a route to a path starting with
+ // multiple leading slashes. However if a form is added to the 404 page that
+ // submits back to the same URI this presents an open redirect
+ // vulnerability. Also, Drupal 7 renders the same page for
+ // http://www.example.org/foo and http://www.example.org////foo.
+ if (strpos($path, '//') === 0) {
+ $path = '/' . ltrim($path, '/');
+ $qs = $request->getQueryString();
+ if ($qs) {
+ $qs = '?' . $qs;
+ }
+ $event->setResponse(new RedirectResponse($request->getUriForPath($path) . $qs));
+ }
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ static function getSubscribedEvents() {
+ $events[KernelEvents::REQUEST][] = ['redirect', 1000];
+ return $events;
+ }
+
+}
diff --git a/core/lib/Drupal/Core/Form/FormBuilder.php b/core/lib/Drupal/Core/Form/FormBuilder.php
index 64e01e61276e..a24330d77f26 100644
--- a/core/lib/Drupal/Core/Form/FormBuilder.php
+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -725,7 +725,14 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
protected function buildFormAction() {
// @todo Use <current> instead of the master request in
// https://www.drupal.org/node/2505339.
- $request_uri = $this->requestStack->getMasterRequest()->getRequestUri();
+ $request = $this->requestStack->getMasterRequest();
+ $request_uri = $request->getRequestUri();
+
+ // Prevent cross site requests via the Form API by using an absolute URL
+ // when the request uri starts with multiple slashes..
+ if (strpos($request_uri, '//') === 0) {
+ $request_uri = $request->getUri();
+ }
// @todo Remove this parsing once these are removed from the request in
// https://www.drupal.org/node/2504709.
diff --git a/core/modules/system/src/Tests/Form/ExternalFormUrlTest.php b/core/modules/system/src/Tests/Form/ExternalFormUrlTest.php
new file mode 100644
index 000000000000..f179ec938077
--- /dev/null
+++ b/core/modules/system/src/Tests/Form/ExternalFormUrlTest.php
@@ -0,0 +1,108 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\system\Tests\Form\ExternalFormUrlTest.
+ */
+
+namespace Drupal\system\Tests\Form;
+
+use Drupal\Core\Form\FormInterface;
+use Drupal\Core\Form\FormStateInterface;
+use Drupal\simpletest\KernelTestBase;
+use Drupal\user\Entity\User;
+use Symfony\Component\HttpFoundation\Request;
+
+/**
+ * Ensures that form actions can't be tricked into sending to external URLs.
+ *
+ * @group system
+ */
+class ExternalFormUrlTest extends KernelTestBase implements FormInterface {
+
+ /**
+ * {@inheritdoc}
+ */
+ public static $modules = ['user', 'system'];
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getFormId() {
+ return 'external_form_url_test';
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function buildForm(array $form, FormStateInterface $form_state) {
+ $form['something'] = [
+ '#type' => 'textfield',
+ '#title' => 'What do you think?',
+ ];
+ return $form;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function validateForm(array &$form, FormStateInterface $form_state) {}
+
+ /**
+ * {@inheritdoc}
+ */
+ public function submitForm(array &$form, FormStateInterface $form_state) {}
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function setUp() {
+ parent::setUp();
+ $this->installSchema('system', ['key_value_expire', 'sequences']);
+ $this->installEntitySchema('user');
+
+ $test_user = User::create([
+ 'name' => 'foobar',
+ 'mail' => 'foobar@example.com',
+ ]);
+ $test_user->save();
+ \Drupal::service('current_user')->setAccount($test_user);
+
+ }
+
+ /**
+ * Tests form behaviour.
+ */
+ public function testActionUrlBehavior() {
+ // Create a new request which has a request uri with multiple leading
+ // slashes and make it the master request.
+ $request_stack = \Drupal::service('request_stack');
+ $original_request = $request_stack->pop();
+ $request = Request::create($original_request->getSchemeAndHttpHost() . '//example.org');
+ $request_stack->push($request);
+
+ $form = \Drupal::formBuilder()->getForm($this);
+ $markup = \Drupal::service('renderer')->renderRoot($form);
+
+ $this->setRawContent($markup);
+ $elements = $this->xpath('//form/@action');
+ $action = (string) $elements[0];
+ $this->assertEqual($original_request->getSchemeAndHttpHost() . '//example.org', $action);
+
+ // Create a new request which has a request uri with a single leading slash
+ // and make it the master request.
+ $request_stack = \Drupal::service('request_stack');
+ $original_request = $request_stack->pop();
+ $request = Request::create($original_request->getSchemeAndHttpHost() . '/example.org');
+ $request_stack->push($request);
+
+ $form = \Drupal::formBuilder()->getForm($this);
+ $markup = \Drupal::service('renderer')->renderRoot($form);
+
+ $this->setRawContent($markup);
+ $elements = $this->xpath('//form/@action');
+ $action = (string) $elements[0];
+ $this->assertEqual('/example.org', $action);
+ }
+
+}
diff --git a/core/modules/system/src/Tests/Routing/RouterTest.php b/core/modules/system/src/Tests/Routing/RouterTest.php
index f79cc5f891a2..b945676744f5 100644
--- a/core/modules/system/src/Tests/Routing/RouterTest.php
+++ b/core/modules/system/src/Tests/Routing/RouterTest.php
@@ -10,6 +10,7 @@
use Drupal\Core\Cache\Cache;
use Drupal\Core\Language\LanguageInterface;
use Drupal\simpletest\WebTestBase;
+use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\Exception\RouteNotFoundException;
/**
@@ -248,4 +249,23 @@ public function testRouterUninstallInstall() {
$route = \Drupal::service('router.route_provider')->getRouteByName('router_test.1');
$this->assertNotNull($route, 'Route exists after module installation');
}
+
+ /**
+ * Ensure that multiple leading slashes are redirected.
+ */
+ public function testLeadingSlashes() {
+ $request = $this->container->get('request_stack')->getCurrentRequest();
+ $url = $request->getUriForPath('//router_test/test1');
+ $this->drupalGet($url);
+ $this->assertEqual(1, $this->redirectCount, $url . " redirected to " . $this->url);
+ $this->assertUrl($request->getUriForPath('/router_test/test1'));
+
+ // It should not matter how many leading slashes are used and query strings
+ // should be preserved.
+ $url = $request->getUriForPath('/////////////////////////////////////////////////router_test/test1') . '?qs=test';
+ $this->drupalGet($url);
+ $this->assertEqual(1, $this->redirectCount, $url . " redirected to " . $this->url);
+ $this->assertUrl($request->getUriForPath('/router_test/test1') . '?qs=test');
+ }
+
}
--
GitLab