Code Excerpt: Does this create a vulnerability to XSS?

Question

So I'm relatively new to JavaScript, although I've been doing HTML/CSS UI frontend work forever (blasphemy, I know) and am in the process of developing my own boilerplates for use with future projects. I'm also fairly new to the idea of securing code from XSS, CodeInjection etc.

The following is a fully working excerpt that I've been working on. In production, the JavaScript will be minified and loaded from an external file, not loaded inline, it's just for the example for ease of use.

I'm wondering if by creating variable output as plain text and inserting it via document.getElementById("x").href, leaves an opening to vulnerabilities such as CSS attacks. There is a ton of information regarding these types of vulnerabilities, but as a relative newcomer to the secure code environment, it's daunting and a bit overwhelming.

PS, the <p></p> is also just for testing.

Thanks to all in advance!

For the code:

<!DOCTYPE html>

<html>
    <head>
        <meta charset="utf-8" />
        <title>Title</title>

        <!-- required CSS -->

        <!-- conditional CSS -->
        <link rel="stylesheet" type="text/css" href="assets/css/mq/null" id="mediaquery" />
    </head>

    <body>

        <p id="test"></p>

        <script type="text/javascript">
        (function setCSS() {
            if (window.matchMedia("screen and (max-width: 767px)").matches) {
                layout = "mobile";
            } else if (window.matchMedia("screen and (min-width: 768px) and (max-width: 1024px)").matches) {
                layout = "tablet";
            } else if (window.matchMedia("screen and (min-width: 1025px) and (max-width: 1280px)").matches) {
                layout = "desktop";
            } else if (window.matchMedia("screen and (min-width: 1281px)").matches) {
                layout = "xldesktop";
            }
                window.addEventListener('resize', setCSS, false);
                document.getElementById("test").innerHTML = layout;
                document.getElementById("mediaquery").href = 'assets/css/mq/device.'+layout+'.css';
        })();
        </script>

    </body>
</html>

NB: you'll be creating an ever increasing set of event listeners bound on `resize` with this code. — Alnitak, Sep 15 '14 at 17:22
I was worried about that, however in testing and using the developer tools within Chrome, I'm not seeing an increase in HTTP requests whenever the window size is changed. Is there a more thorough way to be sure? — Gabrial MacLeod, Sep 15 '14 at 17:25
What kind of XSS vulnerability are you thinking about? The question is always: **Where** does the "*variable output*" come from? — Bergi, Sep 15 '14 at 17:26
@Bergi the whole reason for doing this is to restrict overhead, designing for mobile first, it makes no sense for mobile users to download assets meant for large desktop environments, etc. I've read articles regarding the negligent hit on performance by including all assets in a single CSS file, or simply loading all external CSS files, however I feel this is cleaner. Assets are loaded on demand. — Gabrial MacLeod, Sep 15 '14 at 17:27
@Bergi I had come across this: https://www.owasp.org/index.php/Testing_for_CSS_Injection_(OTG-CLIENT-005) — Gabrial MacLeod, Sep 15 '14 at 17:29
@GabrialMacLeod if your reply is to me, chances are you'll only see an HTTP request if the media changes. However the code as written will _definitely_ cause an ever increasing set of calls to `setCSS` for each resize event that will progressively slow down the browser. — Alnitak, Sep 15 '14 at 17:32
@GabrialMacLeod: So what? If the attacker can't control the `layout` variable (and how would he?), he can't load arbitrary css. — Bergi, Sep 15 '14 at 17:37
@Alnitak I guess you're correct, in that a user could potentially sit there and continually resize their window back and forth incessantly, or do the same thing regarding portrait/landscape orientation of their device which is kind of silly. However is there a better method to use than the "resize" trigger? I'm still writing down notes for things to research and follow up on — Gabrial MacLeod, Sep 15 '14 at 17:46
@Alnitak he could probably use namespaces together with jQuery and unbind the resizer and poke a new one: `jQuery.bind('resize.glob', ...` and `jQuery.unbind('.glob');` — Axel Amthor, Sep 15 '14 at 17:50
@AxelAmthor no, it would be simplest just to create an outer IIFE (to avoid global scope polution) and then make sure that the call to `addEventListener` is _outside_ of `setCSS`. — Alnitak, Sep 15 '14 at 17:51
@Alnitak Right, if it is that simple. But where is "outside" ... — Axel Amthor, Sep 15 '14 at 17:53

score 0 · Accepted Answer · edited May 23 '17 at 12:21

The only part of your posted code that could lead to an XSS vulnerability is

document.getElementById("test").innerHTML = layout;

If you are setting innerHTML and you have any HTML in your layout variable that could be user controlled then this could be an attack vector. However, in this case layout appears to be one of either mobile, tablet, desktop or xldesktop.

If you want to include any user set content or content from an untrusted source, you should HTML encode your output. In this case I would not go with the accepted answer as it does not appear to include the single or double quote characters which as essential for XSS prevention, but the answer by Anentropic will make your output secure for output in an HTML context.

function htmlEscape(str) {
    return String(str)
            .replace(/&/g, '&amp;')
            .replace(/"/g, '&quot;')
            .replace(/'/g, '&#39;')
            .replace(/</g, '&lt;')
            .replace(/>/g, '&gt;');
}

The quote characters are required to be encoded because if you output to a quoted attribute value then a malicious user could escape this context otherwise. Please note if you output to unquoted attribute values then you must escape all characters with ASCII values less than 256 except for alphanumeric characters.

Another route is to use JQuery where you can set the text property rather than html.

Code Excerpt: Does this create a vulnerability to XSS?

1 Answers1