PHP cross site scripting vulnerability

[xoggoth]

Got a Sitelock warning about
"Cross site scripting vulnerability found in args:back,fwd,maxpage,page"

This is a very simple page that allows you to click on a back or forward button and scroll between a set of pictures.

Eg forward button code:

if (isset($_POST['fwd']))
{
$page = $_POST['page'];
$maxpage = $_POST['maxpage'];
$page++;
if ($page>$maxpage)
{
$page = 1;
}
else
{
$fwd = "visible";
}
$back = "visible";
}

HTML stuff here

<?php
echo "<img src='month$page.jpg' width='950' alt='$page'>";
?>

more HTML

<form name="page" action="<?php echo htmlentities($_SERVER['PHP_SELF']); ?>" method="post">
<?php
echo <<<END
<div class="content-back"><input style="visibility:$back" type="submit" name="back" value=""></div>
<div class="content-fwd"><input style="visibility:$fwd" type="submit" name="fwd" value=""></div>
<input type='hidden' name='page' value='$page'>
<input type='hidden' name='maxpage' value='$maxpage'>
END;


Any ideas why that would be vulnerable? Cheers.

[vwdan]

It's because you're using unsanitised variables directly within the script. As it happens, because you're just putting them into vanilla HTML then I don't think it's really a vulnerability (It's no different to what one could do with the Developer Tools in Chrome etc) but it'd get dangerous if you were putting them into any calls or DB queries.

Try using something like this: http://php.net/manual/en/function.htmlspecialchars.php

$page = htmlspecialchars($_POST['page']);

[xoggoth]

Much cheers. Their message is a bit misleading but putting that for $page seems to have sorted it. I was already using HTML entities in the form to self post but not similarly protecting the page bit.

[vwdan]

Much cheers. Their message is a bit misleading but putting that for $page seems to have sorted it. I was already using HTML entities in the form to self post but not similarly protecting the page bit.

Ah yes - never even spotted that bit.

Glad you got it sorted!

[darrylmg]

I'm not 100% sure, but could there not be an issue with $maxpage?

Some unscrupulous person could inject Javascript into the query string and it could pop out within the page and be executed.
The JS could then pull in some really malicious code via Ajax...

[xoggoth]

I'm starting to wonder if their messages mean anything at all. Although this is the first email I got, I note a list of messages on their webpage. Site is ok, site has a problem, Site is ok, site has a problem, Site is ok, site has a problem.... Maybe it only has a problem on Mondays.

[NickFitz]

I'm not 100% sure, but could there not be an issue with $maxpage?

Some unscrupulous person could inject Javascript into the query string and it could pop out within the page and be executed.
The JS could then pull in some really malicious code via Ajax...

I'd definitely recommend ensuring $maxpage is a meaningful value, as it's coming in from the client then being echoed back out. At least make sure it's a number:

$maxpage = intval($_POST['maxpage'], 10);
// do sanity checking here, as an XSS attack or such would result in $maxpage being zero 

Might also be worth doing the same with $page, as there's a code path which I think won't necessarily coerce it into being a number.

[xoggoth]

Cheer chaps, will do.