0

How do you safely push a user influenced url to the DOM, namely to the src attribute of an img?

<img> won't execute anything else than a valid img and scripting in svg is limited. Casual onload tricks won't work aswell because the input is quoted. Is this enough to ensure a safe DOM insertion via img?

The snippet is simplified, there are checks for url being a valid url.

Edit: something like http://foo" onerror="alert(&apos;XSS&apos;)" " is not a valid url, see here: http://foo" onerror="alert('XSS')" ". You can assume that url is for sure a valid url. See https://en.wikipedia.org/wiki/Percent-encoding#Types_of_URI_characters, Check if a Javascript string is a url and https://jsfiddle.net/mppkgd7u/

var user = 'random/url/path'; // any kind of user generated string
var url = 'http://' + user; // guaranteed to be url
document.getElementById('view').innerHtml = '<img src="'+url+'">';
console.log(document.getElementById('view').innerHtml);
<div id=view>
</div>
rndus2r
  • 496
  • 4
  • 17
  • There is no reason to use `innerHTML` over DOM manipulation here. Enough to worry about allowing arbitrary URLs without introducing potential HTML injection. – Ry- Aug 20 '17 at 18:27
  • Yes, that's what Anthony is suggesting. Can you provide an example for potential HTML injection in given context? – rndus2r Aug 20 '17 at 21:03
  • 1
    I’m saying there is no need to rely on a quirk of your input data when you can just do the right thing at no cost. – Ry- Aug 21 '17 at 02:19
  • I appreciate your (and Anthony's) alternative, but I don't see any difference between your suggestion and my approach. See this fiddle: https://jsfiddle.net/9ek1tar8/ That's why I asked for an example. Or at least the difference between both approaches. – rndus2r Aug 21 '17 at 05:21
  • 2
    https://jsfiddle.net/9ek1tar8/1/, but even if there weren’t a difference, I don’t see why you would insist on doing it the wrong way just because it happens to work for a particular type of value. – Ry- Aug 21 '17 at 05:35
  • Thanks. The snippet is simplified - the whole script is a bit more complex. I need to replace all placeholders (user urls) with respective images without losing the surrounding text - I'm not sure how to push a HTMLObject into a text context. That's why I'm sticking to my approach. I'm new to JS, too, so I want to learn why something is done a certain way, not just how it is done. The answer below did rely on a malformed url which is not possible ; your comment shows some html being injected, but it can't be more harmful than any other link, no? – rndus2r Aug 21 '17 at 06:48
  • 1
    It may or may not be harmful. It will certainly produce an *incorrect* result. Anyway, you can just HTML-encode in that case like every decent template engine does and again not have to worry about this. – Ry- Aug 21 '17 at 06:49

1 Answers1

5

What if the user's string is 'no" onerror="alert(0)'?

This'll expand to:

<img src="http://no" onerror="alert(0)">

The safest way is to use document.createElement and assign the src attribute. Using innerHTML isn't a very safe approach

var img = document.createElement('img');
img.src = ...;
anthony sottile
  • 61,815
  • 15
  • 148
  • 207
  • It is a good answer and you caught me off guard there for a moment, so I thought this would be the answer. But `http://foo" onerror="alert('XSS')" "` is not a valid url, thus the payload would not be accepted as stated in question. `url` is guaranteed to be a valid url. I will emphasize that again. – rndus2r Aug 20 '17 at 03:10
  • How do you define a "valid url"? – anthony sottile Aug 20 '17 at 03:13
  • 1
    HTTP valid syntax according to this rfc https://tools.ietf.org/html/rfc3986#section-1.1.2 for example – rndus2r Aug 20 '17 at 03:14
  • Could you also show us a regex, for example, of the url-validation? – clabe45 Aug 20 '17 at 03:20
  • AnthonySottile No, because " is not allowed and ' is reserved. See https://en.wikipedia.org/wiki/Percent-encoding#Types_of_URI_characters @clabe45 https://stackoverflow.com/questions/5717093/check-if-a-javascript-string-is-a-url => https://jsfiddle.net/mppkgd7u/ – rndus2r Aug 20 '17 at 03:34