1214652 - Unsafe innerHTML/outerHTML/insertAdjacentHTML usage in gaia::music

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Music, defect)

Description

[Frederik Braun [:freddy]]

Please see the hints in bug 1211384 about fixing these kinds of problems.
The Firefox OS Security team is there to help you with any kind of question that you may have. You can reach out by setting the needinfo or sec-review flag to [e-mail]

Unsafe assignment to innerHTML:
In apps/music/components/gaia-fast-list/gaia-fast-list.js, line 1196, column 5:
> this.els.cached.innerHTML = html;
In apps/music/components/gaia-header/dist/gaia-header-es5.js, line 205, column 15:
> props.template.innerHTML = output.template;
In apps/music/components/gaia-header/dist/gaia-header-es5.js, line 472, column 13:
> style.innerHTML = css.trim();
In apps/music/components/gaia-header/dist/gaia-header-es5.js, line 515, column 13:
> el.lightStyle.innerHTML = el.lightCss;
In apps/music/components/gaia-header/dist/gaia-header.js, line 168, column 5:
> props.template.innerHTML = output.template;
In apps/music/components/gaia-header/dist/gaia-header.js, line 424, column 3:
> style.innerHTML = css.trim();
In apps/music/components/gaia-header/dist/gaia-header.js, line 465, column 3:
> el.lightStyle.innerHTML = el.lightCss;
In apps/music/components/gaia-theme/lib/gaia-theme-selector.js, line 12, column 3:
> this.createShadowRoot().innerHTML = template;
In apps/music/components/gaia-toolbar/gaia-toolbar.js, line 26, column 3:
> this.createShadowRoot().innerHTML = template;
In apps/music/components/poplar/poplar.js, line 260, column 3:
> div.innerHTML = html;
In apps/music/elements/music-artwork.js, line 167, column 3:
> shadowRoot.innerHTML = template;
In apps/music/elements/music-controls.js, line 75, column 3:
> shadowRoot.innerHTML = template;
In apps/music/elements/music-overlay.js, line 138, column 3:
> shadowRoot.innerHTML = template;
In apps/music/elements/music-rating.js, line 45, column 3:
> shadowRoot.innerHTML = template;
In apps/music/elements/music-scan-progress.js, line 87, column 3:
> shadowRoot.innerHTML = template;
In apps/music/elements/music-scan-progress.js, line 157, column 3:
> style.innerHTML = globalStyles;
In apps/music/elements/music-search.js, line 140, column 3:
> shadowRoot.innerHTML = template;
In apps/music/elements/music-seek-bar.js, line 108, column 3:
> shadowRoot.innerHTML = template;
In apps/music/elements/music-tab-bar.js, line 34, column 3:
> this.querySelector('style').innerHTML += styles;
In apps/music/elements/music-view-stack.js, line 84, column 3:
> style.innerHTML = template;

Comment 1

[Hubert Figuiere [:hub]]

Lot of these are from gaia components though.

Comment 2

[Frederik Braun [:freddy]]

You can fix the bugs in here individually, pushing the issues in the gaia components to a later point.
If you want, I can reach out to the developers of the gaia components and discuss an upstream fix.

Comment 3

[Hubert Figuiere [:hub]]

I was just mentionning it as getting the fixes from upstream might be best as they are shared. I'll fix at least the local ones.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] hfiguiere:bug1214652-innerhtml > mozilla-b2g:master

Comment 5

[Hubert Figuiere [:hub]]

Comment on attachment 8673713 [details] [review]
[gaia] hfiguiere:bug1214652-innerhtml > mozilla-b2g:master

This is all the music specific changes.

Comment 6

