Drupal CSS security

In May 2008, ubercart 1.0 was released and security vulnerabilities were patched. With 80% of all attacks today involving injection attacks like XSS and SQL injection we developers need to learn how to handle them. I put together this summary of the fixes and a cheat sheet of ways to cleanse data.

But there are a few things I want to mention up front.

I'm using Ubercart as an example only because a security alert got to me about the time I was putting this paper together. I'm not trying to point fingers here and since UberCart cleaned their code up very well they deserve praise. The kinds of mistakes made will always start to surfaced whenever a popular product starts to really mature. As an example, look at Microsoft. No matter what you think of Windows, you have to admit that it is a popular product and it seems to have gone through some of those maturing phases.

This is not a comprehensive code review, it started as a study for myself and grew from there. I only looked at the lines that were changed in the fixed version and choose those changes that apparently illustrated the error. A couple of lines I choose were not a security problem if you looked deep enough. Sometimes the data was cleansed elsewhere and the line was changed for other reasons.

However, of the 95 code changes I saw there were what looked like 7 basic mistakes and I composed a list of 9 lessons to learn. While most of those lessons came from what looked like mistakes, two of those lessons are my own impression of coding in a community effort like this.

mistake #1 = Using the wrong placeholder in t() function (count = 24).

uc_order.module line 2020
bad:  'title' => t('Order !order_id', array('!order_id' => arg(3))),
good: 'title' => t('Order @order_id', array('@order_id' => arg(3))),

mistake #2 = Not sanitizing output with check_plain (count = 50).

uc_credit.module line 337
bad:  ... 'data' => $arg1->payment_details['cc_owner']);
good: ... 'data' => check_plain($arg1->payment_details['cc_owner']));

mistake #3 = Printing debug information (count = 1).

uc_payment.module line 564
bad:  print $output . $debug;
good: print $output;

mistake #4 = Not sanitizing output with filter_xss_admin (count = 7).

uc_credit.module line 337
bad:  ... $payment->comment,
good: ... filter_xss_admin($payment->comment),

mistake #5 = Not sanitizing output with t() placeholders at all (count = 10).

uc_payment_order.inc line 21
bad:  $output = t('Balance:') .' '. uc_currency_format($arg1);
good: $output = t('Balance: @bal', array('@bal' => uc_currency_format($arg1)));

mistake #6 = Not sanitizing db input with db_query placeholders (count = 2).

uc_file.module line 861
bad:  $count_query = "SELECT COUNT(*) FROM {uc_file_users} WHERE uid = $uid";
good: $count_query = "SELECT COUNT(*) FROM {uc_file_users} WHERE uid = %d";

mistake #7 = Using super globals directly.

uc_file.module line 899
bad:  $url = explode("_autocomplete_file/",$_SERVER['REQUEST_URI']);
good: $url = explode('_autocomplete_file/', request_uri());

Most of the mistakes opened the system up to Cross-Site-Scripting. But there were two places SQL injections was possible and one place important information could be leaked. Using SQL indirectly which can be exploited as an injection. Variables and Watchdog messages are stored in an SQL database so they could be structured for an injection attack. In one case a super-global was used without being sanitized or using a function that would sanitize it automatically. Unfortunately, an HTTP header can be structured to include a variable that was printed with a value that would cause a XSS. Out of those 7 mistakes there are 9 lessons we can learn:

1. Cleanse data with "filter_xss_admin()" when we want to preserve all HTML except scripting and style.
2. Cleanse data with "filter_xss()" when we want to preserve HTML according to a white list of allowed tags.
3. Cleanse data with "check_plain()" when we don't need HTML at all.
4. Cleanse data with "db_query()" placeholders before it goes into persistent storage.
5. Cleanse data we want to be translatable with t() placeholders.
6. Don't give info away with output for development like debugging code.
7. Clean all persistent storage locations like variables and watchdog logs.
8. Watch for a team players weakness.
   I got the feeling that I could tell who worked on the file by pattern of errors made. I assume each author has different habits and each is prone to a different set of mistakes.
