SA-CORE-2010-001 - Open redirection

in addition;

1. parse_url should not be used on relative urls (per the note on php.net)

parse_url breaks on the following sample destination, where only '/bar' will endup in $path.

'foo/bar?val=http://foo

2. idem for drupal_parse_url, phpdoc states:

* This function should only be used for URLs that have been generated by the
* system, resp. url(). It should not be used for URLs that come from external
* sources, or URLs that link to external resources.

    * For usability sake, we might want to warn users when they are going to be redirected to another site.
    * Is there a legitimate use case for usage of the destination parameter to redirect to an external website after form submission, or should it be suppressed? If people insist on doing that, they still can override the normal submit redirect.

There are legitimate cases for redirection to another site -- or at least to another base url -- within Drupal, particularly in the Domain Access world (http://drupal.org/project/domain). I would not want those redirects announced by default.

Trigger module also allow the arbitrary redirection of any form action (essentially) to any URL.

In some cases, you have to set a destination, because, for instance, node module _hardcodes_ a form redirect into its submit handler that cannot be overridden in any other way.

Do you mean it's hardcoded for the search form for node? Or somewhere else in node module?

There is a patch to deal with the search one, to make it more sane.

@pwolanin

I mean the node editing form. http://api.drupal.org/api/function/node_form_submit/7, specifically:

  if ($node->nid) {
    unset($form_state['rebuild']);
    $form_state['values']['nid'] = $node->nid;
    $form_state['nid'] = $node->nid;
    $form_state['redirect'] = 'node/' . $node->nid; // <-- this cannot be altered by hook_form_alter().

To force this redirect to go elsewhere, you either have to use Actions/Triggers (which is itself broken in this regard, see #732542: system_goto_action breaks core APIs) or tamper with $_GET['destination'] which overrules the 'redirect' from $form_state.

My basic point is that sometimes I want to redirect a node to something other than $base_url/node/$node->nid.

@agentrickard - why not add another submit hook that runs after?

That would still require a redirection to an absolute URL that may or may not match the HTTP_HOST in $base_url, which is what this issue is about.

@agentrickard - I don't think the patch changes the ability to use redirect to go to an external URL, it only prevents it from coming in $_REQUEST['destination']

And that's the problem. Because as I understand it, that's really the only way to override what node_form_submit() is doing with any guarantee. A secondary #submit function might work, but also see #732542: system_goto_action breaks core APIs which uses 'destination' to solve a specific problem with drupal_goto() and Triggers. And Triggers allows any arbitrary URL to be used.

One approach might be to require external redirects to be authenticated with drupal_get_token(). Thoughts?
(example: http://drupalsite.org/form?destination=http://elsewhere.com&token=4af3cf...)

I couldn't reproduce this, even on Drupal 6.15. scor thought it might be only be exploitable with certain Apache settings, does anyone know more about how to trigger it? I'm using Apache 2.2.14 on OS X.

Before moving on here, we need a test that exposes the flaw and ensures that it's actually still there.

$_REQUEST['destination'] has been replaced with $_GET['destination'].

Parsing of URLs now happens in http://api.drupal.org/api/function/drupal_parse_url/7, and actually I thought we would have taken this edge-case into account when all of those functions were revamped + cleaned up recently.

Doesn't appear to be a threat any longer (at least using the trivial example given in the original report.

print_r(drupal_parse_url('trickparseurl:http://cwe.mitre.org/data/definitions/601.html'));

gives

Array ( [path] => trickparseurl:http://cwe.mitre.org/data/definitions/601.html [query] => Array ( ) [fragment] => ) 

Will leave status so others can review.

Thanks! Can we quickly turn this into a test, so we do not break it in the future?

Yeah, this is actually still an issue. I am thinking drupal_goto should never redirect to an external URL. It currently is, you can see by going to http://example.com/node/add/page?destination=http://google.com

I don't think there's a valid use case for using destination for an external URL, but maybe I'm wrong. The doc for drupal_goto() states "This issues an on-site HTTP redirect.", so I think that if we decide otherwise, we need to change the doc.

I have patched drupal_goto() to eliminate this threat using url_is_external() (which as far as I could tell is the exact code in the security patch above), and added a general-purpose drupal_parse_url() test case, as well as one that tests its vigilance against crafted URLs.

+++ modules/simpletest/tests/common.test
@@ -1478,6 +1478,30 @@ class ValidUrlTestCase extends DrupalUnitTestCase {
+    $parts = drupal_parse_url(
+      'http://example.com/?foo=bar#baz'
+    );
+    ¶
+    $this->assertEqual($parts['path'], 'http://example.com', t('drupal_parse_url() correctly parsed path.'));

This is the only assertion that failed, and it failed, because you missed the trailing slash in the comparison.

91 critical left. Go review some!

Sorry about that. I missed the original drupal_parse_url() tests, so it wasn't even necessary. I put the forged URL test in the original test. Let's try this.

+++ includes/common.inc
@@ -659,7 +659,7 @@ function drupal_encode_path($path) {
+  if (isset($_GET['destination']) && !url_is_external($_GET['destination'])) {
     $destination = drupal_parse_url(urldecode($_GET['destination']));

I wonder whether it is correct to check url_is_external() that early? Will the URL be identified as external if it is encoded somehow, or if it is forged as in the way we are discussing here?

Ideally, this should probably be covered by tests, too :-/

+++ modules/simpletest/tests/common.test
@@ -200,8 +200,13 @@ class CommonURLUnitTest extends DrupalWebTestCase {
+    // Test that drupal_parse_url doesn't expose the hole discussed at
+    // http://drupal.org/node/733028
+    $parts = drupal_parse_url('forged:http://cwe.mitre.org/data/definitions/601.html');
+    $this->assertFalse(valid_url($parts['path'], TRUE), t('drupal_parse_url() correctly parsed a forged URL.'));

The inline comment should not contain a reference to this issue, but instead explain what is being tested and why.

Furthermore, it should state what the expected result is -- and a short summary of that has to be used for the assertion message.

Lastly, I'm not sure what this test/assertion is trying to ensure. valid_url() returns FALSE for any absolute URL that doesn't start with ftp|https?|..., so will always be FALSE on a "forged:" URL scheme...?

Needs plenty of explanation (in code).

+++ modules/simpletest/tests/common.test
@@ -200,8 +200,13 @@ class CommonURLUnitTest extends DrupalWebTestCase {
+  }
+  ¶

Trailing white-space here.

Powered by Dreditor.

@cha0s

There are valid reasons to use drupal_goto() to send a user to a URL other than $base_url, as described in #2 above, particularly when using hook_url_rewrite_outbound() or system_goto_action().

For example: On user insert, pop the user to an external URL for them to opt-out of newsletters run through an external service.

We should still be able to pass an external $path to drupal_goto() and expect it to work.

For security, we should be trying to prevent INVALID URLs, not assessing the location of the URL.

@sun: Will address these in an upcoming patch. Though,

'Lastly, I'm not sure what this test/assertion is trying to ensure. valid_url() returns FALSE for any absolute URL that doesn't start with ftp|https?|..., so will always be FALSE on a "forged:" URL scheme...?'

Yes, it's supposed to return false, as in the original post, you could see that parsing the crafted URL resulted in a valid URL, now it results in an invalid URL (correct behavior)

@agentrickard: I don't have time to touch this right now, but I think my previous statement was wrong... that is, as far as I rememeber, drupal_goto *can* redirect to an external URL, you just can't do that through the 'destination' query parameter. This means you can do it if you have PHP privilege, but not by using crafted URLs. Let me know if there is still a legit case where you'd need this power in the query parameter. To me it feels dangerous.

The only piece to check is #732542: system_goto_action breaks core APIs, where we have to decide how to handle system_goto_action(), which may end up using a destination value.

If that's the case, then we need to rethink that patch in light of this security check.

We shouldn't hold up this (critical) patch on #732542: system_goto_action breaks core APIs

We could remove the system_goto() action if it's impossible to make it work properly, agreed it shouldn't hold up this security fix.

#732542: system_goto_action breaks core APIs is also a critical security fix. Just because it's less common doesn't mean it's not.

I mean if it can't be made secure and also functional, we could remove that action from core altogether.

no, please leave the security.drupal.org tag on issues that were transferred from that site after an SA

Note: url_is_external() is either broken or misnamed. It does not check that a URL is external to the calling website. It checks that the URL is absolute. Huge difference. See #819844: url_is_external() is badly documented.

Revised and updated patch that agrees with cha0s's approach and addresses most of sun's issues.

Patch looks good. Here's a reroll with trailing space removed.

Looks good to me also.

Looks fine to me, has a nice test too.

#32: 733028-open-redirect.patch queued for re-testing.

Looks great, thanks! Committed to HEAD!

We'll have to revisit #732542: system_goto_action breaks core APIs in light of this, though.