776606 - Sanitize user portraits URLs in the Social API

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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

[Jared Wein [:jaws] (please needinfo? me)]

I'll take this. We will need to get this fixed before uplifting the user portrait feature to Firefox 16.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Slight typo in the STR:
element.style.listStyleImage = "url('javascript:alert(1)')";

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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: [e-mail]]]

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

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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: [e-mail]]]

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

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v3

Sorry about that earlier patch, this one should be better.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56e0971c81ea

Comment 10

[:Benjamin Peterson]

Backed out for test failures in mochitest-oth
https://hg.mozilla.org/integration/mozilla-inbound/rev/83caf3e51946

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Test fixes

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Pushed test fixes and patch to tryserver:
https://tbpl.mozilla.org/?tree=Try&rev=bd16758f3481

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for checkin

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Backed out since it looks like it's causing leaks:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6f3b00a101

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

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

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Updated patch for checkin

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/157026a9b9c6