Risks of unsanatized HTML input

So, I had this error handling/reporting code in a WebExtension:

{ ... } catch (error) {
    document.body.innerHTML = (`<h1>500</h1>${ error && error.message || '' }`);
}

prefixed with the comment

// inline scripts are not allowed (CSP, firefox), so this is not a security problem

The reviewer did not agree with that. My question is why. (I know how to avoid it, that’s not the point.)

I ask this because the Content Security Policy of WebExtensions in Firefox does disallow script-src 'unsafe-inline'. The AMO reviews are only concerned with Firefox (and Fennec). So any kind of code injection (XSS) should be out of the picture, right?

As far as I see it, the worst thing that could happen is that an attacker could (visually) mess with my error page and include passive content fetches from their servers. Is that all (I think the developers should be allowed to decide if they permit that), or am I missing something?

Maybe the error message can be tampered with containing a malicious link?

Where is the error coming from?

Inserting un-sanitized remote content is a security problem.

However … above code is easily fixed so it will not be a problem, even if it is remote.
It is longer, but you need to write it once.
It is a LOT faster too.

{ ... } catch (error) {

  let h1 = document.createElement('h1');  // create H1
  h1.textContent = '500';                 // Add its text
  document.body.textContent = '';         // clear the document.body completely (no DOM or Text)
  document.body.appendChild(h1);          // append H1
  let text = error && error.message ? error.message || '';
  if (text) {
    document.body.appendChild(document.createTextNode(text)): // add the text safely (as text, not DOM)
  }
}

If you mean that an adversary could inject something like <a href="https://evil.com/malware.exe">Click here for cat cute videos</a>, then sure. But if I sanitize the HTML, that link would probably survive. Besides, it is content, I should be allowed to decide which content to display in my extension.


I should probably have said that. It could be any kind of runtime error, including one I explicitly throw that contains the literal location of the document. So yes, it would be quite easy to inject arbitrary HTML code in there.

Inserting un-sanitized remote content is a security problem.

That is exactly what is my question is about: Why? If script tags are injected, those won’t be executed (CSP), so where is the risk?

However … above code is easily fixed so it will not be a problem, even if it is remote.

As I said, I know. My approach is very different to yours, though.

It is a LOT faster too.

I actually very much doubt that. It is a last resort error handling, It shouldn’t be run to often, and if it is, there is nothing to run afterwards, so it doesn’t even matter how long it hakes to run it. But it does matter how long the parsing of the code takes, because that is done even if that section is not executed.
I see a good chance that in that case, my code below is faster (less code and tokens to parse).
Besides, I want to allow interpreted HTML in there:

{ ... } catch (error) { 
	document.body.innerHTM = (`<iframe sandbox
	style="position: fixed;left:0;top:0;width:100%;height:100%;border:none"
	src="data:text/html;charset=utf-8;base64,`+ btoa(`
		<h1>500</h1>
		${ error && error.message || '' }
	`) +`"></iframe>`);
}

Granted, this is very inefficient, but it really doesn’t matter whether displaying the error takes 10 or 1000 milliseconds.

Thanks for both your answers, though.

Why not use innerText if you don’t expect render the code in error.message?

Assume the error message contains a link syntax, then your code render it.

I do expect HTML code. And again: I am aware of several ways to mitigate code injection here, that is not the point of my question. I want to know why I should even bother. IMHO, the CSP already takes care of the XSS issue.

I think you will get unexpected content and mislead / deceive risks if you do not master the content. You are almost impossible to eliminate all risks if you do not understand the possible content.

Here are some test data that I did personally.

elem.appendChild(document.createTextNode('test')); //x6 faster than innerHTML
elem.textContent = 'test';  //x5 faster than innerHTML
elem.innerHTML = 'test';

Not exactly … some inline execution like on* will happen e.g.:

document.body.innerHTML = '<img src="nothing" onerror="alert(\'evil\')" onclick="alert(\'evil\')">';

There are other situations like when injecting into a content page and the code will follow content. CSP (not addons) etc…

Finally, developers are free to chose the method they prefer. However, that may not pass the review. Passing un-sanitized remote content to innerHTML will not pass the review.

1 Like

Sure. but is misleading content part of the reviews? I don’t think that is listed here:
https://developer.mozilla.org/en-US/Add-ons/AMO/Policy/Reviews


That would be pretty bad and fortunately isn’t the case:
image

when injecting into a content page and the code will follow content. CSP

Yes. The script in question won’t ever be loaded as a content script, but I admit that it could generally happen (not in my particular script, but that is more of an coincident). This should take care of that:


if (!(/^\w+-extension:\/\//).test(location.href)) { return Promise.reject(
    new Error(`This script can only be loaded in extension pages`)
); }

Passing un-sanitized remote content to innerHTML will not pass the review.

I could only understand such a general and strict rule if doing so actually fell in the category of “Create or expose security vulnerabilities” (or any other) on the page linked above. Which brings us back to my original question: Does it? And if yes, which?

Please don’t think I ask this for the sake of arguing with you. I think the add-on reviews on AMO are a great thing. I merely want to understand the criteria of it and in this case the possible security issues, if there are any.


I actually very much doubt that.

Here are some test data that I did personally.

I’m sorry, I didn’t want to say that your code won’t execute faster than mine. It most certainly will. My point there was that execution speed isn’t the important thing there. Not because performance generally doesn’t matter, but because your optimization is not for the common case and not on the critical path. And I am pretty sure it dos increase the compile time of the script (very slightly) and in JavaScript, compile time is run time.

document.title = `Try <a href="https://evil.com/malware.exe">Click here for cat cute videos</a>`
document.body.innerHTML = `"${document.title}" cannot be closed`

It is an attack vector, so reviewer will reject it.

In addition, your users should not be disturbed from untreated error messages.

It is an attack vector

That is where I currently disagree (because a strong CSP is in place). If you have arguments supporting that claim, I’ll happily be convinced. But please don’t just repeat that it is.

In addition, your users should not be disturbed from untreated error messages.

True. But not point of this discussion. (And I think a cryptic error message is better than none.)

CSP does not avoid insert misleading information from the message, why do you ignore it?

My general problem here is this:
My understanding of the review policy is that embedding remote content is OK, nut running remote code isn’t.
If that is true, then why can’t I just use innerHTML = remoteString as far as the reviews are concerned?
One general suggestion is to “sanitize” the HTML, without sating exactly what that means. My perception was that the sanitation should remove all scripts. But what is the point if scripts won’t be executed anyway?

I do agree that embedding arbitrary remote content can potentially be confusing, but depending on how I do the sanitation, it won’t help with this. And I could also have that without any innerHTML like API:

<!DOCTYPE html>
<html><head></head><body>
<iframe style="position:fixed;left:0;top:0;width:100%;height:100%;border:none"></iframe>
<script>
     document.querySelector('iframe').src = location.hash.slice(1);
</script>
</body></html>

That thing could display pretty much anything, but wont give the content access to anything privileged. Would this conflict with the review policy?

I thought about this some more. You are right. Preventing JavaScript code injection isn’t everything. Allowing the injection of arbitrary content including CSS can allow an attacker to essentially cover your entire page, which means an attackers page is displayed under your name (and the trust the user may have in that name).
Disallowing inline styles doesn’t entirely mitigate this either. An attacker might still be able to use classes and ids combine the white listed CSS in undesirable ways.
So you would need to remove classes and ids as well. And that requires sanitation.

Thank you both for this discussion!

1 Like