19

I've been doing some experimenting with innerHTML to try and figure out where I need to tighten up security on a webapp I'm working on, and I ran into an interesting injection method on the mozilla docs that I hadn't thought about.

var name = "<img src=x onerror=alert(1)>";
element.innerHTML = name; // Instantly runs code.

It made me wonder a.) if I should be using innerHTML at all, and b.) if it's not a concern, why I've been avoiding other code insertion methods, particularly eval.

Let's assume I'm running javascript clientside on the browser, and I'm taking necessary precautions to avoid exposing any sensitive information in easily accessible functions, and I've gotten to some arbitrarily designated point where I've decided innerHTML is not a security risk, and I've optimized my code to the point where I'm not necessarily worried about a very minor performance hit...

Am I creating any additional problems by using eval? Are there other security concerns other than pure code injection?

Or alternatively, is innerHTML something that I should show the same amount of care with? Is it similarly dangerous?

danShumway
  • 450
  • 4
  • 15
  • The risk is always in letting content produced by one user run in the context of another user's session. If I write a script, it's no big deal if I run it in my own browser, but it *is* a big deal if that script gets stored and run in someone else's browser. – apsillers May 31 '13 at 15:05
  • Are you asking if you should use `innerHTML` *or* `eval`, because they are not analogous – Explosion Pills May 31 '13 at 15:06
  • @ExplosionPills - More of a question of "should I use either at all?" I'm wondering what the differences are. Would they be purely analogous security risks, or would one be more of a risk than the other and why? – danShumway May 31 '13 at 15:14
  • @user2267361: `eval()` is baseline more dangerous than `innerHTML`. However, if you allow the insertion of a `script` tag or an event handler in the `innerHTML`, you've basically given them access to `eval`. – Sébastien Renauld May 31 '13 at 15:15
  • @apsillers: indeed. I never said otherwise. Script tag **or event handler**. – Sébastien Renauld May 31 '13 at 15:17

3 Answers3

21

tl;dr;

Yes, you are correct in your assumption.

Setting innerHTML is susceptible to XSS attacks if you're adding untrusted code.

(If you're adding your code though, that's less of a problem)

Consider using textContent if you want to add text that users added, it'll escape it.

What the problem is

innerHTML sets the HTML content of a DOM node. When you set the content of a DOM node to an arbitrary string, you're vulnerable to XSS if you accept user input.

For example, if you set the innerHTML of a node based on the input of a user from a GET parameter. "User A" can send "User B" a version of your page with the HTML saying "steal the user's data and send it to me via AJAX".

See this question here for more information.

What can I do to mitigate it?

What you might want to consider if you're setting the HTML of nodes is:

  • Using a templating engine like Mustache which has escaping capabilities. (It'll escape HTML by default)
  • Using textContent to set the text of nodes manually
  • Not accepting arbitrary input from users into text fields, sanitizing the data yourself.

See this question on more general approaches to prevent XSS.

Code injection is a problem. You don't want to be on the receiving end.

The Elephant in the room

That's not the only problem with innerHTML and eval. When you're changing the innerHTML of a DOM node, you're destroying its content nodes and creating new ones instead. When you're calling eval you're invoking the compiler.

While the main issue here is clearly un-trusted code and you said performance is less of an issue, I still feel that I must mention that the two are extremely slow to their alternatives.

Community
  • 1
  • 1
Benjamin Gruenbaum
  • 270,886
  • 87
  • 504
  • 504
  • This is perfect, I hadn't looked into data sanitation at all before this. – danShumway May 31 '13 at 15:19
  • @w5m wow, I guess I'm really tired -_-' , my spelling is usually a lot better. Thanks a lot for the edit. OP, I'm glad you found this answer useful, feel free to drop by in the JS chat here in SO if you have any more questions on XSS. – Benjamin Gruenbaum May 31 '13 at 17:01
6

The quick answer is: you did not think of anything new. If anything, do you want an even better one?

<scr\0ipt>alert("XSSed");</scr\0ipt>

The ground, bottom line is that there are more ways to trigger XSS than you think there is. All the following are valid:

  • onerror, onload, onclick, onhover, onblur etc... are all valid
  • The use of character encoding to bypass filters (null byte highlighted above)

eval falls into another category, however - it is a byproduct, most of the time to obfuscate. If you're falling to eval and not innerHTML, you're in a very, very small minority.

The key to all this is to sanitize your data using a parser that keeps up to date with what pen testers discover. There are a couple of those around. They absolutely need to at least filter all the ones on the OWASP list - those are pretty much common.

Sébastien Renauld
  • 19,203
  • 2
  • 46
  • 66
  • 2
    +1 for the null char ... oh my god !!!!! i have some filters to tweak in my code !!!! – rupps May 31 '13 at 15:11
  • @rupps: Which language are you coding in? If you're filtering in PHP, http://htmlpurifier.org/ might be worth looking at as a first step before custom filters. Handles most of the cases on the OWASP list. – Sébastien Renauld May 31 '13 at 15:14
  • 1
    I have extensive client-side libraries developed by myself, but thanks for the tip ... as XSS gets more popular definitely it makes sense to rely on a community-maintained library to get rid of the brilliant new nasties discovered every other day .... – rupps May 31 '13 at 15:18
  • Very nice trick with the null char from OWASP!, I think what one should take from this is the fact one should white-list and not black list. – Benjamin Gruenbaum May 31 '13 at 17:01
2

innerHTML isn't insecure in and of itself. (Nor is eval, if only used on your code. It's actually more of a bad idea for several other reasons.) The insecurity arises in displaying visitor-submitted content. And that risk applies to any mechanism with which you embed user-content: eval, innerHTML, etc. on the client-side, and print, echo, etc. on the server-side.

Anything you put on the page from a visitor must be sanitized. It doesn't matter a great deal whether you do it when the initial page is being built or added asynchronously on the client-side.

So ... yes, you need to show some care when using innerHTML if you're displaying user-submitted content with it.

svidgen
  • 13,744
  • 4
  • 33
  • 58