Information disclosure/open redirect vulnerability via blocks that contain a form

I was wondering whether this is something we should fix upstream

I can see why they might not want that, but on the other hand they are easily running into the same issue.
Maybe a new method or parameter will be a good compromise.

What we should do for sure is to write test coverage for that bug. Sorry for not seeing it on the other issue.

for example, http://d8.dev (not http://localhost/d8).

Testbot runs in /checkout - this will make testing with a web-test difficult.

Having a crack at a unit test.

failing unit test <3 FormInterface

Note: contacted Javier Eguiluz (kinda the drupal symfony contact person) and he wants to open up an issue in the symfony issue describing the problem.
It seemed to be quite positive, from its feedback

@dawehner so should this move to the private issue tracker?

@dawehner so should this move to the private issue tracker?

I'm not sure, if I understand it correctly javier more thought about this as security hardening, let's see

@larowlan drupal 8 security issues that only affect drupal 8 are handled in public.

I think he meant private issue tracker of symfony.

If upstream is up for it great, if not I would vote we override the Request object and provide our own class so that Request->getRequestUri() is always safe.

Great catch, David!

We should consider marking security hardenings with something like: @securityHardening or just "Security hardening: ..."

so that during code review removal of such code is a little more obvious - even though the code with "prevent CSRF" was pretty clear ...

yep I meant upstream

Drupal\system\Tests\Form\ExternalFormUrlTest 3 passes 2 fails

Fails as expected

I talked to Matthew O'Phinney, and Zend Diactoros (the PSR-7 library) uses the ltrim() approach. Seems legit, although I agree, better upstream.

https://twitter.com/mwop/status/609046964747902976

Unpublishing until such time as symfony decide whether to fix it in public or privately

Thanks @larowlan, I was encouraging others to do the same and unpublish the issue earlier today.

@catch, @effulgentsia, @webchick, @alexpott and I agreed this issue is definitely critical for Drupal 8 core due to the exploitable security vulnerability.

If Symfony chooses to treat this issue as public security hardening then we can proceed with this as a public Drupal 8 issue at that time since it does not affect D7.

This issue was unpublished to give Symfony time to decide 1. whether to fix the issue upstream 2. whether to do so privately via their security process if so.

The answer to both was 'no' after some discussion, so I'm re-publishing this.

$form_state wasn't used, so that now-illegal setCached() call can go, and we can't call render() "outside of a render context".

This gives the appropriate test fails.

#28++ — I wanted to ask about that setCached() call, I suspected it to be illegal.

1. +++ b/core/modules/system/src/Tests/Form/ExternalFormUrlTest.php
   @@ -0,0 +1,100 @@
   + * Ensures that form actions can't be tricked into sending to external urls.
   

   Nit: s/urls/URLs/

