Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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.