Last Comment Bug 776606 - Sanitize user portraits URLs in the Social API
: Sanitize user portraits URLs in the Social API
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: 17 Branch
: All All
: -- normal (vote)
: Firefox 17
Assigned To: (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
:
Mentors:
Depends on:
Blocks: 773743 780010
  Show dependency treegraph
 
Reported: 2012-07-23 10:58 PDT by (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
Modified: 2012-08-03 07:32 PDT (History)
3 users (show)
jaws: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.28 KB, patch)
2012-07-25 15:16 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
gavin.sharp: review-
Details | Diff | Review
Patch v2 (2.58 KB, patch)
2012-07-25 17:35 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
gavin.sharp: review-
Details | Diff | Review
Patch v3 (2.56 KB, patch)
2012-07-26 14:45 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
gavin.sharp: review+
Details | Diff | Review
Test fixes (2.23 KB, patch)
2012-07-30 23:52 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review
Patch for checkin (8.12 KB, patch)
2012-08-01 11:32 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review
Updated patch for checkin (4.66 KB, patch)
2012-08-02 15:32 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review

Description (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-23 10:58:54 PDT
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.
Comment 1 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-23 11:00:31 PDT
I'll take this. We will need to get this fixed before uplifting the user portrait feature to Firefox 16.
Comment 2 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-25 11:45:30 PDT
Slight typo in the STR:
element.style.listStyleImage = "url('javascript:alert(1)')";
Comment 3 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-25 15:16:23 PDT
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).
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-25 17:08:37 PDT
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.
Comment 5 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-25 17:35:13 PDT
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.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-26 13:15:28 PDT
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).
Comment 7 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-26 14:45:35 PDT
Created attachment 646357 [details] [diff] [review]
Patch v3

Sorry about that earlier patch, this one should be better.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-30 16:54:32 PDT
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).
Comment 9 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-30 19:30:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/56e0971c81ea
Comment 10 :Benjamin Peterson 2012-07-30 21:01:21 PDT
Backed out for test failures in mochitest-oth
https://hg.mozilla.org/integration/mozilla-inbound/rev/83caf3e51946
Comment 11 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-30 23:52:06 PDT
Created attachment 647458 [details] [diff] [review]
Test fixes
Comment 12 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-31 15:29:30 PDT
Pushed test fixes and patch to tryserver:
https://tbpl.mozilla.org/?tree=Try&rev=bd16758f3481
Comment 13 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-08-01 11:32:26 PDT
Updated push to tryserver:
https://tbpl.mozilla.org/?tree=Try&rev=2dcbdb058d3f

Landed again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68e5b042c0f5
Comment 14 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-08-01 11:32:50 PDT
Created attachment 648023 [details] [diff] [review]
Patch for checkin
Comment 15 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-08-01 13:57:39 PDT
Backed out since it looks like it's causing leaks:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6f3b00a101
Comment 16 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-08-02 15:32:06 PDT
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
Comment 17 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-08-02 15:32:40 PDT
Created attachment 648520 [details] [diff] [review]
Updated patch for checkin
Comment 18 Ed Morley [:emorley] 2012-08-03 07:32:59 PDT
https://hg.mozilla.org/mozilla-central/rev/157026a9b9c6

Note You need to log in before you can comment on or make changes to this bug.