9. Watch-out for things you are too comfortable with.
   For example SQL input was cleaned very well with two exceptions. This could be because the author was relaxed about SQL injections because of that carefulness. What is the old saying? "Familiarity breeds contempt".
10. Don't use superglobals if you have a choice and cleanse them if you don't.

cleansing functions available

check_plain()
It will convert all special characters to HTML entities so "" will become "<HTML>" as the string will not execute as HTML.

filter_xss()
It will clean-up a string of all HTML that in not in a list of what you deem as ok, make sure everything is well-formed, and it will only allow safe protocols to run (see check_url()).

filter_xss_admin()
A version of filter_xss() with a very liberal white list. Everything except the script and the style tag (<SCRIPT><STYLE>) is allowed.

check_markup()
Run all the enabled input filters on a piece of text.

drupal_urlencode()
Use this in a regular URL, not a "clean url" to escape extra characters.

check_url()
Use this to verify that a URL only uses safe protocols. By default the safe protocols are: HTTP, HTTPS, FTP, NEWS, NNTP, TELNET, MAILTO, IRC, SSH, SFTP, and WEBCAL. You can alter that list with the 'filter_allowed_protocols' entry in the SETTINGS.PHP file.

mime_header_encode()
Use this to encode all non-ASCII or UTF-8 characters to a safe version.

drupal_eval()
Use this to execute a line of PHP. Something you should rarely need to do. But if you do then use druapl_eval() instead of PHP's eval().

t() placeholders

• Use one of the following symbols and the name of the value to signal an insertion.
  ! does nothing to the string
  % puts the string inside an <EM> tag
  @ runs the string through check_plain() which is listed above
  

• List the inserted strings as an associated array with the name of the placeholder (%NAME) as the key

db_query() placeholders

• Use the leading symbol % to signal a string insertion the same way PRINTF does.
  %s for a string
  %d for a numeric value
  %f float, as your locale dictates
  %b for binary data
  %%
  

• List the inserted strings in the order that they occur.

Some extra points

• All XSS cleansing should be done when a users information is displayed and not when it is stored. So just before you save a variable, do not use check_plain() or the like, just db_query().

• When trying to anticipate what the bad guys will do in the future it doesn't hurt to filter everything even if we don't think can be used against us. The Anti-Virus world got caught up in that. Instead of chasing a few viruses and worms like they did before 2000, now they are being cased by a crowd of viruses and derivatives.

• Don't try to assume something is safe. Remember when it was thought that only executable programs (.COM, .EXE in windows) could contain a virus? Along came compressed viruses and malformed multi-media headers that triggered a buffer overflow. Assumptions in programming are definitely useful but can come back to bite you.

• The forms API has a lot of built in protection when passing data from one page to another, but it is beyond the scope of this paper so I won't cover it. Later I might write another paper on Session controls.

• Learn as much about PHP vulnerabilities as possible and where it differs with Drupal. You will start to appreciate Drupal even more for protecting you from common problems with its Database abstraction and Form API.

Additional reading

• A DIFF of Ubercart 1.0beta7 and 1.0rc1
• ProDrupal Development, chapter 20, John VanDyk, Apress
• How to break web software, Mike Andrews, Addison Wesley
• Hack notes- web security, chapter 2 and page 148-9, Mike Shema, Osbourne
• The art of software security assessment chapter 18, Mark Dowd, Addison Wesley
• Puzzles for Hackers, chapter 1.5.4, Ivan Sklyarov, aList
• http://drupalecommerce.org/api/function/filter_xss/471
• http://ha.ckers.org/xss.html
• http://en.wikipedia.org/wiki/Cross-site_scripting
• http://xssed.com
• http://www.owasp.org/index.php/Cross_Site_Scripting

Drupal documentation

• http://api.drupal.org/?q=api/function/filter_xss/5
• http://api.drupal.org/api/function/valid_url/5
• http://api.drupal.org/api/function/check_url/5
• http://api.drupal.org/api/function/drupal_urlencode/5
• http://api.drupal.org/api/group/database/5

current issues (as of 06/08)

• http://drupal.org/node/242873
• http://drupal.org/node/258137