Closed Bug 1209210 Opened 9 years ago Closed 9 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
FxOS-S8 (02Oct)

People

(Reporter: asuth, Assigned: hub)

References

Details

Attachments

(1 file)

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.).
I'll take it.
Assignee: nobody → hub
Attachment #8667080 - Flags: review?(jdarcangelo)
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+
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.
merged
https://github.com/mozilla-b2g/gaia/commit/d9317af0306d17c62950995e42f7dcdbbd676c8b
Status: ASSIGNED → RESOLVED
Closed: 9 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.

Attachment

General

Created:
Updated:
Size: