Album metadata is inserted unescaped into innerHTML without rationalizing comment in views/home/view.js

RESOLVED FIXED in FxOS-S8 (02Oct)

Status

Firefox OS
Gaia::Music
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: asuth, Assigned: hub)

Tracking

unspecified
FxOS-S8 (02Oct)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
I was just checking out the revamped music app when I noticed this:

https://github.com/mozilla-b2g/gaia/blob/master/apps/music/views/home/view.js#L64
    this.albums.forEach((album) => {
      var template =
`<a class="tile"
    href="/player?id=${album.name}"
    data-artist="${album.metadata.artist || unknownArtist}"
    data-album="${album.metadata.album || unknownAlbum}"
    data-file-path="${album.name}">
  <img>
</a>`;

      html += template;
    });

    this.tiles.innerHTML = html;


The album name/artist/name can all potentially escape out of the double quotes and try and do mean things.  Because of our CSP, mean things will have trouble directly doing more than breaking the UI.  However, since we've adopted custom elements, clever mean things could certainly try and trick existing custom elements in doing their bidding.

While it's possible the metadata has already been sanitized to be HTML-safe elsewhere, that seems like it would be weird and would merit a comment.  (Also, it seems like :freddyb's linter should have generated an error on this?  Did the linter get confused or was the lint error whitelisted?)

The expected attack vector for something like this would of course not be the user's own music collection, but instead users foolishly downloading sketchy attachments from email, accidental downloads on sketchy sites (possibly involving some level of clickjacking... I'm not sure how much the browser prompts about downloads and whether it has anti game-tapping suppression, etc.).
(Assignee)

Comment 1

3 years ago
I'll take it.
Assignee: nobody → hub
Created attachment 8667080 [details] [review]
[gaia] hfiguiere:bug1209210-sanitize > mozilla-b2g:master
Depends on: 1209416
(Assignee)

Updated

3 years ago
Attachment #8667080 - Flags: review?(jdarcangelo)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Comment on attachment 8667080 [details] [review]
[gaia] hfiguiere:bug1209210-sanitize > mozilla-b2g:master

Hub: Thanks for taking this! LGTM with nits addressed. Also, I don't know who the owner is for 'shared/js/sanitizer.js', but they should probably look at this too before landing.
Attachment #8667080 - Flags: review?(jdarcangelo) → review+
(Assignee)

Comment 4

3 years ago
As discussed on IRC. the change in sanitizer.js are from the dependent bug. There are two commits in the PR.
I updated the Sanitizer in bug 1209416. Just waiting for check-in.
(Assignee)

Comment 6

3 years ago
merged
https://github.com/mozilla-b2g/gaia/commit/d9317af0306d17c62950995e42f7dcdbbd676c8b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S8 (02Oct)
You need to log in before you can comment on or make changes to this bug.