Do a quick security review when porting your module

Heine —Thu, 2007/02/22 - 08:48

Adapted from a mail I sent to the Drupal development list.

Porting a module is an excellent opportunity to keep an eye out for security problems (evidence: DRUPAL-SA-2006-031). Here's a quick security reminder regarding input (user-supplied data). Code samples are only included to make a point, do not hold them against me.

Escape or filter text before display

If you do not escape or filter text before display, you enable a user to insert arbitrary HTML and scripts into pages. This type of attack is know as cross site scripting aka XSS.

For more information, see: Handle text in a secure fashion.

Escape / filter:

check_plain or theme('placeholder') for plain text.
check_markup or filter_xss for markup containing text.

Drupal 5 brought a (brilliant IMO) change to t():

Depending on the placeholder's sigil, it is passed through theme('placeholder') (%) or check_plain (@) automatically.

t('I escape %user_data', array('%user_data' => $data)); // I escape <em>user_data</em> (safe)
t('I escape @user_data', array('@user_data' => $data)); // I escape user_data (safe)
t('I do not escape !user_data', array('!user_data' => $data));// XSS vulnerability

Sometimes the difficult part is to know whether a function requires escaped text or does the escaping itself:

Automatically escape:

l() - escapes text and attributes unless you pass TRUE for the $html parameter. In that case only attributes are check_plained.
menu items & breadcrumbs
block descriptions (*not* titles)
theme('username')

* Common pitfalls:

watchdog - you have to provide a safe $message

drupal_set_title - you have to provide a safe $title

Form elements - #description needs to be safe

Form elements - #value of #type markup and item need to be safe. Note that the default form element #type is markup!

Examples:

drupal_set_title($node->title); // XSS vulnerability
drupal_set_title(check_plain($node->title));

watchdog('content', t("Deleted !title", array('!title' => $node->title)); // XSS
watchdog('content', t("Deleted %title", array('%title' => $node->title)); // or @

$form['unsafe'] = array('#value' => $user->name); // XSS
$form['safe'] = array('#value' => check_plain($user->name));
$form['safe'] = array('#value' => theme('username', $user));

$form['bad'] = array(
'#type' => 'textfield';
'#default_value' => check_plain($u_supplied), // bad: escaped twice
'#description' => t("Old data: !data", array('!data' => $u_supplied)), // XSS
);

$form['good'] = array(
'#type' => 'textfield';
'#default_value' => $u_supplied,
'#description' => t("Old data: @data", array('@data' => $u_supplied)),
);

Database (ab)use

See also Database access and When to use db_rewrite_sql in the Writing secure code pages.

Summary: Whatever you do, *never* use user supplied data directly in a query. Use db_query as it was meant to be used: data goes in arguments. And make sure to enclose %s in single quotes: '%s'.

BAD:

db_query("SELECT t.s FROM {table} t WHERE t.field = $from_user");
db_query("SELECT t.s FROM {table} t WHERE t.field = %s", $from_user); // Will result in an error.

Good:

db_query("SELECT t.s FROM {table} t WHERE t.field = '%s'", $from_user);

BAD ($from_user is an array of numbers):

db_query("SELECT t.s FROM {table} t WHERE t.field IN (%s)", $from_user);

Good:

$placeholders = implode(',', array_fill(0, count($from_user), "%d"));
db_query("SELECT t.s FROM {table} t WHERE t.field IN ($placeholders)", $from_user);

Anything else: bad. Why is this bad? Suppose you check the data that you stuff in to a query (e.g. 'all numeric'). Or, you know a user name is validated to contain only certain 'safe' characters.

The problem is that you open the possibility that a code path develops where unfiltered content ends up at your query. In that case you're toast.

This code path can arise because you 1) accidentally remove validation in future development, 2) a user adds a module that is more lenient in the data it accepts or 3) you add a bunch of code that skips validation (recent xTracker problem).

When you query the database for content to display to users, make sure to use db_rewrite_sql. For more information see When to use db_rewrite_sql.

Writing secure code

If you want to read more about security, I suggest that you start at Input the root of all evil in the Writing secure code pages.

I hope to expand and improve those pages in the future, but some of them are already useful.

You found a vulnerability?

What to do when you find a vulnerability in your module? Simple; write us at security@drupal.org.

Heine