diff --git a/core/lib/Drupal/Component/Utility/UrlHelper.php b/core/lib/Drupal/Component/Utility/UrlHelper.php index 3293d5c..5f67181 100644 --- a/core/lib/Drupal/Component/Utility/UrlHelper.php +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php @@ -214,10 +214,15 @@ public static function encodePath($path) { */ public static function isExternal($path) { $colonpos = strpos($path, ':'); - // Avoid calling stripDangerousProtocols() 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. - return $colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && static::stripDangerousProtocols($path) == $path; + // Avoid calling drupal_strip_dangerous_protocols() 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. + return (strpos($path, '//') === 0) + || ($colonpos !== FALSE + && !preg_match('![/?#]!', substr($path, 0, $colonpos)) + && static::stripDangerousProtocols($path) == $path); } /** diff --git a/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php index 1a468f1..a92a5a8 100644 --- a/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php @@ -10,6 +10,7 @@ use Drupal\Component\Utility\UrlHelper; use Drupal\Core\Routing\RequestContext; use Drupal\Core\Routing\UrlGeneratorInterface; +use Symfony\Component\HttpKernel\Event\GetResponseEvent; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\HttpKernel\Event\FilterResponseEvent; use Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent; @@ -67,7 +68,7 @@ public function checkRedirectUrl(FilterResponseEvent $event) { // be absolute (start with a '/'). For such a case, prepend the // scheme and host, because the 'Location' header must be absolute. if (strpos($destination, '/') === 0) { - $destination = $request->getSchemeAndHttpHost() . $destination; + $destination = $this->requestContext->getCompleteBaseUrl() . $destination; } else { // Legacy destination query parameters can be relative paths that @@ -93,6 +94,33 @@ public function checkRedirectUrl(FilterResponseEvent $event) { } /** + * Sanitize the destination parameter to prevent open redirect attacks. + */ + 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 @@ -100,6 +128,7 @@ public function checkRedirectUrl(FilterResponseEvent $event) { */ static function getSubscribedEvents() { $events[KernelEvents::RESPONSE][] = array('checkRedirectUrl'); + $events[KernelEvents::REQUEST][] = array('sanitizeDestination', 100); return $events; } } diff --git a/core/lib/Drupal/Core/Routing/RedirectDestination.php b/core/lib/Drupal/Core/Routing/RedirectDestination.php index 58d7184..7c80b11 100644 --- a/core/lib/Drupal/Core/Routing/RedirectDestination.php +++ b/core/lib/Drupal/Core/Routing/RedirectDestination.php @@ -62,7 +62,10 @@ public function getAsArray() { public function get() { if (!isset($this->destination)) { $query = $this->requestStack->getCurrentRequest()->query; - if ($query->has('destination')) { + if (UrlHelper::isExternal($query->get('destination'))) { + $this->destination = '/'; + } + elseif ($query->has('destination')) { $this->destination = $query->get('destination'); } else { diff --git a/core/lib/Drupal/Core/Routing/UrlGenerator.php b/core/lib/Drupal/Core/Routing/UrlGenerator.php index 1926d44..c305e91 100644 --- a/core/lib/Drupal/Core/Routing/UrlGenerator.php +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php @@ -245,15 +245,20 @@ public function generateFromPath($path = NULL, $options = array()) { 'prefix' => '', ); + // A duplicate of the code from \Drupal\Component\Utility\UrlHelper::isExternal() + // 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 - // \Drupal\Component\Utility\UrlHelper::stripDangerousProtocols() if $path - // contains a ':' before any / ? or #. Note: we could use - // \Drupal\Component\Utility\UrlHelper::isExternal($path) here, but that - // would require another function call, and performance here is critical. $colonpos = strpos($path, ':'); - $options['external'] = ($colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && UrlHelper::stripDangerousProtocols($path) == $path); + // Avoid calling drupal_strip_dangerous_protocols() 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. + $options['external'] = (strpos($path, '//') === 0) + || ($colonpos !== FALSE + && !preg_match('![/?#]!', substr($path, 0, $colonpos)) + && UrlHelper::stripDangerousProtocols($path) == $path); } if (isset($options['fragment']) && $options['fragment'] !== '') { diff --git a/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php index bc21ea8..6af92f7 100644 --- a/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php +++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php @@ -113,6 +113,11 @@ protected function buildLocalUrl($uri, array $options = []) { // https://www.drupal.org/node/2417459 $uri = substr($uri, 5); + // Strip leading slashes from internal paths to prevent them becoming external + // URLs without protocol. /example.com should not be turned into + // //example.com. + $uri = ltrim($uri, '/'); + // Allow (outbound) path processing, if needed. A valid use case is the path // alias overview form: // @see \Drupal\path\Controller\PathController::adminOverview(). diff --git a/core/modules/system/src/Tests/Form/ConfirmFormTest.php b/core/modules/system/src/Tests/Form/ConfirmFormTest.php index ffcc44a..bd13a1f 100644 --- a/core/modules/system/src/Tests/Form/ConfirmFormTest.php +++ b/core/modules/system/src/Tests/Form/ConfirmFormTest.php @@ -7,6 +7,8 @@ namespace Drupal\system\Tests\Form; +use Drupal\Component\Utility\String; +use Drupal\Core\Url; use Drupal\simpletest\WebTestBase; /** @@ -23,7 +25,7 @@ class ConfirmFormTest extends WebTestBase { */ public static $modules = array('form_test'); - function testConfirmForm() { + function ptestConfirmForm() { // Test the building of the form. $this->drupalGet('form-test/confirm-form'); $site_name = $this->config('system.site')->get('name'); @@ -50,4 +52,26 @@ function testConfirmForm() { $this->assertUrl('form-test/confirm-form', array('query' => array('destination' => 'admin/config')), "The form's complex cancel link was followed."); } + /** + * Tests that the confirm form does not use external destinations. + */ + public function testConfirmFormWithExternalDestination() { + $this->drupalGet('form-test/confirm-form'); + $this->assertCancelLinkUrl(Url::fromRoute('form_test.route8')); + $this->drupalGet('form-test/confirm-form', array('query' => array('destination' => 'node'))); + $this->assertCancelLinkUrl(Url::fromUri('internal:/node')); + $this->drupalGet('form-test/confirm-form', array('query' => array('destination' => 'http://example.com'))); + $this->assertCancelLinkUrl(Url::fromRoute('form_test.route8')); + } + + /** + * Asserts that a cancel link is present pointing to the provided URL. + */ + function assertCancelLinkUrl(Url $url, $message = '', $group = 'Other') { + $links = $this->xpath('//a[@href=:url]', [':url' => $url->toString()]); + $message = ($message ? $message : String::format('Cancel link with url %url found.', ['%url' => $url->toString()])); + return $this->assertTrue(isset($links[0]), $message, $group); + } + + } diff --git a/core/modules/system/src/Tests/Routing/DestinationTest.php b/core/modules/system/src/Tests/Routing/DestinationTest.php new file mode 100644 index 0000000..3d45aab --- /dev/null +++ b/core/modules/system/src/Tests/Routing/DestinationTest.php @@ -0,0 +1,82 @@ + 'node', + 'output' => 'node', + 'message' => "Standard internal example node path is present in the 'destination' parameter.", + ], + [ + 'input' => '/example.com', + 'output' => '/example.com', + 'message' => 'Internal path with one leading slash is allowed.', + ], + [ + 'input' => '//example.com/test', + 'output' => '', + 'message' => 'External URL without scheme is not allowed.', + ], + [ + 'input' => 'example:test', + 'output' => 'example:test', + 'message' => 'Internal URL using a colon is allowed.', + ], + [ + 'input' => 'http://example.com', + 'output' => '', + 'message' => 'External URL is not allowed.', + ], + [ + 'input' => 'javascript:alert(0)', + 'output' => 'javascript:alert(0)', + 'message' => 'Javascript URL is allowed because it is treated as an internal URL.', + ], + ]; + foreach ($test_cases as $test_case) { + // Test $_GET['destination']. + $this->drupalGet('system-test/get-destination', ['query' => ['destination' => $test_case['input']]]); + $this->assertIdentical($test_case['output'], $this->getRawContent(), $test_case['message']); + // Test $_REQUEST['destination']. + $post_output = $this->drupalPost('system-test/request-destination', '*', ['destination' => $test_case['input']]); + $this->assertIdentical($test_case['output'], $post_output, $test_case['message']); + } + + // Make sure that 404 pages do not populate $_GET['destination'] with + // external URLs. + \Drupal::configFactory()->getEditable('system.site')->set('page.404', 'system-test/get-destination')->save(); + $this->drupalGet('http://example.com', ['external' => FALSE]); + $this->assertResponse(404); + $this->assertIdentical('/', $this->getRawContent(), 'External URL is not allowed on 404 pages.'); + } + +} diff --git a/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php index 5b4bf73..6b6b063 100644 --- a/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php +++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php @@ -88,6 +88,34 @@ public function drupalSetMessageTest() { } /** + * Controller to return $_GET['destination'] for testing. + * + * @param \Symfony\Component\HttpFoundation\Request $request + * The request. + * + * @return \Symfony\Component\HttpFoundation\Response + * The response. + */ + public function getDestination(Request $request) { + $response = new Response($request->query->get('destination')); + return $response; + } + + /** + * Controller to return $_REQUEST['destination'] for testing. + * + * @param \Symfony\Component\HttpFoundation\Request $request + * The request. + * + * @return \Symfony\Component\HttpFoundation\Response + * The response. + */ + public function requestDestination(Request $request) { + $response = new Response($request->request->get('destination')); + return $response; + } + + /** * Try to acquire a named lock and report the outcome. */ public function lockAcquire() { diff --git a/core/modules/system/tests/modules/system_test/system_test.routing.yml b/core/modules/system/tests/modules/system_test/system_test.routing.yml index 4251ced..86d0a31 100644 --- a/core/modules/system/tests/modules/system_test/system_test.routing.yml +++ b/core/modules/system/tests/modules/system_test/system_test.routing.yml @@ -87,3 +87,19 @@ system_test.configure: _title_callback: '\Drupal\system_test\Controller\SystemTestController::configureTitle' requirements: _access: 'TRUE' + +system_test.request_destination: + path: '/system-test/request-destination' + defaults: + _controller: '\Drupal\system_test\Controller\SystemTestController::requestDestination' + requirements: + _access: 'TRUE' + + +system_test.get_destination: + path: '/system-test/get-destination' + defaults: + _controller: '\Drupal\system_test\Controller\SystemTestController::getDestination' + requirements: + _access: 'TRUE' + diff --git a/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php index 0477b7a..537def6 100644 --- a/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php +++ b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php @@ -358,6 +358,10 @@ public static function providerTestIsExternal() { array('/internal/path', FALSE), array('https://example.com/external/path', TRUE), array('javascript://fake-external-path', FALSE), + // External URL without an explicit protocol. + array('//drupal.org/foo/bar?foo=bar&bar=baz&baz#foo', TRUE), + // Internal URL starting with a slash. + array('/drupal.org', FALSE), ); } diff --git a/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php index 9ff5414..3d5baf1 100644 --- a/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php @@ -14,6 +14,7 @@ use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Event\FilterResponseEvent; +use Symfony\Component\HttpKernel\Event\GetResponseEvent; use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\HttpKernel\KernelEvents; @@ -48,7 +49,11 @@ public function testDestinationRedirect(Request $request, $expected) { ->expects($this->any()) ->method('generateFromPath') ->willReturnMap([ - ['test', ['query' => [], 'fragment' => '', 'absolute' => TRUE], 'http://example.com/drupal/test'] + ['test', ['query' => [], 'fragment' => '', 'absolute' => TRUE], 'http://example.com/drupal/test'], + ['example.com', ['query' => [], 'fragment' => '', 'absolute' => TRUE], 'http://example.com/drupal/example.com'], + ['example:com', ['query' => [], 'fragment' => '', 'absolute' => TRUE], 'http://example.com/drupal/example:com'], + ['javascript:alert(0)', ['query' => [], 'fragment' => '', 'absolute' => TRUE], 'http://example.com/drupal/javascript:alert(0)'], + ['/test', ['query' => [], 'fragment' => '', 'absolute' => TRUE], 'http://example.com/drupal/test'], ]); } @@ -81,12 +86,91 @@ public function testDestinationRedirect(Request $request, $expected) { public static function providerTestDestinationRedirect() { return array( array(new Request(), FALSE), - array(new Request(array('destination' => 'http://example.com')), FALSE), - array(new Request(array('destination' => 'http://example.com/foobar')), FALSE), - array(new Request(array('destination' => 'http://example.ca/drupal')), FALSE), - array(new Request(array('destination' => 'test')), 'http://example.com/drupal/test'), - array(new Request(array('destination' => 'http://example.com/drupal/')), 'http://example.com/drupal/'), - array(new Request(array('destination' => 'http://example.com/drupal/test')), 'http://example.com/drupal/test'), +// array(new Request(array('destination' => 'http://example.com')), FALSE), +// array(new Request(array('destination' => 'http://example.com/foobar')), FALSE), +// array(new Request(array('destination' => 'http://example.ca/drupal')), FALSE), +// array(new Request(array('destination' => 'test')), 'http://example.com/drupal/test'), +// array(new Request(array('destination' => '/test')), 'http://example.com/drupal/test'), + array(new Request(array('destination' => '/example.com')), 'http://example.com/drupal/example.com'), +// array(new Request(array('destination' => 'example:com')), 'http://example.com/drupal/example:com'), +// array(new Request(array('destination' => '//example:com')), FALSE), +// array(new Request(array('destination' => '//example:com/test')), FALSE), +// array(new Request(array('destination' => 'javascript:alert(0)')), 'http://example.com/drupal/javascript:alert(0)'), +// array(new Request(array('destination' => 'http://example.com/drupal/')), 'http://example.com/drupal/'), +// array(new Request(array('destination' => 'http://example.com/drupal/test')), 'http://example.com/drupal/test'), ); } + + /** + * Tests that $_GET only contain internal URLs. + * + * @covers ::sanitizeDestination + * + * @dataProvider providerTestSanitizeDestination + * + * @see \Drupal\Component\Utility\UrlHelper::isExternal + */ + public function testSanitizeDestinationForGet($input, $output) { + $request = new Request(); + $request->query->set('destination', $input); + + $url_generator = $this->getMock('Drupal\Core\Routing\UrlGeneratorInterface'); + $request_context = new RequestContext(); + $listener = new RedirectResponseSubscriber($url_generator, $request_context); + $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); + $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST); + + $dispatcher = new EventDispatcher(); + $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'sanitizeDestination'], 100); + $dispatcher->dispatch(KernelEvents::REQUEST, $event); + + $this->assertEquals($output, $request->query->get('destination')); + } + + /** + * Tests that $_REQUEST['destination'] only contain internal URLs. + * + * @covers ::sanitizeDestination + * + * @dataProvider providerTestSanitizeDestination + * + * @see \Drupal\Component\Utility\UrlHelper::isExternal + */ + public function testSanitizeDestinationForPost($input, $output) { + $request = new Request(); + $request->request->set('destination', $input); + + $url_generator = $this->getMock('Drupal\Core\Routing\UrlGeneratorInterface'); + $request_context = new RequestContext(); + $listener = new RedirectResponseSubscriber($url_generator, $request_context); + $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); + $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST); + + $dispatcher = new EventDispatcher(); + $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'sanitizeDestination'], 100); + $dispatcher->dispatch(KernelEvents::REQUEST, $event); + + $this->assertEquals($output, $request->request->get('destination')); + } + + /** + * Data provider for testSanitizeDestination(). + */ + public function providerTestSanitizeDestination() { + $data = []; + // Standard internal example node path is present in the 'destination' parameter. + $data[] = ['node', 'node']; + // Internal path with one leading slash is allowed. + $data[] = ['/example.com', '/example.com']; + // External URL without scheme is not allowed. + $data[] = ['//example.com/test', '']; + // Internal URL using a colon is allowed. + $data[] = ['example:test', 'example:test']; + // External URL is not allowed. + $data[] = ['http://example.com', '']; + // Javascript URL is allowed because it is treated as an internal URL. + $data[] = ['javascript:alert(0)', 'javascript:alert(0)']; + + return $data; + } } diff --git a/core/tests/Drupal/Tests/Core/Routing/RedirectDestinationTest.php b/core/tests/Drupal/Tests/Core/Routing/RedirectDestinationTest.php index 3f38859..0545c4b 100644 --- a/core/tests/Drupal/Tests/Core/Routing/RedirectDestinationTest.php +++ b/core/tests/Drupal/Tests/Core/Routing/RedirectDestinationTest.php @@ -108,6 +108,11 @@ public function providerGet() { $request->query->set('other', 'value'); $data[] = [$request, '/current-path?other=value']; + // A request with a dedicated specified external destination. + $request = Request::create('/'); + $request->query->set('destination', 'https://www.drupal.org'); + $data[] = [$request, '/']; + return $data; } diff --git a/core/tests/Drupal/Tests/Core/Utility/UnroutedUrlAssemblerTest.php b/core/tests/Drupal/Tests/Core/Utility/UnroutedUrlAssemblerTest.php index 6f5401f..6a3642a 100644 --- a/core/tests/Drupal/Tests/Core/Utility/UnroutedUrlAssemblerTest.php +++ b/core/tests/Drupal/Tests/Core/Utility/UnroutedUrlAssemblerTest.php @@ -68,6 +68,14 @@ public function testAssembleWithNeitherExternalNorDomainLocalUri() { /** * @covers ::assemble + * @expectedException \InvalidArgumentException + */ + public function testAssembleWithLeadingSlash() { + $this->unroutedUrlAssembler->assemble('/drupal.org'); + } + + /** + * @covers ::assemble * @covers ::buildExternalUrl * * @dataProvider providerTestAssembleWithExternalUrl @@ -89,6 +97,7 @@ public function providerTestAssembleWithExternalUrl() { ['http://example.com/test', ['https' => TRUE], 'https://example.com/test'], ['https://example.com/test', ['https' => FALSE], 'http://example.com/test'], ['https://example.com/test?foo=1#bar', [], 'https://example.com/test?foo=1#bar'], + ['//drupal.org', [], '//drupal.org'], ]; } @@ -115,6 +124,7 @@ public function providerTestAssembleWithLocalUri() { ['base:example', [], TRUE, '/subdir/example'], ['base:example', ['query' => ['foo' => 'bar']], TRUE, '/subdir/example?foo=bar'], ['base:example', ['fragment' => 'example', ], TRUE, '/subdir/example#example'], + ['base:/drupal.org', [], FALSE, '/drupal.org'], ]; }