0

I am using comment system like this:(i have simplified code for this question)

 <?php
        $status_ui = '<textarea id="box"></textarea>';
        $status_ui .= '<button id="status_button" onclick="postComment(\'box\')">Post</button>';
    ?>
    <html>
    <head>
        <script type="text/javascript">
        function postComment(box){
            var data = document.getElementById(box).value;
            data = data.replace(/[^a-zA-Z0-9,.?! ;:'"]+/, '');  
            var hr = new XMLHttpRequest();
            hr.open("POST", "comment_system.php", true);
            hr.setRequestHeader("Content-type", "application/x-www-form-urlencoded");
            hr.onreadystatechange = function() {
                if(hr.readyState == 4 && hr.status == 200) {
                    var html_output = "";
                        current_html = document.getElementById("comment_list").innerHTML;                   
                        html_output +=  '<div class="comment">';
                        html_output +=      data;
                        html_output +=  '</div>';
                        document.getElementById("comment_list").innerHTML = html_output + current_html;
                    }
                    results_box.innerHTML = html_output;
                }
            }
            hr.send("comment="+data);
        </script>
    </head>
    <body>
        <div id="container">
            <?php echo $status_ui; ?>        
            <div id="comment_list"></div>
        </div>
    </body>
    </html>

Users can enter only a-zA-Z0-9,.?! ;:'" and everything works super cool.

In comment_system.php, I am performing all kinds of checks, regex, in order to store my data safely to DB. The question is: how to output the user comment securely to the comment_list div to be XSS proof?

I am using string.replace,and I am aware that it can be bypassed by the bad user.

  • 3
    You're doing your regex check in javascript, so of course it can be bypassed. Are you doing the same on server side? – developerwjk May 27 '14 at 19:57
  • 1
    Complementing what @developerwjk said, also on the server side, make sure you're using [prepared statements](http://www.php.net/manual/en/mysqli.quickstart.prepared-statements.php) – mathielo May 27 '14 at 20:01
  • On the server side I am performing A LOT OF checks in order to be secure(strict white listing, regex, functions, length checks, PDO prepared statements and I am going to use HTML purifier). I made it as secure as possible for initial version. But I am concerned about outputting it securely. – user3368512 May 27 '14 at 20:04
  • Someone can put bad staff inside the input and when I output it can be nasty. Hence, is there any way to stop that(as much as possible)? I have been reading about AntiSamy and Caja project. Are they effective when bad user can bypass everything in JS file. – user3368512 May 27 '14 at 20:08
  • possible duplicate of [How to prevent XSS with HTML/PHP?](http://stackoverflow.com/questions/1996122/how-to-prevent-xss-with-html-php) – HamZa May 27 '14 at 22:24

1 Answers1

2
                    html_output +=  '<div class="comment">';
                    html_output +=      data;
                    html_output +=  '</div>';

This is HTML injection, resulting in a client-side cross-site-scripting (DOM-XSS) vulnerability.

To stop active HTML content being included in the comment div, either:

  • escape HTML-special characters in the data (eg < to &lt;)
  • (better) use DOM methods to set the text in the div element instead of trying to create markup

document.getElementById("comment_list").innerHTML = html_output + current_html;

This is a bad idea. Every time you call this you are serialising the entire contents of the comment list to a new HTML markup string, adding to the string, destroying all the elements in the comment list, and parsing new elements from the markup. This is slow and prone to reserialization errors which also can lead to DOM-XSS (IE has trouble with the backquote character - reading innerHTML doesn't necessarily give you a string that is safe to re-assign to innerHTML).

Again, avoid this by creating your content with DOM methods:

var comment = document.createElement('div');
comment.appendChild(document.createTextNode(data));
var list = document.getElementById('comment_list');
list.insertBefore(comment, list.firstChild);

(And similarly for the results box.) Now it doesn't matter what funny characters might have been in the user input, you will always display correctly. Consequently you can allow users to type a less-then sign without mangling their comment.

In comment_system.php, I am performing all kinds of checks, regex, in order to store my data safely to DB.

Input filtering has its place but it is not the answer to SQL injection. You should be using query parameterisation (with mysqli or PDO) to get data into the database and not creating queries with data in them. Then, again, any input is safe and you can allow the user to type an apostrophe without mangling their comment.

bobince
  • 528,062
  • 107
  • 651
  • 834
  • This is one of the best answers I have ever read!!! 1. Explained what is wrong and why;2. Suggested the solution and explained why. Thank you so, so, so much! I am already using PDO prepared statements with bind parameters. :) Best regards from Serbia – user3368512 May 28 '14 at 21:05
  • I want to thank you AGAIN for pointing me to the right direction. Last two days I have been applying your advice to all my scripts, and now I feel comfortable to modify every script in very short time. So BIG THX. :) – user3368512 May 30 '14 at 21:08
  • +1 excellent answer. For those interested in the relatively new kind of XSS which Bobince mentioned, it is also know as mXSS. See http://security.stackexchange.com/q/46836 and http://cure53.de/fp170.pdf – Fabrício Matté Jun 08 '14 at 06:18