1492041 - Insufficient documentation for HTML fragment sanitation in chrome-privileged documents

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: General, defect, P3)

Description

[tobias.bengfort]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180830143136

Steps to reproduce:

In my thunderbird extension, I use a client-side template. The result is injected into the page by assigning to element.innerHTML. This stopped working after updating to thunderbird 60.


Actual results:

It took me some time to find out that this was because assigning to element.innerHTML is now sanitized by default (1432966). After understanding that I also did not instantly know how to fix the issue.

I also found three different ways to work around the limitation:

- set document.allowUnsafeHTML = true
- use element.setUnsafeInnerHTML instead
- use DOMParser.parseFromString instead


Expected results:

- This is a breaking change and should have been listed in the release notes <https://www.thunderbird.net/en-US/thunderbird/60.0/releasenotes/>
- I understand that this change was made to prevent insecure usage of innerHTML. But client-side templating seems to be a perfectly valid (and secure) usecase. There should be documentation on how that can be done. I also expect that many libraries rely on innerHTML.
- Attackers can easily bypass this restriction by using any (and potentially more) of the methods described above. The first two are explicitly marked as "unsafe", but DOMParser.parseFromString is not. Maybe the sanitation should also applied there?

Comment 1

[Andrew Overholt [:overholt]]

ni?(kmag) as he worked on bug 1432966

Comment 2

[Kris Maglione [:kmag]]

(In reply to tobias.bengfort from comment #0)
> - set document.allowUnsafeHTML = true
> - use element.setUnsafeInnerHTML instead

These were both temporary stopgaps so that we could land this quickly. Their
WebIDL documentation specifically says not to use them.

They have both been removed.

> Expected results:
> 
> - This is a breaking change and should have been listed in the release notes
> <https://www.thunderbird.net/en-US/thunderbird/60.0/releasenotes/>

This is an issue for the Thunderbird team. This change isn't visible to any
external consumers of Firefox.

> - I understand that this change was made to prevent insecure usage of
> innerHTML. But client-side templating seems to be a perfectly valid (and
> secure) usecase. There should be documentation on how that can be done. I
> also expect that many libraries rely on innerHTML.

We've decided that the convenience isn't worth the risk. We don't use any
HTML-based templating in chrome contexts in mozilla-central.

> - Attackers can easily bypass this restriction by using any (and potentially
> more) of the methods described above. The first two are explicitly marked as
> "unsafe", but DOMParser.parseFromString is not. Maybe the sanitation should
> also applied there?

No they cannot. An attacker could only access those methods if the browser were
already compromised, at which point it doesn't make a difference. Sanitizing by
default removes an attack vector.

Comment 3

[tobias.bengfort]

Thanks for the feedback.

> This is an issue for the Thunderbird team. This change isn't visible to any
> external consumers of Firefox.
>
> We've decided that the convenience isn't worth the risk. We don't use any
> HTML-based templating in chrome contexts in mozilla-central.

The change *is* visible to me as a thunderbird addon developer. I *do* use HTML-based templating. Maybe I was not clear in my initial report: I am wondering what is recommended for addon developers like myself.

> > - set document.allowUnsafeHTML = true
> > - use element.setUnsafeInnerHTML instead
>
> These were both temporary stopgaps so that we could land this quickly. Their
> WebIDL documentation specifically says not to use them.

So what about the third option? Is that an intentional workaround or an actual issue?

Comment 4

[Kris Maglione [:kmag]]

(In reply to tobias.bengfort from comment #3)
> Thanks for the feedback.
> 
> > This is an issue for the Thunderbird team. This change isn't visible to any
> > external consumers of Firefox.
> >
> > We've decided that the convenience isn't worth the risk. We don't use any
> > HTML-based templating in chrome contexts in mozilla-central.
> 
> The change *is* visible to me as a thunderbird addon developer. I *do* use
> HTML-based templating. Maybe I was not clear in my initial report: I am
> wondering what is recommended for addon developers like myself.

It is, but as I said, that is an issue for the Thunderbird team. Since legacy extension support was discontinued, we no longer directly support any external consumers of chrome APIs. If Thunderbird wishes to continue doing so, the onus is entirely on them.

> > > - set document.allowUnsafeHTML = true
> > > - use element.setUnsafeInnerHTML instead
> >
> > These were both temporary stopgaps so that we could land this quickly. Their
> > WebIDL documentation specifically says not to use them.
> 
> So what about the third option? Is that an intentional workaround or an
> actual issue?

Not as such. DOMParser creates a new document with a null principal, and is therefore not subject to these types of restrictions.

Comment 5

[tobias.bengfort]

> that is an issue for the Thunderbird team

I totally agree. So should this bug report be reassigned to a different category? (I do not have much experience with this bugtracker)

Comment 6

[Jorg K (CEST = GMT+2)]

Documented here
https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_57
and here
https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_63

Note: TB is mostly community driven and there are many Mozilla platform interface changes to follow, so these pages don't claim to be complete. If you come across a problem, use one of the channels listed on the "57 page" to get in touch.