1
\$\begingroup\$

To protect my web site against XSS injection, I am using OWASP Encoding Project.

Is it a good idea to protect all data retrieved in my DB to avoid XSS by going throuh all the data just returned by the query like this :

public function fetchDataBySql($sql,$dataArray = null,$htmlEncode = true)
{   
    try 
    {
        $db = Zend_Db_Table::getDefaultAdapter();
        $stmt = $db->query($sql,$dataArray);

        $return = $stmt->fetchAll();                    
        $this->numberRows = count($return);

        if($htmlEncode)
        {
            array_walk_recursive($return, function (&$value) {
                $value = Reform::HtmlEncode($value);
            });            
        }

        return $return;           
    } 
    catch (Exception $e)
    {
        echo $e->getMessage();
        Application_Model_DbTable_Default::logError(300,$e->getMessage());
        return false;                                        
    }  
}

and then in my view :

<div><?php echo $this->myData; ?></div>

instead of escape strings in the view like this :

<div><?php echo Reform::HtmlEncode($this->myData); ?></div>

Is there any case where the first solution is not secure?

\$\endgroup\$
  • 3
    \$\begingroup\$ Just follow the rule "Filter input, escape output". Be paranoid :-) \$\endgroup\$ – Serge Kuharev Jun 3 '14 at 15:36
4
\$\begingroup\$

One might forget HTML encoding, but how about doing it automatically:

class HtmlEncode {
    private $obj;

    public __construct($obj) {
        $thos->obj = $obj;
    }

    public __call($method, $args) {
       $ret = call_user_func_array($this->obj, $args);
       if(is_scalar($ret)) return Reform::HtmlEncode($ret);
       return new HtmlEncode($ret);
    }

    public __get($name) {
       if(is_scalar($obj->$name)) return Reform::HtmlEncode($obj->$name);
       return new HtmlEncode($obj->$name);
    }
}

Create $data = new HtmlEncode($this) and now you can safely use:

<div><?php echo $data->myData; ?></div>
<div><?php echo $this->myMethod()->name; ?></div>

You always get either html encoded string or object wrapped in HtmlEncode class.

I wrote it out of my head, so it's untested, it should check if the method or variable is defined, handle arrays, but it's left up to you ;)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Quite same idea than mine in fact except you have created a class. But if I understand well , you suggest to apply the HTML escaping in the controller and not in the model. \$\endgroup\$ – sdespont Jun 4 '14 at 6:31
  • \$\begingroup\$ No, it's more like a view helper. This idea is taken from symfony1 template system. \$\endgroup\$ – Marek Jun 4 '14 at 7:00
4
\$\begingroup\$

It is a bad idea to have an $htmlEncode feature in your database querying function.

One reason is to adhere to the general maxim that every function should have one purpose, and only one purpose. That conventional wisdom is good advice, though I'm not sure how to make it sound convincing. You'll just have to take my word and try it. There are other reasons, though.

You should organize your code in libraries, or layers of abstraction. If you implement HTML escaping in this function, then wouldn't you also have to support HTML unescaping in your database insertion function as well? Wait, that's not likely to be useful. How about supporting JavaScript string escaping, in case you want to include the output of fetchDataBySql() into a JavaScript or JSON response? Your code will be much cleaner if you never try to mingle your layers.

It's much simpler to adopt a strategy of keeping all strings in the most natural form, then escape them just at the point of use. If you HTML-escape some strings in advance, then it will be a headache keeping track of which strings have already been escaped, and which ones haven't. You could use some form of Hungarian notation to help you keep track, but it's best to avoid the problem altogether. Instead, if you stick with the policy of storing unescaped strings in all variables, then code in your HTML template that looks like <?php echo Reform::HtmlEncode($string); ?> is obviously correct by inspection. For that reason, it is worth cluttering your HTML template with encoding calls everywhere, though you might want to create an alias to shorten Reform::HtmlEncode().

Finally, escaping HTML in your database layer is reminiscent of the PHP Magic Quotes fiasco. The PHP developers thought it would be a good idea to prevent SQL injection by making it easy to automatically insert backslashes before all "unsafe" characters. In practice, that design led to all kinds of data corruption, and programmers had to undo the weirdness with stripslashes(), all because the data was being escaped at the wrong layer. Learn from that mistake: you need to apply the appropriate escaping and unescaping at the appropriate layer, and there is no good shortcut to avoid doing it the right way.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Pretty complete and convincing answer. Thank you \$\endgroup\$ – sdespont Jun 4 '14 at 6:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.