SA-2008-073: Content filtering bypass, when filters are being removed

This patch went into the prior release of core, so shoudl be ported to HEAD asap.

From David Rothstein:

Hi,

I've discovered a security issue in Drupal 6 (which most likely
applies to Drupal 5 as well) regarding deleted input formats. The end
result is that when an input format is deleted, certain pieces of text
that used that format will become wide open (vulnerable to XSS
attacks), even if both the original format and the site's default
input format are safe.

A scenario where this can occur is as follows:
1. A site uses CCK to create an input-format enabled textarea. (Note:
CCK is the most obvious example, but the vulnerability exists for any
contributed module that uses input formats.)
2. Users create content in that textarea using a safe input format
which filters out XSS.
3. Later, the site administrator decides to delete that input format.
The confirmation message on the screen says "If you have any content
left in this input format, it will be switched to the default input
format." Assuming the site's default input format also filters out
XSS, this should therefore be a safe operation...
4. Unfortunately, what actually happens is that this CCK field never
gets converted; instead, immediately after the format is deleted,
Drupal will display this text with *no* filtering whatsoever. Thus,
any XSS attack that someone tried (and failed) to enter in this
textarea a long time ago will suddenly start working the next time the
node is viewed.

The vulnerability is due to a combination of the fact that Drupal does
not provide contributed modules any way to know that an input format
has been deleted (so they have no way to know that the format needs to
be switched to the default format, as core does for its own tables in
http://api.drupal.org/api/function/filter_admin_delete_submit/6), and
the fact that check_markup() does not check that an input format
exists before trying to apply it to a piece of text.

Note: I discovered this vulnerability while working on a Drupal 7
input format patch at http://drupal.org/node/11218, and I would up
having to include a fix for it in the latest patch I posted there.
However, I described it as though it were a bug (not a security
issue), and also only described it very vaguely (in fact, I made it
sound like it was an issue introduced by my patch rather than a
preexisting one), so I think it's OK :)

Based on the code I added there, a good fix for Drupal 6 that involves
minimal changes might look like this (code to be added to
check_markup):

**********

    // Get a complete list of filters, ordered properly.
    $filters = filter_list_format($format);

    + // If no filters were returned, check to make sure that this corresponds
    + // to an actual input format before proceeding. If not, we will process
    + // this input format in the same way that we process the default format.
    + // thereby ensuring that text with an unrecognized input format will not
    + // be allowed to pass through unsanitized.
    + if (empty($filters) && !db_result(db_query("SELECT 1 FROM
    {filter_formats} WHERE format = %d", $format))) {
    + $filters = filter_list_format(variable_get('filter_default_format', 1));
    + }

    // Give filters the chance to escape HTML-like data such as code
    or formulas.
    foreach ($filters as $filter) {
    $text = module_invoke($filter->module, 'filter', 'prepare',
    $filter->delta, $format, $text, $cache_id);
    }

Heine says (regarding the patch):

If the default input format is ever deleted, this bit of code gets into an infinite loop, but I cannot think of a (desireable) situation where that happened.

Gabor says:

Drupal 6 does not provide a way to remove the default input format, so how would people delete it (apart from removing it from the database)? It would be good to not leave edge case failures in if this edge case is somehow exploitable from the UI.

Also dww says:

We should also consider a hook in D7 to let contributed modules act on filters being deleted, but that's for the public queue once this lands and the SA goes out for D6 and D5. Even with such a hook, this code should be happening as the defensive default behavior.