views:

198

answers:

5

I recently had a security audit run against one of the sites I work on. This was done with the Acunetix Web Vulnerability Scanner. This came back with a bunch of results that I'm sorting through.

A lot of hits on XSS came up, but I'm not sure if they are false positives or not.

Code such as:

if(isset($_GET['variableNameX']))
    $var_to_use_in_app = mysql_escape_string(trim($_GET['variableNameX']));

Is coming back as being open to XSS. Is any page with a querystring going to come back as being potentially open to XSS, or is it smart enough to know that I'm handling this server side?

Thanks for the help.

A: 

I don't know the specific tool, but its very unlikely that it can "know" that you're handling it on the server side, since that would require solving the equivalence problem in the general case. Beside that, does the tool even have access to your mysql instance? how would it know?

Charlie Martin
+5  A: 

If there is HTML passed into the querystring parameter do you display it on the page? If so, it isn't a false positive.

In other words, if you pass have something like this: http://www.example.com/?myVar=Test

And you display the results of myVar on your page without testing for HTML/Script, then it is possible someone might change it to this: http://www.example.com/?myVar=<b>Test</b>

Or something along these lines: http://www.example.com/?myVar=<script>+doSomethingBad()+</script> (probably encoded...but you get the idea)

Micky McQuade
In this instance it is actually being passed as a var into an API from a third party product...but in other instances it is going into SQL. Does escaping the string not cause all of that to break? I tested a javascript alert and it didn't execute.
Cory Dee
Right, you should encode the HTML before writing it out to the client. The online scanning services that I've used have usually had a "reproduce" option that would show you exactly how your page was vulnerable.
Micky McQuade
A: 

A lot of tools I've used will come back with false positives. If it's auditing the actual code then might be returning a potential vulnerability and time a $_GET or $_POST variable is being assigned to a normal variable even if it's not being printed out later.

Just to be safe you should check everything out and assuming nothing is a false positive.

William
+4  A: 

$var_to_use_in_app = mysql_escape_string(trim($_GET['variableNameX']));

That's generally the wrong thing to do. The string as used internally in your application should always be the plain text version. Then you can be sure than none of your string manipulations will break it, and you won't be outputting the wrong thing to the page.

For example, if you had the submitted string:

O'Reilly

mysql_escape_string would escape it to:

O\'Reilly

which, when you output that string to the HTML page, would look silly. And if you outputted it to a form field that was then submitted again, you'd get another backslash, which would turn into two blackslashes, which if edited again would turn into four, eight... and in not long you've got strings composed of hundreds of backslashes. This is a commonly-seen problem in poorly-written CMSs and servers with the evil magic_quotes feature turned on.

If you then wanted to take the first two letters of the name to put in a database query, you'd snip the substring:

O\

and then concatenate that into the query:

SELECT * FROM users WHERE namefirst2='O\';

whoops, syntax error, that string is now unterminated. Variants of string-processing on pre-escaped strings can just as easily get you into security trouble.

Instead of this approach, keep your strings as simple unescaped text strings in your application everywhere except the final output stage where you concatenate them into a delimited literal in SQL or HTML. For SQL:

"SELECT * FROM users WHERE name='".mysql_real_escape_string($name)."';"

Note the ‘real’ function name — plain old mysql_escape_string fails for some corner cases like East Asian character sets and connections/databases with the ANSI SQL_MODE set, so in general you should always use the ‘real’ version.

You can define a function that is the same as mysql_real_escape_string but has a shorter name (eg. m()) to make this a bit less ugly. Or, better, look at mysqli's parameterised queries.

For HTML, the escaping should be done using the htmlspecialchars() function:

<div id="greeting">
    Hello, Mr. <?php echo htmlspecialchars($name); ?>!
</div>

You can define a function that does the echo(htmlspecialchars()) but has a shorter name (eg. h()) to make this a bit less ugly.

If you have missed out the call to htmlspecialchars, then your scanner is absolutely correct in telling you that your site is vulnerable to XSS. But don't feel too bad, almost every other PHP programmer makes the same mistake.

mysql_[real_]escape_string doesn't help you at all here, because the characters that break out of text in HTML are ‘&’, ‘<’, and, in attributes, ‘"’. None of those are special in SQL string literals, so mysql_escape_string doesn't touch them at all. A malicious:

<script>alert("I'm stealing your cookies! "+document.cookie);</script>

Only gets escaped to:

<script>alert("I\'m stealing your cookies! "+document.cookie);</script>

Which, as far as security is concerned, is no help whatsoever.

bobince
+1  A: 

Hi,

Acunetix is a decent tool but it is NOT a replacement for manual based App Penetration Testing. It doesn't have the logic to follow through potential exploits .....

It is certainly used in the banks and I've recommended it in Proposals (which have been approved) to conduct formal bits of work. If you have a fairly complex App that has lots of dynamically generated pages and budget /time is a concern, you would boot off with Acunetix, or similar tool to point you in the direction of potential hot spots with the App. You can then focus on those areas by manual means - whether this is manual testing or stepping through the code. This is where your Pen Test houses earn their stripes.

Clients simply don't have the resources to go through large App's thoroughly.

Oh, and bear in mind there's lots of false positives.

Noelie Dunne