[Justin D'Arcangelo [:justindarc]]

(In reply to Frederik Braun [:freddyb] from comment #2)
> You can fix the bugs in here individually, pushing the issues in the gaia
> components to a later point.
> If you want, I can reach out to the developers of the gaia components and
> discuss an upstream fix.

This is a bad idea since all changes will be wiped out anytime `bower update` is run. Separate bugs for each of the gaia-components should be filed instead.

Comment 7

[Hubert Figuiere [:hub]]

Which is why I haven't touched them.

Comment 8

[Justin D'Arcangelo [:justindarc]]

freddyb: Most of these innerHTML assignments are using template strings that are NOT using any string interpolation. In pretty much all of those instances, we are simply relying on template strings for a clean, easy way to do multi-line strings for injecting the shadow DOM markup for web components. If we have to call `unwrapSafeHTML()` at the time of the innerHTML assignment, that will mean that the unwrap operation is happening inside the web component's `createdCallback()` which could possibly cause a brief FOUC or a delay in DOM loading. There is one case in the Music NGA app already where we *are* doing string interpolation and we already are using Sanitizer properly. However, in most (if not all) of the whitelisted cases mentioned in this bug, we are not doing string interpolation.

So, two questions:
1.) Is it acceptable to just whitelist these cases where we are not doing string interpolation?
2.) If not, can we do the `unwrapSafeHTML()` operation *outside* of the innerHTML assignment so that it doesn't occur inside the `createdCallback()`? For example:

var proto = Object.create(HTMLElement.prototype);

var template = Sanitizer.unwrapSafeHTML(Sanitizer.createSafeHTML `<div id="container">
  <h1 id="heading"></h1>
  <h2 id="subheading"></h2>
</div>`);

proto.createdCallback = function() {
  var shadowRoot = this.createShadowRoot();
  shadowRoot.innerHTML = template;
};

Comment 9

[Frederik Braun [:freddy]]

NB, if you *did* string interpolation, you could use Sanitizer.escapeHTML`` instead of unwrap(createSafeHTML``).

The linter complains here because it does not understand variable types, or scope, or anything for that matter. It's hard to tell that some variable contains something secure in simplistic static analysis like this. Regardless of whether it is just a string literal or the return value of a well-known escaping function.

What you can do is:

shadowRoot.innerHTML = `<div id="container">
  <h1 id="heading"></h1>
  <h2 id="subheading"></h2>
</div>`

Would this work for you?

Comment 10

[Justin D'Arcangelo [:justindarc]]

(In reply to Frederik Braun [:freddyb] from comment #9)
> NB, if you *did* string interpolation, you could use Sanitizer.escapeHTML``
> instead of unwrap(createSafeHTML``).
> 
> The linter complains here because it does not understand variable types, or
> scope, or anything for that matter. It's hard to tell that some variable
> contains something secure in simplistic static analysis like this.
> Regardless of whether it is just a string literal or the return value of a
> well-known escaping function.
> 
> What you can do is:
> 
> shadowRoot.innerHTML = `<div id="container">
>   <h1 id="heading"></h1>
>   <h2 id="subheading"></h2>
> </div>`
> 
> Would this work for you?

I understand what you're saying about the linter. Yeah, I'd rather avoid any calls at all to the Sanitizer in these cases if I can for performance reasons. Let me take a look and see if we can just inline the `innerHTML` assignments in our custom elements. Thanks!

Comment 11

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8673713 [details] [review]
[gaia] hfiguiere:bug1214652-innerhtml > mozilla-b2g:master

LGTM. Check the comments on the PR before landing.

Comment 12

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8673713 [details] [review]
[gaia] hfiguiere:bug1214652-innerhtml > mozilla-b2g:master

Ahhhh.. Commented on the wrong bug :-X

Comment 13

[Hubert Figuiere [:hub]]

Ok, I'll update by inlining the code...

Comment 14

[Hubert Figuiere [:hub]]

Comment on attachment 8673713 [details] [review]
[gaia] hfiguiere:bug1214652-innerhtml > mozilla-b2g:master

PR has been updated. r?

Comment 15

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8673713 [details] [review]
[gaia] hfiguiere:bug1214652-innerhtml > mozilla-b2g:master

LGTM. Thanks Hub!

Comment 16

[Hubert Figuiere [:hub]]

Merged 
https://github.com/mozilla-b2g/gaia/commit/b4201a0ced4559443fcebaf14af5fb76385bbefe

Leaving open as we still have components that needs upstream fixes.

Comment 17

[Hubert Figuiere [:hub]]

Comment on attachment 8673713 [details] [review]
[gaia] hfiguiere:bug1214652-innerhtml > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: possible XSS
[Testing completed]: integration test and regular test
[Risk to taking this patch] (and alternatives if risky): none.
[String changes made]: none.

Comment 18

[Mahendra Potharaju [:mahe]]

Comment on attachment 8673713 [details] [review]
[gaia] hfiguiere:bug1214652-innerhtml > mozilla-b2g:master

Approved for 2.5 uplift. 

Thanks

Comment 19

[Carsten Book [:Tomcat]]

in 2.5 -> https://hg.mozilla.org/integration/gaia-2_5/rev/22b52ef4f9e9

Comment 20

[Frederik Braun [:freddy]]

I will stop tracking the bugs and this bug is inactive. Closing WONTFIX.