taking care of XSS

Question

I ran a pen-testing app and it found a ton of XSS errors, specfically, I'm guilty of echo'ing unverified data back to the browser through the querystring.

Specifically, running this puts javascript into my page. http://www.mywebsite.com/search.php?q=%00'" [ScRiPt]%20%0a%0d>alert(426177032569)%3B[/ScRiPt].

Thankfully, no where do I let users save data to a database and display back to other uesrs, so I THINK people would only be able to hack themselves with this problem, but I still want to fix it.

The recommendation is to do this:

echo htmlentities($_POST[‘input’], ENT_QUOTES, ‘UTF-8’);

But currently I need to get this patched up asap, then go fix on a case by case basis. I have a header file I include on every page on the site, I know it's bad form, but what could blow up if I did:

array_walk($_POST, 'htmlentities');

I'll need to do it for COOKIE and GET as well. I never use _REQUEST.

Thanks

Answer 1

Blindly escaping all input on the front end would mean that any part of your program that dealt with that input would have to handle html-escaped versions of <, >, &, etc. If you're storing data in a database, then you would have html-escaped data in your database. If you use the data in a non-html context (like sending an email), people would see &lt; instead of <, etc.

You probably just want to escape when outputting.

Answer 2

the code above didn't work, but this does:

$_POST = clean_input($_POST);
$_GET = clean_input($_GET);
$_COOKIE = clean_input($_COOKIE);
$_SESSION = clean_input($_SESSION);

function clean_input($array){
    if(count($array)){
     foreach ($array as $key => $value) {
      $array[$key]=htmlentities($value, ENT_QUOTES, 'UTF-8');
     }
    }
    return $array;
}

I'm just trying to figure out what could go wrong here.

Answer 3

My initial response would be to suggest that you first ensure that your presentation logic is handled separately from your business logic, etc.

If the presentation logic is indeed separate then you would need to evaluate how you are currently outputting to the screen. Can you run every output through the same function call that escapes everything output to the screen?

For example call clean_output when you go to actually output to the screen but I wouldn't do it prior to database manipulations or business logic manipulations on the data because chances are you'll escape something that should have been left as is.

Answer 4

HTML-escaping on the way in is obviously The Wrong Thing, but could be a temporary fix until you replace the code with something proper. In the long term it would be unmaintainable and you'll have loads of weird application-level errors anywhere you start doing substring manipulations (including truncation, which your database may do automatically) across &-encoded characters. It's not that likely to lead to security breaches, although you can't tell without looking at the app in a lot more detail.

If you start encoding things in $_SESSION each time, you'll get multiply-encoded too-long strings like &amp;amp;amp;amp;amp;amp;amp;amp;amp;amp;amp;amp;amp;amp; very quickly.

I THINK people would only be able to hack themselves

Or, an attacker on another web page could redirect or iframe to yours, with enough script injected to display a fake login box that looks just like your site's, harvest the username and password or automatically delete their account. Stuff like that. Not very good.

The recommendation is to do this: echo htmlentities($_POST[‘input’], ENT _QUOTES, ‘UTF-8’);

No need for htmlentities and all those parameters - use htmlspecialchars.

You can save yourself a few keypresses using something like:

function h($s) { echo(htmlspecialchars($s)); }
...
<?php h($POST['input']) ?>

It's really not that much extra hassle.