Closed
Bug 776606
Opened 12 years ago
Closed 12 years ago
Sanitize user portraits URLs in the Social API
Categories
(Firefox Graveyard :: SocialAPI, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 17
People
(Reporter: jaws, Assigned: jaws)
References
Details
Attachments
(1 file, 5 obsolete files)
4.66 KB,
patch
|
Details | Diff | Splinter Review |
The social API feature requests a user's portrait from a provider and sets the portrait to be the CSS list-style-image of an element. Investigations in bug 773743 found that arbitrary JS execution could be performed within the CSS value, for example: element.style.listStyleImage = "url(javascript:alert(1))"; Running this code generates a JS error (showing that the JS is being executed) because this code does not have access to |window|. However, https://bugzilla.mozilla.org/show_bug.cgi?id=773743#c9 says that this code is probably running with chrome-privileges, so we shouldn't disregard any potential for risk here.
Assignee | ||
Comment 1•12 years ago
|
||
I'll take this. We will need to get this fixed before uplifting the user portrait feature to Firefox 16.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Whiteboard: [Fx16]
Assignee | ||
Comment 2•12 years ago
|
||
Slight typo in the STR: element.style.listStyleImage = "url('javascript:alert(1)')";
Assignee | ||
Comment 3•12 years ago
|
||
With this patch I'm no longer able to reproduce the JS error, and it also now uses the same approach and fallback as the updateProfile function (http://hg.mozilla.org/mozilla-central/file/46bd216c417f/browser/base/content/browser-social.js#l285).
Attachment #645900 -
Flags: review?(gavin.sharp)
Attachment #645900 -
Flags: feedback?(amuntner)
Comment 4•12 years ago
|
||
Comment on attachment 645900 [details] [diff] [review] Patch I don't know that the updateProfile code is any better, it doesn't do any extra validation either. I don't think it's safe to set a chrome xul:image's src to some arbitrary content-supplied value. We certainly don't let that happen for favicons (see shouldLoadFavicon, which checks schemeIs(http[s])). Seems like we should do something similar.
Attachment #645900 -
Flags: review?(gavin.sharp) → review-
Assignee | ||
Comment 5•12 years ago
|
||
This patch sanitizes the portrait whenever the profile is updated so that we don't have to worry about consumers forgetting to sanitize.
Attachment #645900 -
Attachment is obsolete: true
Attachment #645900 -
Flags: feedback?(amuntner)
Attachment #645967 -
Flags: review?(gavin.sharp)
Attachment #645967 -
Flags: feedback?(amuntner)
Comment 6•12 years ago
|
||
Comment on attachment 645967 [details] [diff] [review] Patch v2 >diff --git a/toolkit/components/social/SocialService.jsm b/toolkit/components/social/SocialService.jsm > updateUserProfile: function(profile) { >+ try { >+ let portraitUri = makeURI(profile.portrait); Does this work? I don't see makeURI defined anywhere here (it is defined in browser chrome windows via contentAreaUtils.js, but that's not loaded in this module scope). >+ if (!portraitUri || !(uri instanceof Ci.nsIURI) || Similarly, "uri" doesn't seem to be defined (and a null check and instanceof checks are redundant, makeURI will throw if it fails).
Attachment #645967 -
Flags: review?(gavin.sharp) → review-
Assignee | ||
Comment 7•12 years ago
|
||
Sorry about that earlier patch, this one should be better.
Attachment #645967 -
Attachment is obsolete: true
Attachment #645967 -
Flags: feedback?(amuntner)
Attachment #646357 -
Flags: review?(gavin.sharp)
Attachment #646357 -
Flags: feedback?(amuntner)
Comment 8•12 years ago
|
||
Comment on attachment 646357 [details] [diff] [review] Patch v3 Nit: Just get .scheme once rather than calling schemeIs three times (schemeIs is more of an optimization to avoid an unnecessary copy when called from C++, when called in JS through xpconnect the overhead from three function calls+argument conversion outweighs that of the simple getter).
Attachment #646357 -
Flags: review?(gavin.sharp)
Attachment #646357 -
Flags: review+
Attachment #646357 -
Flags: feedback?(amuntner)
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/56e0971c81ea
Flags: in-testsuite-
Target Milestone: --- → Firefox 17
Comment 10•12 years ago
|
||
Backed out for test failures in mochitest-oth https://hg.mozilla.org/integration/mozilla-inbound/rev/83caf3e51946
Assignee | ||
Comment 11•12 years ago
|
||
Assignee | ||
Comment 12•12 years ago
|
||
Pushed test fixes and patch to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=bd16758f3481
Summary: Possible chrome-level JS injection attack with user portraits in the Social API → Sanitize user portraits URLs in the Social API
Assignee | ||
Comment 13•12 years ago
|
||
Updated push to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=2dcbdb058d3f Landed again: https://hg.mozilla.org/integration/mozilla-inbound/rev/68e5b042c0f5
Flags: in-testsuite- → in-testsuite+
Target Milestone: Firefox 17 → ---
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #646357 -
Attachment is obsolete: true
Attachment #647458 -
Attachment is obsolete: true
Assignee | ||
Comment 15•12 years ago
|
||
Backed out since it looks like it's causing leaks: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6f3b00a101
Assignee | ||
Comment 16•12 years ago
|
||
I've moved the updating of browser_shareButton.js to bug 780010 while I investigate the leaks that are being reported with those test changes. Try push: https://tbpl.mozilla.org/?tree=Try&rev=6a01e9632c15 Pushed to inbound, hopefully it sticks this time: https://hg.mozilla.org/integration/mozilla-inbound/rev/157026a9b9c6
Whiteboard: [Fx16]
Target Milestone: --- → Firefox 17
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #648023 -
Attachment is obsolete: true
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/157026a9b9c6
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•