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
Comment | File | Size | Author |
---|---|---|---|
#39 | 2512452.36.patch | 4.34 KB | alexpott |
#23 | interdiff.txt | 1.51 KB | dawehner |
#23 | 2512452-23.patch | 7.74 KB | dawehner |
#21 | increment.txt | 3.61 KB | pwolanin |
#21 | 2512452-21.patch | 6.67 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin at Acquia commentedComment #2
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedComment #3
dawehnerWow, 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.
Comment #4
catchYes let's postpone on that issue. The fact we've found yet another open redirect means we really ought to fix this centrally.
Comment #5
dawehnerThis here a link, but I think we are doing something wrong on a more fundamental level.
You can do something like:
This cannot be the right thing to do!
Comment #7
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedThis 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?
Comment #8
dawehnerWell I think yes, some integration test coverage should be done here.
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.
Comment #9
dawehnerHere is my patch.
Comment #10
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedThat triggers an exception shown to the user if they go to a bad destination query string - I don't think that's desired?
Comment #11
fnqgpc CreditAttribution: fnqgpc commentedHi, 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 ('/').
Comment #12
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #13
alexpottThe fix looks good. I guess @Fabianx wants test coverage of the logic in ConfirmFormHelper::buildCancelLink() - which seems like a good idea.
sprintf() should be used in exceptions.
Comment #14
alexpottComment #15
neclimdulSeems 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.
Comment #16
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@neclimdul - no, no - not at all. That's a much more substantial issue to remove all uses of system path
Comment #17
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedchanged to sprintf and added a web test case.
Comment #18
Fabianx CreditAttribution: Fabianx as a volunteer commentedYou got the wrong one.
Comment #19
alexpottFor the comment in #18 :)
Comment #20
alexpottBut yeah let's convert them both here to be consistent.
Comment #21
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedok, there are a bunch of exception messages in that class.
Comment #22
jibranThanks for the fix. Back to RTBC.
Comment #23
dawehnerI 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.
Comment #24
alexpottCommitted 1462460 and pushed to 8.0.x. Thanks!
Comment #26
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@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.
Comment #27
dawehnerFair, yeah I just wanted to ensure that educate more people about it.
Comment #28
catchThis 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.
Comment #29
catchReverted for now.
Comment #31
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedSo, 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.
Comment #32
catchI 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.
Comment #33
Gábor Hojtsy@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
Comment #34
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedin 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
Comment #35
alexpottYep @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.
Comment #36
alexpottSorry for derailing this with all the exception/SafeMarkup stuff. Here's a patch that continues to use SafeMarkup in the exception.
Comment #37
xjmComment #38
dawehnerDamnit, alex you have beaten me :)
Comment #39
alexpottComment #40
Fabianx CreditAttribution: Fabianx for Drupal Association commentedRTBC + 1
Comment #41
catchLeaving as is then standardizing + updating docs in the follow-up sounds good.
Committed/pushed to 8.0.x, thanks!