Comment 24 for bug 1308727

Revision history for this message
Gabriel Hurley (gabriel-hurley) wrote : Re: XSS in Horizon Heat template - resource name

The javascript change for horizon.instances.js is a very roundabout way that's probably still exploitable although I don't have an immediate example.

the .text() method will take the text representation of the content of an element, hence resulting in certain HTML entities being unescaped...

by contrast the .html() method takes the HTML content of the element, reading HTML entities as their original entity form.

the problem here lies in what happens when you re-insert the value you pulled out into a new element on this line: https://github.com/openstack/horizon/blob/master/horizon/static/horizon/js/horizon.instances.js#L73

because that uses the .html() method, any malicious code gets re-inserted as HTML.

In the short term, I think a better solution is to continue to use .text() for the read, and to change the code where the new element is constructed to either construct the safe (non-user-input) HTML parts and then fill in the user bits with the .text() method (meaning that the read and the write both use .text() thus ensuring text stays text; or otherwise to sanitize the read values before inserting them back into the DOM. There are plenty of examples of how to do XSS santization in javascript on Stack Overflow, or pretty much any javascript templating library has it built in.

The patch as it stands should probably be reworked to address this; I don't think it should be too hard to fix.

I would recommend reviewing the rest of the javascript codebase for places where elements are being constructed by hand using the .html() method. Those are always red flags where you need to be sure that any variables are safe.