Step Secure the Node List

The node-list feature the Vulnerable module has several major XSS and SQL injection problems. This page provides two features:

It can be accessed with a number in the URL, in which case it will load that node and display it.

If it is accessed without any additional arguments, it will simply display a list of all the nodes on the site.

This presents several problems. First things first, though. The single case:

$access = db_result(db_query("SELECT n.nid FROM {node} n WHERE n.nid = $node->nid"));

drupal_set_message($node->title);

This code is both weak to exploitation and does too much work. It would be possible to fix this code while maintaining its basic structure with the following changes:

$access = db_result(db_query(db_rewrite_sql(''SELECT n.nid FROM {node} n WHERE n.status = 1 AND n.nid = %d''), $node->nid));

drupal_set_message(check_plain($node->title));

Adding in the db_rewrite_sql, moving the query variable into a parameter, adding a check that the node is published, and adding a check_plain to the node title will all protect this code from SQL injection, access bypass, and XSS attacks. But it still does too much work. Chapter 9 showed the proper way to use node_access to reduce the effort in this example:

if (node_access('view', $node)) { drupal_set_message($node->title);

Now for the bigger listing of nodes. Once again, you have to deal with the usual suspects of XSS, access bypass, and SQL injection. However, there is one other potential problem with this page. One element of the definition of security you are working with relates to scalability: ''the site cannot be forced off-line or into a degraded mode by a remote visitor.'' A page that attempts to list every single node on a site with hundreds, thousands, or hundreds of thousands of nodes would be a very resource-intensive page. Instead the page should be modified so it shows a subset of the nodes on the site at any given time:

$query = db_rewrite_sql("SELECT n.nid, n.title, nr.body, nr.format FROM 1{node} n

INNER JOIN {node_revisions} nr ON n.vid = nr.vid WHERE n.status = 1 ORDER BY nid DESC");

$item[] = l($result->nid, 'node/1. $result->nid) .' '. check_plain($result->title) .' | '. check_markup($result->body, $result->format);

The query is now wrapped in db_rewrite_sql. It checks that the nodes are published and pulls in the node format field from the node_revisions table for use later. The query is sent through pager_query so that it will query only a range of 10 records at a time. The results are built using the l function, which automatically does a check_plain on the text of the URL. The title of the node is manually sent through check_plain to filter it. And the node body is now sent through check_markup with the format as the second argument so that all filtering rules will be applied to the nodeā€”for both security and presentation purposes.

The two major differences to help make this page scalable are that the query now runs only on a range of nodes using the pager_query functionality and that there is a pager added to the bottom of the output via the call to theme('pager'...). If you have ever built a pager before and weren't aware of this feature of Drupal, you are probably ecstatic at seeing how easy it is to build a pager in Drupal. This is yet another example of how building things in the ''Drupal way'' should be easier and safer.

This scalability weakness of the code is something that most security reviews of the code would miss. It might not be identified until the site is live and performance impacted. It's important as you write and review code that you consider multiple potential perspectives. The first review is usually to confirm whether the code achieves the functionality and does so in a manner that matches the look and feel of the site. However, you should also do a second review to make sure that the code follows the security best practices for common problems. Finally, try to think of creative ways to use the functionality to damage the site. Only with vigorous attention to security can you ensure the safety of your site.

Was this article helpful?

0 0

Post a comment