Dear all, 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', '%user_data'); // I escape <em>user_data</em> (safe) t('I escape @user_data', '@user_data'); // I escape user_data (safe) t('I don't escape !user_data', '!user_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 ==================================== See also <http://drupal.org/node/101496> 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, BAD, BAAAD: 1.db_query("SELECT t.s FROM {table} t WHERE t.field = $from_user"); 2.db_query("SELECT t.s FROM {table} t WHERE t.field = %s", $from_user); 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). Writing secure code ==================================== If you want to read more about security, I suggest that you start at "Input the root of all evil" <http://drupal.org/node/101495> in the "Writing secure code" <http://drupal.org/node/62304> 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. Thank you for listening, Heine Deelstra
Heine Deelstra wrote:
Depending on the placeholder's sigil, it is passed through theme('placeholder') (%) or check_plain (@) automatically.
t('I escape %user_data', '%user_data'); // I escape <em>user_data</em> (safe) t('I escape @user_data', '@user_data'); // I escape user_data (safe) t('I don't escape !user_data', '!user_data'); // XSS vulnerability
Somewhere in the preparation of themail, I horribly mutilated the call to t(), which should have read: t('I escape %user_data', array('%user_data' => $data)); t('I escape @user_data', array('@user_data' => $data)); t('I don't escape !user_data', array('!user_data' => $data)); Regards, Heine
Am 19.12.2006 um 15:30 schrieb Heine Deelstra:
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);
Grep Drupal core for "IN (%s)", there are some occurences without the array_fill you suggest. Besides, if count($from_user) == 0, PHP will throw a warning. Also note that if you use this and have additional parameters, you have to add them to the $from_user array as all following parameters will be discarded if the second parameter is an array. Konstantin Käfer – http://kkaefer.com/
Konstantin Käfer wrote:
Am 19.12.2006 um 15:30 schrieb Heine Deelstra:
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);
Grep Drupal core for "IN (%s)", there are some occurences without the array_fill you suggest. Besides, if count($from_user) == 0, PHP will throw a warning. Also note that if you use this and have additional parameters, you have to add them to the $from_user array as all following parameters will be discarded if the second parameter is an array.
Please keep in mind that core is not gospel. Basic sanity checks of course still apply: if $from_user is empty you have an empty IN() clause regardless. The note about multiple parameters is spot on: all arguments go in the array. Thank you. Heine
Am 19.12.2006 um 17:34 schrieb Heine Deelstra:
Basic sanity checks of course still apply: if $from_user is empty you have an empty IN() clause regardless.
Well, if $from_user is empty, count($from_user) will return 0. The PHP Manual states that the second parameter to array fill needs to be
0.
Konstantin Käfer – http://kkaefer.com/
This "guide" should be a handbook page. Otherwise its usefulness will be short lived. ..chrisxj
On 19 Dec 2006, at 23:54, Chris Johnson wrote:
This "guide" should be a handbook page. Otherwise its usefulness will be short lived.
Or examples in the PHPdoc code, even. Thanks for spreading best practices, Heine! -- Dries Buytaert :: http://www.buytaert.net/
I posted this here until someone from the doc team can tidy it up, merge it, ..etc. http://drupal.org/node/62304#comment-185423 On 19 Dec 2006, at 23:54, Chris Johnson wrote:
This "guide" should be a handbook page. Otherwise its usefulness will be short lived.
participants (5)
-
Chris Johnson -
Dries Buytaert -
Heine Deelstra -
Khalid B -
Konstantin Käfer