Broken same-origin check in SocialService.jsm:_manifestFromData

RESOLVED WONTFIX

Status

()

Firefox
SocialAPI
RESOLVED WONTFIX
4 years ago
7 months ago

People

(Reporter: florian, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox22 unaffected, firefox23- wontfix, firefox24- wontfix, firefox25- wontfix)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 780403 [details] [diff] [review]
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.
Attachment #780403 - Flags: review?
(Reporter)

Updated

4 years ago
Attachment #780403 - Flags: review? → review?(mixedpuppy)
How far does this go back, so that we can set flags for affected builds?
Keywords: sec-moderate
(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.
Assignee: nobody → mixedpuppy
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → affected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
If low-risk enough we could take this in the second-to-last Beta since it's a new regression in FF23
status-firefox22: --- → unaffected
tracking-firefox23: ? → +
tracking-firefox24: ? → +
tracking-firefox25: ? → +
Created attachment 780545 [details] [diff] [review]
manifest-same-origin
Attachment #780403 - Attachment is obsolete: true
Attachment #780403 - Flags: review?(mixedpuppy)
Attachment #780545 - Flags: review?(mhammond)
(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.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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 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?
(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).
(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?)
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.
status-firefox23: affected → wontfix
tracking-firefox23: + → -
tracking-firefox24: + → -
tracking-firefox25: + → -
Group: core-security
Keywords: sec-moderate
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
and triple duh.
https://tbpl.mozilla.org/?tree=Try&rev=b88c2470d530
@markh ping on review
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.
Attachment #780545 - Flags: review?(mhammond) → review-
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.
Flags: needinfo?(gavin.sharp)
What does the patch to remove all the unnecessary same-origin checking look like?
Flags: needinfo?(gavin.sharp)
Assignee: mixedpuppy → nobody
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox24: affected → wontfix
status-firefox25: affected → wontfix
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.