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.

Files: 
CommentFileSizeAuthor
#15 768244-globalredirect-external-redirect-stop.patch5.2 KBDave Reid
PASSED: [[SimpleTest]]: [MySQL] 237 pass(es).
[ View ]
#10 768244-globalredirect-external-redirect-stop.patch5.26 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] 234 pass(es), 6 fail(s), and 0 exception(es).
[ View ]
#10 768244-globalredirect-external-redirect-stop-d6.patch5.19 KBDave Reid
#5 globalredirect.external_fix.patch903 bytesjjs
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch globalredirect.external_fix.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#2 globalredirect.external_redirect.patch766 bytesnicholasThompson
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch globalredirect.external_redirect.patch. See the log in the details link for more information.
[ View ]
#1 globalredirect_external.patch1.76 KBbenpjohnson
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch globalredirect_external_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
globalredirect_external.patch971 bytesbenpjohnson
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch globalredirect_external.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Comments

StatusFileSize
new1.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch globalredirect_external_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

The first patch doesn't fix the problem and is a mistake. Please see the attached for a updated version.

Version:6.x-1.2» 6.x-1.x-dev
Priority:Normal» Critical
Status:Active» Needs review
StatusFileSize
new766 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch globalredirect.external_redirect.patch. See the log in the details link for more information.
[ View ]

This 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?

I've committed that patch to the dev branch of D6, however I'd still appreciate a discussion on if "://" is a sufficient protection...

Status:Needs review» Needs work

In 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 '://'

StatusFileSize
new903 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch globalredirect.external_fix.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

I 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).

The patch seems to work well, the only issue I've come across with this is a redirection loop when: /?q=http:// is just entered.

Subscribe 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.

I 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'.

Ok I can confirm http://mysql.drupal6dev.local/index.php?q=http%3A%2F%2Fgoogle.com is still letting it redirect.

Status:Needs work» Needs review
StatusFileSize
new5.19 KB
new5.26 KB
FAILED: [[SimpleTest]]: [MySQL] 234 pass(es), 6 fail(s), and 0 exception(es).
[ View ]

Ok 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.

Status:Needs review» Needs work

The last submitted patch, 768244-globalredirect-external-redirect-stop.patch, failed testing.

Status:Needs work» Needs review

Version:6.x-1.x-dev» 7.x-1.x-dev

Status:Needs review» Needs work

The last submitted patch, 768244-globalredirect-external-redirect-stop.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.2 KB
PASSED: [[SimpleTest]]: [MySQL] 237 pass(es).
[ View ]

Having good results with patch 768244-globalredirect-external-redirect-stop-d6.patch under 6.x-1.x-dev

Status:Needs review» Reviewed & tested by the community

Bout that time, then?

Thanks for your work on this, Dave.

Subscribing. Any idea when a release for this will drop? Pretty please? :)

I'm perfectly happy to apply this patch - however, I cannot replicate the redirect using the existing 7.x-1.x branch...

C:\Users\Nicholas Thompson\Desktop>c:\curl.exe -I http://vmdev7.dev/index.php?q=http%3A%2F%2Fgoogle.com
HTTP/1.1 301 Moved Permanently
Date: Tue, 20 Dec 2011 11:51:54 GMT
Server: Apache/2.2.15 (CentOS)
X-Powered-By: PHP/5.3.8
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Last-Modified: Tue, 20 Dec 2011 11:51:54 +0000
Cache-Control: no-cache, must-revalidate, post-check=0, pre-check=0
ETag: "1324381914"
Location: http://vmdev7.dev/http%3A//google.com
Connection: close
Content-Type: text/html; charset=UTF-8

After applying the patch, the same request results in a 404:

C:\Users\Nicholas Thompson\Desktop>c:\curl.exe -I http://vmdev7.dev/index.php?q=http%3A%2F%2Fgoogle.com
HTTP/1.1 404 Not Found
Date: Tue, 20 Dec 2011 11:53:09 GMT
Server: Apache/2.2.15 (CentOS)
X-Powered-By: PHP/5.3.8
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Last-Modified: Tue, 20 Dec 2011 11:53:09 +0000
Cache-Control: no-cache, must-revalidate, post-check=0, pre-check=0
ETag: "1324381989"
Content-Language: en
X-Generator: Drupal 7 (http://drupal.org)
Connection: close
Content-Type: text/html; charset=utf-8

Is the goal of this patch to ensure that bad URL's just 404 without having to redirect to clean first?

I could, however replicate this with the D6 branch...
Before patch:

C:\Users\Nicholas Thompson>c:\curl.exe -I http://vmdev6.dev/index.php?q=http%3A%2F%2Fgoogle.com
HTTP/1.1 301 Moved Permanently
Date: Tue, 20 Dec 2011 12:17:15 GMT
Server: Apache/2.2.15 (CentOS)
X-Powered-By: PHP/5.3.8
Set-Cookie: SESSb090aee14f1df12bbbf7d931cb0f27fc=mae7jgmpu42ojf5s1fl0uap886; expires=Thu, 12-Jan-2012 15:50:35 GMT; path=/; domain=.vmdev6.dev
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Last-Modified: Tue, 20 Dec 2011 12:17:15 GMT
Cache-Control: store, no-cache, must-revalidate
Cache-Control: post-check=0, pre-check=0
Location: http://google.com
Connection: close
Content-Type: text/html; charset=utf-8

After the patch:

C:\Users\Nicholas Thompson>c:\curl.exe -I http://vmdev6.dev/index.php?q=http%3A%2F%2Fgoogle.com
HTTP/1.1 404 Not Found
Date: Tue, 20 Dec 2011 12:17:41 GMT
Server: Apache/2.2.15 (CentOS)
X-Powered-By: PHP/5.3.8
Set-Cookie: SESSb090aee14f1df12bbbf7d931cb0f27fc=305saal1dpvpj8vres9eqa8bd0; expires=Thu, 12-Jan-2012 15:51:01 GMT; path=/; domain=.vmdev6.dev
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Last-Modified: Tue, 20 Dec 2011 12:17:41 GMT
Cache-Control: store, no-cache, must-revalidate
Cache-Control: post-check=0, pre-check=0
Connection: close
Content-Type: text/html; charset=utf-8

I'm going to apply patches to both D6 and D7 branches - thanks for sorting this out Dave, great patch (as usual!).

Status:Reviewed & tested by the community» Fixed

Fixed in dev - this will be in the next release for both D6 and D7.

Thanks to all involved :)

Thanks for working on the patch for this issue. Our McAfee secure scanner found this bug recently.

Status:Fixed» Closed (fixed)

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