Non-clean to Clean allows external redirect

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

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

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.

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.

#10: 768244-globalredirect-external-redirect-stop.patch queued for re-testing.

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

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

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.