Check Drupal 7 core for vulnerabilities in SA-CONTRIB-2010-066

Reading over the File token integration in system.tokens.inc shows the tokens have been sanitized where requested.

Regarding the second chunk, in function file_field_formatter_view() of file.field.inc in D7:

    case 'file_url_plain':
      foreach ($items as $delta => $item) {
        $element[$delta] = array('#markup' => empty($item['uri']) ? '' : file_create_url($item['uri']));
      }
      break;

This seems related to the fix applied in SA-CONTRIB-2010-066, so I'm guessing file_create_url($item['uri']) needs to be wrapped in check_plain()?

I'm not actually sure what to make of the third chunk, as I don't see relevant code in the current File module.

add beta blocker tag

This module does not appear on the D7 beta blocker section of the community initiative page. Should it be?

Added it there.

This is a first stab at fixing file_field_formatter_view(). This is the first time I've ever been involved with stream wrappers / file API / file.module and in addition to the code being extremely ugly, I'm pretty certain the approach is also flawed. But now you experts have a patch to review and can tell me how to do it better/right. :)

"But now you experts have a patch to review.."

after thorough testing, AFAICT Drupal 7 core is not affected, so no patch is necessary.

File field's hook implementation of hook_field_formatter_view() calls file_create_url() for the 'URL to file' format, the other formats ('generic file' and 'table of files') use hook theme_file_link() which calls file_create_url() and l(). l() also check_plain()s the file's description (link anchor text) when descriptions are used for the 'generic file' and 'table of files' formats (the 'URL to file' format is plain text and therefore doesn't use a description).

file_create_url() URL encodes public and private file schemes via the call to $wrapper->getExternalUrl():

In file_create_url():

 if ($wrapper = file_stream_wrapper_get_instance_by_uri($uri)) {
      return $wrapper->getExternalUrl();
    }
 

For the public stream wrapper this invokes:

 function getExternalUrl() {
    $path = str_replace('\\', '/', $this->getTarget());
    return $GLOBALS['base_url'] . '/' . self::getDirectoryPath() . '/' . drupal_encode_path($path);
  }
 

For the private stream wrapper this invokes:

 function getExternalUrl() {
    $path = str_replace('\\', '/', $this->getTarget());
    return url('system/files/' . $path, array('absolute' => TRUE));
  }
 

pwolanin, penny for your thoughts?

pwolanin, I think you're right about http/https/relative URLs, but whose responsibility is it to url encode those? It seems like it would be the callers.

   case 'file_url_plain':
      foreach ($items as $delta => $item) {
        $element[$delta] = array('#markup' => empty($item['uri']) ? '' : file_create_url($item['uri']));
      }
      break;

This definitely needs a check_plain().

pwolanin, yea, you're probably right.

All those url() in system_tokens() needs to be wrapped in check_plain(), or they are not safe to output in an HTML context.

This is not a question of urlencoding, but of HTML encoding. Even a properly encoded URL needs to be HTML encoded before being output. system_token() is way too late in the process to worry about URL-encoding.

         case 'raw':
-          $replacements[$original] = filter_xss($date);
+          $replacements[$original] = $sanitize ? check_plain($date) : $date;
           break;

If this is a date from format_date(), it's already HTML, so filter_xss() was correct.

This patch removes the failing tests - I think they were invalid since there is no indication in the API that I should be able to add a file with query parameters.

We probably just need to make those tests generate absolute URLs for the test CSS and JS files with query strings, right?

We have an 'external' option to drupal_add_js() and drupal_add_css(). I think that's what the test wanted to confirm.

DamZ's comment in #26 still need to be addressed

@damZ: $date comes from the snippet above posted by pwolanin, i.e. not coming from format_date().

with DamZ's comments addressed, I meant to RTBC this.

Committed to CVS HEAD. Yay. :)