Bug 334415

Comment 1 John Arthorne 2011-01-14 14:58:40 EST

Type the following in the Orion search field:

<script>alert(document.cookie);</script>

Hit enter, and see that the script gets executed.

Comment 2 John Arthorne 2011-01-14 15:20:18 EST

It looks like this happens here in explorer-table.js:

progress.innerHTML = "Loading <b>" + path + "</b>...";

Mark, do we have a helper function somewhere for escaping/sanitizing inputs?

Comment 3 Mark Macdonald 2011-01-14 15:27:24 EST

(In reply to comment #2)
> It looks like this happens here in explorer-table.js:
> 
> progress.innerHTML = "Loading <b>" + path + "</b>...";
> 
> Mark, do we have a helper function somewhere for escaping/sanitizing inputs?

No. We should pull out the escaping code from eclipse.Searcher.formatHighlight() and put it in a helper.

Comment 4 John Arthorne 2011-01-14 15:37:03 EST

Please take this Mark.

Comment 5 Mark Macdonald 2011-01-14 18:00:52 EST

It looks like there are several other places where we output unsanitized user input into an innerHTML:

- Favorite names
- Search results view (if no matches are found)
- Filename area in coding.html (?)
- Global search (Ctrl+H) results
- Unit test viewer

It's probably possible to inject scripts, break the page layout, etc, in all of these cases too...

Comment 6 Sundar 2011-01-15 18:13:21 EST

As a practice i recommend to sanitize all the user inputs which come as part of GET and POST request as well. It is a good idea to use Servlet filters for sanitizing all the inputs.

Additional reference:
http://greatwebguy.com/programming/java/simple-cross-site-scripting-xss-servlet-filter/

Comment 7 Mark Macdonald 2011-01-21 16:29:20 EST

I don't like the idea of having the server transform all the data it gets, but still, that's only part of the problem. Some exploits can be done using data the server never sees -- for example, the fragment identifier in a URL.

That's why we need to improve our client-side coding practices. We should only display user input inside a DOM TextNode, so the browser parses it as simple text and not HTML. We can't safely use innerHTML or shortcuts to it (eg. passing HTML strings to dojo.place()) when user input is involved, since it's too easy for that data to escape and become code.

I'll use this bug to fix any places in our client code that are using innerHTML in a bad way.