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)
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.).
Comment 2•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8667080 -
Flags: review?(jdarcangelo)
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 3•9 years ago
|
||
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•9 years ago
|
||
As discussed on IRC. the change in sanitizer.js are from the dependent bug. There are two commits in the PR.
Comment 5•9 years ago
|
||
I updated the Sanitizer in bug 1209416. Just waiting for check-in.
Assignee | ||
Comment 6•9 years ago
|
||
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.
Description
•