2. +++ b/core/modules/system/src/Tests/Form/ExternalFormUrlTest.php
   @@ -0,0 +1,100 @@
   +class ExternalFormUrlTest extends KernelTestBase implements FormInterface {
   

   It's kind of strange that a test implements FormInterface.

3. +++ b/core/modules/system/src/Tests/Form/ExternalFormUrlTest.php
   @@ -0,0 +1,100 @@
   +    $dom->loadXML($markup);
   

   Drupal doesn't render XML, but HTML. Let's use loadHtml(). Otherwise we may run into strange problems due to XML's more strict parsing.

4. +++ b/core/modules/system/src/Tests/Form/ExternalFormUrlTest.php
   @@ -0,0 +1,100 @@
   +    $xpath = new \DOMXPath($dom);
   +    $form = $xpath->query('//form');
   +    $action = $form->item(0)->getAttribute('action');
   +
   +    $this->assertNotEqual('//example.org', $action);
   +    $this->assertEqual('/example.org', $action);
   

   Is there a particular reason to not use $this->xpath()?

#30.2
grep for extends KernelTestBase implements FormInterface
It's a beautiful thing for a self-contained test (able to call \Drupal::formBuilder()->buildForm($this);)

Given that we already have our own Request factory already, see \Drupal\Core\DrupalKernel::setupTrustedHosts
we could also manipulate the incoming request, which IMHO is not the worst idea, given that it just "normalizes" the request.

@larowlan is in his well earned holidays

Using the FAPI based solution from the IS, and addressing the rest of #30.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -731,6 +731,7 @@ protected function buildFormAction() {
+    $parsed['path'] = '/' . ltrim($parsed['path'], '/');

Seems a comment here would be good.

Borrowing the comment from request_uri().
Do want to look into a more generic fix in this issue, or was this FAPI bit enough to fix the critical part, and have a major follow-up?

I think that we should look at a more generic issue because atm you probably could ddos a site with this. Since this is not normalised as it is in d7 filing up page cache is pretty simple

Discussed at length with @ircmaxwell and @neclimdul in #drupal-wscci

The upshot is that we probably want to bear https://github.com/zendframework/zend-diactoros/commit/68fc7424a66735926... in mind.

We could just implement middleware or a request listener that redirects http://example.org///foo to http://example.org/foo and be done? That way we should be able to fix cache pollution, google ranking and this security issue all in one go?

So at the moment d8 will produce 404's for example.com//node/1 and example.com//admin which feels correct. So perhaps the idea to implement redirects is wrong. @ircmaxwell also pointed out that the use of relative urls in form actions is problematic for reasons like this. Why do we do that?

I think that we should look at a more generic issue because atm you probably could ddos a site with this. Since this is not normalised as it is in d7 filing up page cache is pretty simple

Well, I think this kinda a bad argument, given that you can pass along query attributes really easy and by that bypass page caching.

@ircmaxwell also pointed out that the use of relative urls in form actions is problematic for reasons like this. Why do we do that?

I can just guess that its historically driven. In case we would use absolute URLs, we would be more safe, this is for sure. The only tiny downsite I could see is that we need more cache entries, like for example one for http:// and one for https:// URLs.

So what about checking for "//" at the beginning of the request cycle and in case its there just throw a 404?

Drupal 7 page cache will let you do http://example.com/node/1/xyz so 8.x won't fill up any differently (i.e. https://www.drupal.org/node/1000/xyz).

given that you can pass along query attributes really easy

Yes I don't think we can worry about this, or if we do we'd need to open another issue to fix the problem in many more places.

Also, are we sure search engines treat multiple slashes as separate URLs? I've never seen any indication that this is the case?

The middleware isn't necessarily a bad idea in itself, but I don't think it should block this issue, and unless there's a better indication that this is a real problem, I think it could live in global redirect module or similar.

The middleware isn't necessarily a bad idea in itself, but I don't think it should block this issue, and unless there's a better indication that this is a real problem, I think it could live in global redirect module or similar.

Fair point, it would be additional security hardening.

The patch for itself looks reasonable for me. We have for example test coverage.

Actually theoretically http://example.com//foo is a valid url (if a little weird). However adding

node.add_page.alt:
  path: '//node/add'
  defaults:
    _title: 'Add content'
    _controller: '\Drupal\node\Controller\NodeController::addPage'
  options:
    _node_operation_route: TRUE
  requirements:
    _node_add_access: 'node'

To node.routing does not work. I think the correct thing here is to add a security request listener that 404's for any path that starts with // and be done. Since our application just does not support url's with multiple leading slashes. If we did then this change would break forms that submitted back to them.

#43 sounds very sensible.

Actually doing a 404 is not possible because the open redirect vulnerability exists on blocks on the 404 page which submit to the page they are on.

In D7 URL's like https://www.drupal.org///////////////////////////////planet work. In D8 this would result in a 404. I think the best solution is to redirect URI's which have leading slashes to a single slash.

This is a completely different approach so no interdiff.

I wonder if the test added works on testbot given it is has a base path.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectLeadingSlashes.php
   @@ -0,0 +1,49 @@
   +   *   The Event to process.
   

   Should this specify the exact type of event? Not sure.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectLeadingSlashes.php
   @@ -0,0 +1,49 @@
   +  /**
   +   * Registers the methods in this class that should be listeners.
   +   *
   +   * @return array
   +   *   An array of event listener definitions.
   +   */
   

   Can be @inheritdoc.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectLeadingSlashes.php
   @@ -0,0 +1,49 @@
   +    $events[KernelEvents::REQUEST][] = array('redirect', 1000);
   

   Let's use the short notation.

4. +++ b/core/modules/system/src/Tests/Routing/RouterTest.php
   @@ -248,4 +249,22 @@ public function testRouterUninstallInstall() {
   +    $url = $request->getUriForPath('/////////////////////////////////////////////////router_test/test1');
   

   I think a comment like this would be helpful here:

   // Confirm that a single redirect is sufficient to end up on the final page, regardless of the number of leading slashes.
   

   P.S.: I'm pretty sure this is the most strange looking assertion in D8 :)

5. +++ b/core/modules/system/src/Tests/System/UncaughtExceptionTest.php
   @@ -17,6 +17,13 @@
   +   * Exceptions thrown by site under test that contain this text are ignored.
   

   I don't think this comment is right?

Oops #45 was a multi-patch. Fixed that and addressed everything from @Wim Leers review. Thanks @Wim Leers.

Any reason we don't use a middleware approach?

I'm totally fine with not using a middleware approach.

So yep testbot exposed a weakness when dealing with drupal8 installed in a subdirectory. Also we need to deal with query strings.

Yeah like Wim said in #45, someone could put a login block or some other unsafe theme code on a 40X page. Redirect is the way to go.

re: #49 I suggested a listener in IRC mostly because as a general rule I have concerns about making the middleware stack too deep. IMHO if we can implement something as a listener we should as I feel like it gives us more flexibility and maintainability.

We should certainly add some integration test coverage, that includes a form

+++ b/core/lib/Drupal/Core/EventSubscriber/RedirectLeadingSlashes.php
@@ -0,0 +1,52 @@
+    // It is impossible to create a link or a route to a path starting with
+    // 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.

Starting with *multiple* leading slashes.

I'm otherwise on board with this approach, though I agree with dawehner that we should add a test with a malicious form just to be sure we're catching the security concern.

I think its worth to try that, see https://github.com/symfony/symfony/issues/15292

I still feel that the redirect path is worth pursuing because this makes D8 behaviour similarly to D7 in that the same page will end up being displayed.

I'm not sure how to add an explicit test for the form given then as the patch and test show it is not possible to get to a page with multiple leading slashes.

We could do a negative test - make sure it's not a 200, and assertNoRaw() for the form action with the double slash? I usually don't like those but I think that works for covering the security issue itself.

I still feel that the redirect path is worth pursuing because this makes D8 behaviour similarly to D7 in that the same page will end up being displayed.

I don't disagree at all, but yeah I think the security issue is in the interaction between the form component and the $request, so the fix should be done there.
Doing stuff on top is certainly not a bad idea.

I'm not sure how to add an explicit test for the form given then as the patch and test show it is not possible to get to a page with multiple leading slashes.

We could add a kernel test, change the container and deregister the listener.

Here's a test. I think we should use an absolute URL if the request URI starts with multiple slashes. So that it we every do support multiple leading slashes resolving to different routes (why I have no idea but who knows) the code here will not let us down.

The test added is jsut an adaptation of @tim.plunkett's test in #36

Addressing #55 point about the comment.

We have a proper test coverage now!

+++ b/core/core.services.yml
@@ -1021,6 +1021,10 @@ services:
+  redirect_leading_slashes:
+    class: Drupal\Core\EventSubscriber\RedirectLeadingSlashes
+    tags:
+      - { name: event_subscriber }

Nitpick: It's not an official coding standard but by convention I think most of our subscriber classes have a *Subscriber suffix, and the service name has "subscriber" in it.

Actually, trolling through core.services.yml just now it seems that's only like 2/3 of the time. *sigh* Not a blocker here, just a note that we're being inconsistent.

Other than that, I agree this is ready to commit as soon as the bot is happy with it. Thanks, Alex!

RTBC + 1, agree with #64 that the rename would be nice.

Fine by me to rename it.

Looks right to me as well. I wasn't sure about the redirect, but given 7.x service pages like /////node1 it seems reasonable, and sites can override if they really don't like it.

Committed/pushed to 8.0.x, thanks!