Problem/Motivation

The confirm form cancel button is subject to hijacking via the destination query string

reported STR:

1. Log in as an admin to your Drupal installation.
2. You need to have at least one custom block in your library to be vulnerable. If you don't then go to Structure > Block Layout > Custom block library > Add custom block and create one. The content is not important.
3. Without logging out, now open the following URI:

http://YOURDOMAIN/block/1/delete?destination=/http://www.attacker.com

4. Click on cancel. You will be taken to attacker.com.

found as part of the Drupal 8 security bug bounty

https://tracker.bugcrowd.com/submissions/e5e1eeaf5644a758f1df96fc37c8fd0...

NOTE from pwolanin: It looks like this or another related issue may have fixed one vector but created another #2501655: ConfirmFormHelper::buildCancelLink() incorrectly handles ?destination URLs with a leading slash

A couple weeks ago the attack worked with ?destination=/admin/http://www.attacker.com

but now that is URL encoded and gives a 404 on cancel.

Proposed resolution

Ensure that

$ drush ev "print_r(\Drupal\Core\Url::fromUserInput('/http://example.com')->toString())"
http://example.com⏎

no longer works, by fixing Url::fromUserInput

For confirmation forms, callback to the ordinary cancel URL, in case the passed in destination is invalid.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

dawehner’s picture

Wow, yeah this should be fixed. Note: I think postponing this onto #2507831: Harden redirect responses to make external URIs opt in (was SA-CORE-2015-002 foward-port) might be sense as it would provide some sort of API for checking it.

catch’s picture

Status: Active » Postponed

Yes let's postpone on that issue. The fact we've found yet another open redirect means we really ought to fix this centrally.

dawehner’s picture

Status: Postponed » Needs review
FileSize
1.13 KB
24.03 KB

This here a link, but I think we are doing something wrong on a more fundamental level.

You can do something like:

$ drush ev "print_r(\Drupal\Core\Url::fromUserInput('/http://example.com')->toString())"

http://example.com⏎

This cannot be the right thing to do!

Status: Needs review » Needs work

The last submitted patch, 5: 2512452-5.patch, failed testing.

pwolanin’s picture

This isn't a redirect, it's just a link.

So seems we need to fix that Url method only for this issue?

Also some validation that the cancel link is never external?

dawehner’s picture

Also some validation that the cancel link is never external?

Well I think yes, some integration test coverage should be done here.

So seems we need to fix that Url method only for this issue?

Well I think on a fundamental level we should disallow the PHP snipped I posted above, because it can easily lead to that problem.

But yes, actually I should have posted just the interdiff.

dawehner’s picture

FileSize
1.13 KB

Here is my patch.

pwolanin’s picture

Issue tags: +D8 Security Bounty, +D8 Accelerate
FileSize
2.26 KB
1.62 KB

That triggers an exception shown to the user if they go to a bad destination query string - I don't think that's desired?

fnqgpc’s picture

Hi, I'm the one who reported this bug. It seems that the last patch from dawehner has fixed it. If you don't want to throw an exception I think you could just set $uri_parts['path'] to a default value ('/').

Fabianx’s picture

Issue tags: +Needs tests
alexpott’s picture

The fix looks good. I guess @Fabianx wants test coverage of the logic in ConfirmFormHelper::buildCancelLink() - which seems like a good idea.

