897485 - Broken same-origin check in SocialService.jsm:_manifestFromData

Status: RESOLVED WONTFIX

(Firefox Graveyard :: SocialAPI, defect)

Description

[Florian Quèze [:florian]]

Attachment: same-origin-check-SocialService.jsm.patch

Impact: A social provider can install itself with a workerURL, sidebarURL or shareURL that doesn't have the same origin as the provider itself.

The same-origin check is done at install time, and there's no check at when starting the provider. I've verified that a provider from localhost:5000 can get http://google.com to show up as the sidebar.

With the attached fix, Firefox refuses to install a provider that contain URLs to different origins.

Requesting review from Shane mostly so that this doesn't get lost, but I expect the patch to need a test before landing, and I don't have time to write it before going in vacations.

Comment 1

[Matt Wobensmith [:mwobensmith][:matt:]]

How far does this go back, so that we can set flags for affected builds?

Comment 2

[Shane Caraveo (:mixedpuppy)]

(In reply to Matt Wobensmith from comment #1)
> How far does this go back, so that we can set flags for affected builds?

Fx23, so we should be able to uplift this.  Since Florian is on vacation now, I'll take over the patch to do the tests, and pass the review over to markh.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

If low-risk enough we could take this in the second-to-last Beta since it's a new regression in FF23

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: manifest-same-origin

Comment 5

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

(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> The same-origin check is done at install time, and there's no check at when
> starting the provider. I've verified that a provider from localhost:5000 can
> get http://google.com to show up as the sidebar.

A provider can also do this by having their sidebar URL redirect to Google.com, right? I'm not sure I understand the attack that we're trying to prevent, here.

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> (In reply to Florian Quèze [:florian] [:flo] from comment #0)
> > The same-origin check is done at install time, and there's no check at when
> > starting the provider. I've verified that a provider from localhost:5000 can
> > get http://google.com to show up as the sidebar.
> 
> A provider can also do this by having their sidebar URL redirect to
> Google.com, right? I'm not sure I understand the attack that we're trying to
> prevent, here.

I don't think it's an attack vector as much as it is a manifest validation bug.  I'm inclined towards requiring same-origin for the primary urls in the manifest (as the comments in the code there indicate).  Redirecting content panels to other origins has valid use cases and should be allowed.  This probably is not necessary to be a security bug.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 780545 [details] [diff] [review]
manifest-same-origin

Review of attachment 780545 [details] [diff] [review]:
-----------------------------------------------------------------

The patch is good, as the comments and code certainly imply we are doing the same-origin check there.  However, as Gavin implied in an earlier comment, I don't see how this is actually offering any safety if those same URLs can redirect to somewhere off-origin.

IOW, while the patch is good, I'm not convinced it is enough to mitigate anything.  OTOH, if it's not considered worthwhile to check for those redirections, then we probably don't need this patch and instead can just tweak the comments/code so it doesn't *look* like we are doing the same-origin check?

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #7)

> The patch is good, as the comments and code certainly imply we are doing the
> same-origin check there.  However, as Gavin implied in an earlier comment, I
> don't see how this is actually offering any safety if those same URLs can
> redirect to somewhere off-origin.
> 
> IOW, while the patch is good, I'm not convinced it is enough to mitigate
> anything.  OTOH, if it's not considered worthwhile to check for those
> redirections, then we probably don't need this patch and instead can just
> tweak the comments/code so it doesn't *look* like we are doing the
> same-origin check?

I dont see this as a security issue or mitigation, rather an enforcement that an app is initially contained in a domain.  One valid use case for allowing the content area's to redirect to another domain is, for example, googles single signon which exists on it's own domain.  We do otherwise enforce same-origin (upon open) in a number of places, such as the flyouts, chatwindows, etc., and mozSocial injection requires same-origin as the manifest.

At an absolute minimum I do think the workerURL should be same-origin since there is no UI the user can look into to see what the workerURL is.  Workers (at least by spec) cannot change location since location is a getter only (http://www.w3.org/TR/workers/#workerlocation).

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #8)

> I dont see this as a security issue or mitigation, rather an enforcement
> that an app is initially contained in a domain.

Given there are valid use-cases to redirect, and that such redirections would be largely invisible to the user, why bother enforcing this in the first place?

> At an absolute minimum I do think the workerURL should be same-origin since
> there is no UI the user can look into to see what the workerURL is.  Workers
> (at least by spec) cannot change location since location is a getter only
> (http://www.w3.org/TR/workers/#workerlocation).

As Gavin pointed out in another bug, our worker implementation probably *does* let them redirect.  So it seems reasonable we should prevent this.  But that's still leaves the question open about why exactly we must force a same-origin check for this in the manifest (eg, why isn't it a valid use-case that a domain wants to serve their worker from their CDN?)

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Conversation on this bug (and the sec-moderate rating) lead me to believe this isn't actually needing to be tracked and when there's a call on the best way to deal with this please nominate for uplift if low risk.

Comment 11

[Shane Caraveo (:mixedpuppy)]

per irc with gavin, we'll fix the inconsistency and file a later bug to see about lifting the restriction.

https://tbpl.mozilla.org/?tree=Try&rev=6423c88f736c

Comment 12

[Shane Caraveo (:mixedpuppy)]

and triple duh.
https://tbpl.mozilla.org/?tree=Try&rev=b88c2470d530

Comment 13

[Shane Caraveo (:mixedpuppy)]

@markh ping on review

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 780545 [details] [diff] [review]
manifest-same-origin

Review of attachment 780545 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> @markh ping on review

See comment 7 and comment 9 - IMO this change is pointless and would just *force* providers to use redirection when they have a valid use-case for hosting off-origin.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Forcing them to have same-origin was the original scope of the manifest checking, however, I don't really care one way or another.  Gavin suggested fixing the check for now with another bug to follow up on a decision around removing the restriction.  I'll leave it up to him which way to go.

Comment 16

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

What does the patch to remove all the unnecessary same-origin checking look like?