0

I'm still learning PHP and SQL. I'm trying to create a simple content management system for a website's list of events. All of the input form fields are either Text areas or Text boxes (yes, I want them that way), and I want to leave the user the ability to add HTML links in addition to text in these fields. The following functions seem a good place to start with sanitizing the input I get from the user, but since I'm new to this I wanted to get the opinions of more knowledgeable developers. What more should I be doing to try to protect the database?

P.S. Thanks to CSS-Tricks for these functions.

function cleanInput($input) {

    $search = array(
         '@<script[^>]*?>.*?</script>@si',   // Strip out javascript
         '@<style[^>]*?>.*?</style>@siU',    // Strip style tags properly
         '@<![\s\S]*?--[ \t\n\r]*>@'         // Strip multi-line comments
    );

    $output = preg_replace($search, '', $input);
    return $output;
}

function sanitize($input) {
    if (is_array($input)) {
       foreach($input as $var=>$val) {
          $output[$var] = sanitize($val);
       }
    }
    else {
       if (get_magic_quotes_gpc()) {
          $input = stripslashes($input);
       }
       $input  = cleanInput($input);
   $output = htmlentities($output);
       $output = mysql_real_escape_string($input);
     }
     return $output;
}
Leann
  • 216
  • 2
  • 4
  • 10
  • 4
    All of that is useless junk EXCEPT for `mysql_real_escape_string()`. That's the **ONLY** thing that will protect your DB... until it's removed from PHP, because it's a deprecated function. You're just throwing the kitchen sink at the problem. you don't need html/js filtering junk when you're dealing with a db. – Marc B Feb 14 '13 at 17:15
  • @Marc B on a tear today. – Jason McCreary Feb 14 '13 at 17:19
  • @MarcB Sanitization is appropriate if a restricted subset of the input should be permitted. Otherwise, yes, you should not be sanitizing, you should be escaping. – Francis Avila Feb 14 '13 at 17:24
  • 1
    You don't account for `on*` HTML attributes which allow arbitrary JS, nor do you account for the `behavior` IE CSS property that can do a lot of evil, and it would probably be possible to nest script tags in a way that will bypass the regex. – zneak Feb 14 '13 at 17:27
  • 1
    @francis: then there should be dedicated functions for each type of sanitization. this code is about as useful as PHP's original moronic assumption that "php will never be used for anything but database queries, so let's addslashes() EVERYTHING!". – Marc B Feb 14 '13 at 17:28
  • 1
    This code is terrible, no question. – Francis Avila Feb 14 '13 at 17:30
  • @MarcB - Isn't it good to strip out input from the user that you don't want to store in the database, as well as ensure it can't alter the DB? – Leann Feb 14 '13 at 17:53
  • Besides the feedback you've already received take a look at HTMLPurifier: http://htmlpurifier.org/ – Benny Hill Feb 14 '13 at 17:55
  • @leann: you're trying to use this function as a blank "make everything safe, always" function. that rarely EVER works in the real life. once data is in the db, it can't DO anything to do the db, which is why m_r_e_s() is the ONLY function in all that which will protect your db. – Marc B Feb 14 '13 at 18:19
  • There's also the glaring bug that you do m_r_e_s() on $input, yet are doing htmlentities() on $output. so basically you completely SKIP the htmlentities step on what you're sticking into the db. – Marc B Feb 14 '13 at 18:20
  • @Benny - I'm looking into the HTMLpurifier. Thank you. – Leann Feb 14 '13 at 18:27
  • @MarcB - You're right. That's a typo. It was supposed to be htmlentities($input). – Leann Feb 14 '13 at 18:28

2 Answers2

1

Quite easily:

$testinput = "<script>alert('p0wned');</script >\n
    <a href='http://example.org' onclick=\"alert('p0Wned again!)\">Click me!</a>";

var_export(cleanInput($testinput));

Also, htmlescape is almost always the wrong thing to use--it will mangle utf8 input. Also, you should not be storing html-escaped data in your DB. I'm not even sure why you use it here at all--won't you have to unescape the html to display it?

