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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benpjohnson’s picture

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

nicholasThompson’s picture

Version: 6.x-1.2 » 6.x-1.x-dev
Priority: Normal » Critical
Status: Active » Needs review
FileSize
766 bytes

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?

nicholasThompson’s picture

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

mpotter’s picture

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

Anonymous’s picture

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

trothwell’s picture

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.

greggles’s picture

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.

Dave Reid’s picture

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

Dave Reid’s picture

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

Dave Reid’s picture

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.

Dave Reid’s picture

Status: Needs work » Needs review
Dave Reid’s picture

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.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
5.2 KB
cedric’s picture

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

greggles’s picture

Status: Needs review » Reviewed & tested by the community

Bout that time, then?

Thanks for your work on this, Dave.

crimsondryad’s picture

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

nicholasThompson’s picture

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?

nicholasThompson’s picture

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

nicholasThompson’s picture

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 :)

nottaken’s picture

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.