Writing secure code

Know about a security issue? Please alert the security team.

Whether you're writing a PHP snippet or an entire module, it's important to keep your code secure.

Here's how you prevent three major security risks:

Cross site scripting attacks by properly checking output
SQL injection attacks by using the database abstraction layer
Node access restrictions bypass by using db_rewrite_sql

To prevent Cross site scripting (XSS) attacks, read the How to handle text in a secure fashion page. To sum up that page: If something that you output is not surrounded by one of the various check_* functions, it is very likely that it's insecure.

Second, you need to utilize the database layer correctly. Never, ever write user data into your SQL. You need to read db_query docs on the syntax. Common and very insecure practice is to simply end your query with something like

db_query('SELECT foo FROM {table} t WHERE t.name = '. $_GET['user']);

Instead, you must use

db_query("SELECT foo FROM {table} t WHERE t.name = '%s' ", $_GET['user']);

A non-trivial example is when you want to list all nids that are in an array $content_types:

<?php
$args = $content_types;
$placeholders = array_fill(0, count($args), "'%s'");
pager_query(db_rewrite_sql('SELECT n.nid, n.title FROM {node} n INNER JOIN {term_node} tn ON n.nid = tn.nid WHERE n.type in ('. implode(',', $placeholders) .') AND n.status = 1 ORDER BY n.created DESC'), 10, 0, NULL, $args);
?>

We are using array_fill to create an appropriate number of placeholders and then we utilize the possibility to pass the arguments in one array. As there is a variable in the SQL query, we should be very cautious. But as we just created this variable, it's OK.

Another very important point to note: we are dealing with nodes and the node access mechanism kicks in via db_rewrite_sql so we are utilizing it. It's really easy and yet it's so often neglected! See When to use db_rewrite_sql for further details.

So, once more; There are three kind of errors you need to avoid: XSS with proper checking, SQL injections with proper db_query usage and node access bypass by utilizing db_rewrite_sql.

» Login or register to post comments

Heine Deelstra's article

kbahey - January 4, 2007 - 02:32

Heine Deelstra, security team leader for Drupal, wrote an article on the development mailing list some time back in December 2006.

Since he has no time to refine it in a page, I am posting it below as a comment, so someone can take it and merge it with the other pages.

=========================
Porting a module is an excellent opportunity to keep an eye out for security problems (evidence: http://drupal.org/node/103943).

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: http://drupal.org/node/28984

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 don't 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 escapes:

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 and #title of form_elements need 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));
or
$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
====================================

Just wanted to add

cybermalandro - January 12, 2007 - 23:06

I just wanted to add, that besides limiting yourself to Drupal and PHP functions in order to write secured code. Please check projects such as the Open Web Application Security Project. OWASP is a very active open community and they provide good information on Web Application Security. You could start by checking the testing guide. Also, I would encourage developers to understand more about what too look for such as what is a XSS vs. XSRF etc. If you do not know what to look for you would not know what to filter.

Oh! and Did I mention audit your applications?

Writing secure code

Heine Deelstra's article

Just wanted to add

Developing for Drupal

Handbook license

Quick links

User login

Contributor links