However you are going about this the wrong way.

  1. Do not parse/sanitize html with regexes. Use a real html parser such as DOMDocument or html5lib or even tidylib. Unfortunately PHP doesn't seem to have anything as wonderful as Bleach on Python, so you will have to roll your own. An XSLT stylesheet with a whitelist seems like it might be a good way to handle this particular sanitization condition. Update: another user pointed out HTML Purifier, which is also a whitelist-based html sanitizer. I've never used it but it looks like "Bleach in PHP". You should definitely investigate.
  2. Prefer escaping to sanitization. PHP culture has an obsession with sanitization which is really just plain wrong. Escape data at the boundaries of your application (output and database). In the core of your application your data should be in its native form without any escaping.

A general outline of processing is like so:

  1. Input

    1. Turn off magic quotes in your php settings. Include code at the top of your app to fail hard if it's on: if (get_magic_quotes_gpc()) die ('TURN OFF MAGIC QUOTES!!!!');
    2. Validate and normalize/sanitize specific fields of your input according to the expected type of each field. For example, a "dollar amount" has different validation criteria than a whitelisted html fragment field. (Probably you should find and use a validation library.)
    3. If there are errors, send them back to the user with an appropriate HTTP response code.
    4. Save your data to the database using a database library that supports parameter binding, such as PDO library with prepared statements. This way you do not need to remember to escape data by hand.
    5. On success, redirect (code 303) to a page displaying the created or modified record.

  2. Output

    1. Retrieve data from the database.
    2. Feed the data to a template which is PHP code that only deals with html display of data structures. It should not know details of how that data is retrieved or contain any "application-driving" behavior. Treat a template like a function that accepts a data structure and returns a string.

    3. Escape your data inside your template. Individual fields of your data will need to be escaped differently. You almost always need to run it through htmlspecialchars before output; the only case you would not do that is when the data you need to display is already html (i.e. your whitelist-sanitized html fields). Define a helper function like this and use it in your templates:

      function h($str) {
    return htmlspecialchars($str, ENT_QUOTES, 'utf-8');
}

      Even better, try to use a template library that automatically escapes strings for you and that requires you to turn off escaping explicitly. (The common case should be simple to avoid errors, and having to escape is the common case!)

    4. Your html page is the string returned from your template. You may now display it to the user.
Francis Avila
  • 31,233
  • 6
  • 58
  • 96
  • All the comments above are really interesting, but confusing as well. There is so much information, and different opinions, about how to treat input from forms. I'm having a hard time sorting through it all to find a good base to learn from. Suggestions? – Leann Feb 14 '13 at 17:51
  • To respond directly to your answer, how does your code above hack the function exactly? var_export will show the value of the $testinput variable, but if I understand the function correctly the script tags in that variable will be stripped out. And yes, I'm planning to decode the HTML characters upon output. The user needs to be able to add HTML links to the database that will render properly when printed. – Leann Feb 14 '13 at 18:02
  • @Leann there is a space in the closing script tag. the regex will not match that. – Jonathan Kuhn Feb 14 '13 at 18:06
  • Ah, I didn't see that. Sorry. – Leann Feb 14 '13 at 18:15
  • @Leann, I expanded the answer with the best-practices for handling web input and output. – Francis Avila Feb 14 '13 at 19:03
0

While you don't have to sanitize your own string data that you display in the browser or store in a database, you must sanitize all user input that your website obtains through INPUT elements, TEXTAREA elements, from the keyboard via JavaScript/DOM Events, from uploaded files, and from all the other sources I've forgotten to list.

While database sanitizing is well-documented, and partially enforced in the latest version of server-side languages like PHP, there is still no universally-accepted way to sanitize the other sources of user input that I listed.

My own contribution is this little piece of PHP code, that allows any user input to be displayed on a web page or sent to another web page through GET or POST controls and fields in FORM elements or through Ajax without opening your website to malicious use:

function HTMLToSafeHTML($Str)
    {
    return str_replace(['&','<','>','"','\''], ['&amp;','&lt;','&gt;','&quot;','&apos;'], $Str);
    } // HTMLToSafeHTML

To use this function correctly, you must identify and track all user input, then call this function before displaying or otherwise allowing the user input to be interpreted as part of Web processing or programming. Identifying user input allows you to call this function only once. Calling it more than once will display its hard-to-read encoding, which is not useful as text.

For example, if you want to display an error message that shows some user input in boldface, you have to call HTMLToSafeHTML (which you can give a shorter name) on the user input before enclosing it in <strong>...</strong> to make it boldface. While it is harmless to display "<strong>", it is anything but harmless to display user input that might be the result of malicious users trying quite deliberately to break into your website in order to spread a virus or for some other evil purpose.

David Spector
  • 1,520
  • 15
  • 21