Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The Non-clean to Clean feature can be given a URL of the form http://example.com/?q=http://attack.com and it will redirect to attack.com.
I haven't had time to verify this with a clean copy yet but have confirmed that toggling this feature enables/disables the problem. I've attached a patch that fixes the issue in a non-elegant way by just copying code used to fix drupal_goto in the 6.16 update.
Comment | File | Size | Author |
---|---|---|---|
#15 | 768244-globalredirect-external-redirect-stop.patch | 5.2 KB | Dave Reid |
#10 | 768244-globalredirect-external-redirect-stop.patch | 5.26 KB | Dave Reid |
#10 | 768244-globalredirect-external-redirect-stop-d6.patch | 5.19 KB | Dave Reid |
#5 | globalredirect.external_fix.patch | 903 bytes | Anonymous (not verified) |
#2 | globalredirect.external_redirect.patch | 766 bytes | nicholasThompson |
Comments
Comment #1
benpjohnson CreditAttribution: benpjohnson commentedThe first patch doesn't fix the problem and is a mistake. Please see the attached for a updated version.
Comment #2
nicholasThompsonThis patch is applied against dev and does a much earlier check against an attach of this nature. It checks for "://" which I think should be sufficient enough to stop an external redirect and yet loose enough to avoid breaking very odd URL's... Does it sound sensible?
Comment #3
nicholasThompsonI've committed that patch to the dev branch of D6, however I'd still appreciate a discussion on if "://" is a sufficient protection...
Comment #4
mpotter CreditAttribution: mpotter commentedIn the 6.x-1.x-dev (2010-Jul-13) version, the above patch is actually implemented in the _globalredirect_is_active function around line 297.
This doesn't seem to prevent the problem if the URL uses "encoded" characters. For example:
http://example.com/?q=http%3A%2F%2Fattack.com
Edited: I suggest putting a urldecode() around the request_uri() call when testing for the presence of '://'
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedI have an alternate patch (which seems to bypass the urlencode issue entirely), based on a fix to a similar problem in Drupal core a few months ago. It's attached, and seems to work quite well for me (protecting against every type of external URL I can think of, including mailto: and anything else without slashes).
I'm not sure which approach is better, but I do think some sort of fix should be officially released, even if it's just one of these patches on top of the old version (without any of the other changes that have gone into CVS recently).
Comment #6
trothwell CreditAttribution: trothwell commentedThe patch seems to work well, the only issue I've come across with this is a redirection loop when: /?q=http:// is just entered.
Comment #7
gregglesSubscribe and confirming the criticality of this issue.
If there's not a fix and release in the coming weeks that will be a big concern to me.
Comment #8
Dave ReidI cannot confirm this is a problem on the current code, 6.x-1.x-dev. I tried loading http://mysql.drupal6dev.local/index.php?q=http://example.com/ and I got an 'Page not found'.
Comment #9
Dave ReidOk I can confirm http://mysql.drupal6dev.local/index.php?q=http%3A%2F%2Fgoogle.com is still letting it redirect.
Comment #10
Dave ReidOk now that I have the tests actually passing on both branches, here are some tests against both 7.x-1.x and 6.x-1.x with included tests.
Changes in the patch:
- Adds a globalredirect_url_is_external() function that is a duplicate of url_is_external() but that returns TRUE even if the protocol is invalid (otherwise an URL like invalid://example.com would still return FALSE meaning 'non-external').
- Adds a globalredirect_goto() wrapper for drupal_goto() that checks ifglobalredirect_url_is_external($path) returns FALSE before passing through to drupal_goto()
- Removes the check for absolute URL in _globalredirect_is_active() as it is no longer necessary. We are protected on redirect only.
- Adds tests to confirm the non-clean to clean redirection with multiple variations of external URLs included in the request path and query strings.
Comment #12
Dave Reid#10: 768244-globalredirect-external-redirect-stop.patch queued for re-testing.
Comment #13
Dave ReidComment #15
Dave ReidComment #16
cedric CreditAttribution: cedric commentedHaving good results with patch 768244-globalredirect-external-redirect-stop-d6.patch under 6.x-1.x-dev
Comment #17
gregglesBout that time, then?
Thanks for your work on this, Dave.
Comment #18
crimsondryad CreditAttribution: crimsondryad commentedSubscribing. Any idea when a release for this will drop? Pretty please? :)
Comment #19
nicholasThompsonI'm perfectly happy to apply this patch - however, I cannot replicate the redirect using the existing 7.x-1.x branch...
After applying the patch, the same request results in a 404:
Is the goal of this patch to ensure that bad URL's just 404 without having to redirect to clean first?
Comment #20
nicholasThompsonI could, however replicate this with the D6 branch...
Before patch:
After the patch:
I'm going to apply patches to both D6 and D7 branches - thanks for sorting this out Dave, great patch (as usual!).
Comment #21
nicholasThompsonFixed in dev - this will be in the next release for both D6 and D7.
Thanks to all involved :)
Comment #22
nottaken CreditAttribution: nottaken commentedThanks for working on the patch for this issue. Our McAfee secure scanner found this bug recently.