Issue #2504141 by alexpott, tim.plunkett, larowlan, David_Rothstein, dawehner:...

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

@@ -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:

core/lib/Drupal/Core/EventSubscriber/RedirectLeadingSlashesSubscriber.php

+<?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;
+  }
+
+}

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.

core/modules/system/src/Tests/Form/ExternalFormUrlTest.php

+<?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);
+  }
+
+}

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');
+  }
+
 }