PHP strpos() returning odd results

Question

Hi folks,

I've written a basic 'security checker' for my webapp. I'll need to see at a glance whether user-submitted code contains evil stuff.

Here is a screenshot of the code that I'm running this against right now: http://cl.ly/677a6dc40034f096697f

Here is the PHP code I'm using against these three bits of code:

<!-- The View -->
<h2>Security analysis</h2>
<?php echo securitycheck($html, $css, $js); ?>

-

// The controller
function securitycheck($html, $css, $js)
{
    // The code is the html, css, and js, appended together. We're scanning it all.
    $code = $html." ".$css." ".$js;

    // $insecure is our array of naughty things to search for.
    $insecure = array(
                        /* HTML Elements */
                        'applet',
                        'basefont',
                        'base',
                        'behavior',
                        'bgsound',
                        'blink',
                        'embed',
                        'expression',
                        'frameset',
                        'frame',
                        'ilayer',
                        'iframe',
                        'isindex',
                        'javascript',
                        'layer',
                        'link',
                        'meta',
                        'object',
                        'plaintext',
                        'style',
                        'script',
                        'xml',
                        'xss',
                        /* Javascript Elements */
                        'alert',
                        'cmd',
                        'passthru',
                        'eval',
                        'exec',
                        'expression',
                        'system',
                        'fopen',
                        'fromcharcode',
                        'fsockopen',
                        'file',
                        'file_get_contents',
                        'readfile',
                        'unlink',
                        /* Misc Elements */
                        'vbscript:',
                        '<?',
                        '<?php',
                        '?>'
                    );

    $found = "";
    $output = "<p><strong>Potentially insecure items found:</strong> ";

    foreach($insecure as $item)
    {
        if (($pos = strpos($code, $item)) !== FALSE)
        {
            $found .= "$item, ";
        }
    }

    if ($found == "")
    {
        $output .= "None.<br/>";
    }
    else
    {
        $output .= "<span class=\"alert\">".substr($found, 0, -2)."</span>"."</p><br/>";  // cuts trailing comma and space from $found
    }

    return $output;
}

Finally, here is a screenshot of the returned output (in HTML): http://cl.ly/f246dc419fb499dd6bd7

See the screenshot? There are several things wrong. The trailing space and comma haven't been cut off (which I used substr() to do, and also it's reporting two alerts when as you can see from the first screenshot, there's only one been run through this.

What am I doing wrong?

Thanks!

Jack

EDIT: As Fosco kindly pointed out, alert was listed twice in my array (doh!). I've fixed that, however the problem of the trailing comma be left is still there. I know this is a smaller issue but I swear it still shouldn't be there...

Answer 1

An easier way of dealing with the found items would be to use...

$found = array();

foreach($insecure as $item)
{
    if (($pos = strpos($code, $item)) !== FALSE)
    {
        $found[] $item;
    }
}
$found = implode(', ', $found);

And there's only one alert in the string, but it in your $insecure list twice, hence the fact it appears in the output twice. To avoid that you would have to scan each section separately.

Answer 2

At a glance your code looks like it should give the output you want. I am not sure what is going wrong.

Instead of building $found as a string, I would recommend constructing it as an array and then using implode() to get a string:

• replace $found = ""; with $found = array();
• replace $found .= "$item, "; with $found[] = $item;

and replace this block of code:

if ($found == "")
{
    $output .= "None.<br/>";
}
else
{
    $output .= "<span class=\"alert\">".substr($found, 0, -2)."</span>"."</p><br/>";  // cuts trailing comma and space from $found
}

with this:

if (!count($found))
{
    $output .= "None.<br/>";
}
else
{
    $output .= "<span class=\"alert\">".implode(', ',$found)."</span>"."</p><br/>";  // cuts trailing comma and space from $found
}

Answer 3

Don't reinvent the wheel.

http://htmlpurifier.org/

Answer 4

Have you tried just echoing $found and then doing a View Source? I will guess that the issue is that one of your items is not being displayed based on HTML encoding ('<?') and that there is a comma and space that actually is being removed.

But I will echo @Hammerite with his solutions.