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.
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))),
uc_credit.module line 337 bad: ... 'data' => $arg1->payment_details['cc_owner']); good: ... 'data' => check_plain($arg1->payment_details['cc_owner']));
uc_payment.module line 564 bad: print $output . $debug; good: print $output;
uc_credit.module line 337 bad: ... $payment->comment, good: ... filter_xss_admin($payment->comment),
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)));
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";
uc_file.module line 899
bad: $url = explode("_autocomplete_file/",$_SERVER['REQUEST_URI']);
good: $url = explode('_autocomplete_file/', request_uri());
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().
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
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.
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.
Comments
Post new comment