Sanitize user portraits URLs in the Social API

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

17 Branch
Firefox 17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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.
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]
Slight typo in the STR:
element.style.listStyleImage = "url('javascript:alert(1)')";
Created attachment 645900 [details] [diff] [review]
Patch

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 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-
Created attachment 645967 [details] [diff] [review]
Patch v2

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 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-
Created attachment 646357 [details] [diff] [review]
Patch v3

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 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)
https://hg.mozilla.org/integration/mozilla-inbound/rev/56e0971c81ea
Flags: in-testsuite-
Target Milestone: --- → Firefox 17

Comment 10

5 years ago
Backed out for test failures in mochitest-oth
https://hg.mozilla.org/integration/mozilla-inbound/rev/83caf3e51946
Created attachment 647458 [details] [diff] [review]
Test fixes
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
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 → ---
Created attachment 648023 [details] [diff] [review]
Patch for checkin
Attachment #646357 - Attachment is obsolete: true
Attachment #647458 - Attachment is obsolete: true
Backed out since it looks like it's causing leaks:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6f3b00a101
Blocks: 780010
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
Created attachment 648520 [details] [diff] [review]
Updated patch for checkin
Attachment #648023 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/157026a9b9c6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.