+++ b/core/lib/Drupal/Core/Url.php
@@ -426,6 +426,10 @@ protected static function fromInternalUri(array $uri_parts, array $options) {
+        throw new \InvalidArgumentException(SafeMarkup::format('The internal path component "@path" is external. You are not allowed to specify an external URL together with internal:/.', ['@path' => $uri_parts['path']]));

sprintf() should be used in exceptions.

alexpott’s picture

Status: Needs review » Needs work
neclimdul’s picture

+++ b/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
@@ -30,14 +30,20 @@ class ConfirmFormHelper {
       // @todo Revisit this in https://www.drupal.org/node/2418219.

Seems like this issue addresses #2418219: Deprecate destination URLs that don't include the base path and we can remove that todo and close the other issue.

pwolanin’s picture

@neclimdul - no, no - not at all. That's a much more substantial issue to remove all uses of system path

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.94 KB
1.85 KB

changed to sprintf and added a web test case.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Url.php
@@ -426,6 +426,10 @@ protected static function fromInternalUri(array $uri_parts, array $options) {
+      if (UrlHelper::isExternal($uri_parts['path'])) {
+        throw new \InvalidArgumentException(SafeMarkup::format('The internal path component "@path" is external. You are not allowed to specify an external URL together with internal:/.', ['@path' => $uri_parts['path']]));
+      }

You got the wrong one.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

For the comment in #18 :)

alexpott’s picture

But yeah let's convert them both here to be consistent.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.67 KB
3.61 KB

ok, there are a bunch of exception messages in that class.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the fix. Back to RTBC.

dawehner’s picture

Issue summary: View changes
Issue tags: -Needs tests
FileSize
7.74 KB
1.51 KB

I like the fallback in ConfirmFormHelper!

Not sure who reverted my changes to UrlTest, but I'll revert that again and instead converted the other examples.
It helps quite a bit to understand which test is failing.
Let's keep the issue on RTBC though.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1462460 and pushed to 8.0.x. Thanks!

  • alexpott committed 1462460 on 8.0.x
    Issue #2512452 by dawehner, pwolanin, fnqgpc: Confirm form cancel button...
pwolanin’s picture

@dawhener - ah - I modified test array format change since your interdiff in #9 has only the last case changed, not all so it didn't make sense.

dawehner’s picture

@dawhener - ah - I modified test array format change since your interdiff in #9 has only the last case changed, not all so it didn't make sense.

Fair, yeah I just wanted to ensure that educate more people about it.

catch’s picture

+++ b/core/lib/Drupal/Core/Url.php
@@ -232,7 +232,7 @@ public static function fromUserInput($user_input, $options = []) {
     if ((strpos($user_input, '/') !== 0) && (strpos($user_input, '#') !== 0) && (strpos($user_input, '?') !== 0)) {
-      throw new \InvalidArgumentException(SafeMarkup::format("The user-entered string @user_input must begin with a '/', '?', or '#'.", ['@user_input' => $user_input]));
+      throw new \InvalidArgumentException(sprintf("The user-entered string %s must begin with a '/', '?', or '#'.", $user_input));
     }
 

This is removing the escaping from $user_input.

Error::decodeException() does escape the message, so perhaps that's OK. However I don't really remember this being discussed anywhere explicitly.

I think we need to update the coding standards in the handbook with what's recommended at https://www.drupal.org/node/608166 - and it would be good to confirm that we're 100% sure that removing the escaping here is fine.

catch’s picture

Status: Fixed » Needs review

Reverted for now.

  • catch committed 97202a7 on 8.0.x
    Revert "Issue #2512452 by dawehner, pwolanin, fnqgpc: Confirm form...
pwolanin’s picture

So, the change to sprintf() was suggested by alexpott based on the downstream escaping. I thought from him it was the standard and this code was wrong is be using SafeMarkup.

I agree the coding standard is not clear, and maybe differs from 7 to 8 if we are not including exception messages as part of the interface translation.

catch’s picture

I think the code is wrong to be using SafeMarkup too, but we need to fix the documentation, since this has clearly never been documented anywhere at all. We also should be making core consistent as to how it treats exception methods since that is also not the case. Just changing them one-off as currently happens in the patch is not leading towards fixing either of those situations.

Gábor Hojtsy’s picture

@catch: I just made this update to at least fix the docs according to current method names. As to what best to use, that should be discussed / documented. https://www.drupal.org/node/608166/revisions/view/7442853/8611536

pwolanin’s picture

in D7: function _drupal_decode_exception()
in D8: \Drupal\Core\Utility\Error::decodeException()

both make the message plain, so right now we are probably double escaping

alexpott’s picture

Yep @pwolanin you're right - #2514044: Do not use SafeMarkup::format in exceptions has a test that confirms the double escaping. Anyhow I propose rather than hold this up on this issue we go with SafeMarkup::set() and we fix this mess in that issue.

alexpott’s picture

FileSize
4.28 KB
4.34 KB

Sorry for derailing this with all the exception/SafeMarkup stuff. Here's a patch that continues to use SafeMarkup in the exception.

xjm’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: -#2514044: Do not use SafeMarkup::format in exceptions

Damnit, alex you have beaten me :)

alexpott’s picture

FileSize
4.34 KB
Fabianx’s picture

RTBC + 1

catch’s picture

Status: Reviewed & tested by the community » Fixed

Leaving as is then standardizing + updating docs in the follow-up sounds good.

Committed/pushed to 8.0.x, thanks!

  • catch committed 7298c33 on 8.0.x
    Issue #2512452 by dawehner, pwolanin, alexpott, fnqgpc: Confirm form...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.