Fix for SA-CONTRIB-2015-067 - Finder - Open Redirect - Unsupported

I would also like to know, this module has been really useful.

Me too, I've been using finder for a while and it is really useful. Thanks in advance.

Considering the project maintainer hasn't committed anything in 2+ years, someone will need to get ownership of the module transferred to them and the issue patched before we see any progress on this. As it says in the box at the top of the module page, look at hiring a developer if you have a client that needs this module.

To fix the issue in the finder/includes/form.inc file, function finder_form_goto should be changed from

function finder_form_goto($sep, $path = '', $query = NULL, $fragment = NULL, $http_response_code = 302) {

  if (isset($_REQUEST['destination'])) {
    extract(parse_url(urldecode($_REQUEST['destination'])));
  }
  elseif (isset($_REQUEST['edit']['destination'])) {
    extract(parse_url(urldecode($_REQUEST['edit']['destination'])));
  }

...
to

function finder_form_goto($sep, $path = '', $query = NULL, $fragment = NULL, $http_response_code = 302) {
  $destination = FALSE;
  if (isset($_REQUEST['destination'])) {
    $destination = $_REQUEST['destination'];
  }
  elseif (isset($_REQUEST['edit']['destination'])) {
    $destination = $_REQUEST['edit']['destination'];
  }
  
  if($destination) {
    $colonpos = strpos($destination, ':');
    $absolute = ($colonpos !== FALSE && !preg_match('![/?#]!', substr($destination, 0, $colonpos)));
    if (!$absolute) {
      extract(parse_url(urldecode($destination)));
    }
  }

...

Patch looks good

Turns out that after 7.35 and 6.35 releases this patch is no longer necessary because of https://www.drupal.org/node/2455007

this patch is no longer necessary because of...

Do security issues work like this? If so, should the security team unflag the warning on this module with a warning about the issue for sites running older versions of Drupal placed instead in the header?

Applied it, thanks. Hope the flag will be removed soon.

I think it makes sense to update the advisory and mark the module as supported when the module is maintained again.

Does anyone want to apply to maintain the module? Please see https://www.drupal.org/node/251466

Updated title to help people find it.

Does anyone want to apply to maintain the module?

Yes, see #2496049: Offering to maintain finder.

I originally set this patch to RTBC in #6, but I now see that is not following Drupal coding standards.

Even if the vulnerability can't be reproduced any longer (because of https://www.drupal.org/node/2455007), the resulting code should match Drupal core.

Here is the updated patch, against 7.x-2.x.

Can we update the code to match current drupal_goto function?

drupal_goto() has been updated in multiple security fixes (for example, now is calling url_is_external(), and that function was updated as well)

function drupal_goto($path = '', array $options = array(), $http_response_code = 302) {
  // A destination in $_GET always overrides the function arguments.
  // We do not allow absolute URLs to be passed via $_GET, as this can be an attack vector.
  if (isset($_GET['destination']) && !url_is_external($_GET['destination'])) {
    $destination = drupal_parse_url($_GET['destination']);
    $path = $destination['path'];
    $options['query'] = $destination['query'];
    $options['fragment'] = $destination['fragment'];
  }

  drupal_alter('drupal_goto', $path, $options, $http_response_code);

  // The 'Location' HTTP header must be absolute.
  $options['absolute'] = TRUE;

  $url = url($path, $options);

...

I will update the goto function as a matter of course, and I've already mentioned to the security team that I will do so as soon as I can access this module again.
Furthermore the security issue no longer stands because of changes to core that prevent the exploit.

@danielb in what way are you prevented access to the module?

Because of the red message on the project page I cannot edit the project or make changes to the code, etc...

You have commit access and ability to create new releases.

The process from this point is:

* Upload a patch here to fix the bug and get reviews.
* Commit to git, tag and make a release with the "Security update" tag
* Security team will publish it and remove the big red warning

What are you talking about? Why isn't this being dealt with in SA-CONTRIB-2015-067? Who are you? Why do I need to upload a patch here? I've already uploaded one to SA-CONTRIB-2015-067. Why does the security team need to create a "security update" if there is no longer a security hole?

Here's the patch, but this all seems very odd to me.

Oh I just checked, you're on the security team. OK then :)

What are you talking about?

:)

Why isn't this being dealt with in SA-CONTRIB-2015-067?

Do you mean "why isn't this being dealt with in private?" If so, the reason is that there's no need to discuss in private now that it's in the public queue. That helps the security team focus on things a bit better.

Who are you?

:)

Why do I need to upload a patch here? I've already uploaded one to SA-CONTRIB-2015-067.

Yeah, same as earlier. Once it's closed in the private queue all further work moves to the public queue.

Why does the security team need to create a "security update" if there is no longer a security hole?

The "security update" tag on the release node will alert the update status module so that sites using the old version of the module will see that there is a release available that they should upgrade to. Right now people just see "it's not supported" and this will let them see "supported, upgrade!"

That makes sense, thanks for explaining.
The patch is pretty much a sure-thing I reckon, especially with the added core protection. I'm keen to start fixing some bugs in this project but let's get this particular thing sorted out first. If there are no objections I will commit it soon.

Updated project page and published 7.x-2.1 security release.

Let me know if you need anything else.

Thanks!