sandboxed window cannot use postMessage with specific domain specified inside a mozbrowser

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: jack83307, Assigned: bholley)

Tracking

unspecified
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 unaffected, firefox32 unaffected, firefox33 unaffected, firefox34 unaffected, firefox-esr31 unaffected, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(4 attachments)

Reporter

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.114 Safari/537.36

Steps to reproduce:

1. create a sandboxed iframe with allow-scripts permission
2. iframe src was a blob url pointing to an html page with scripts whose source was a data url.  This was done for caching reasons (iframe had to be reloaded and I don't want to incur huge data usage for non firefox os users).  Haven't tried with normal url.
3. a script was added which used parent.postMessage([some message], [parent domain]).  The method for obtaining the parent's domain is unimportant, but has been verified to be the correct domain.


Actual results:

The main page did not receive the message.  This is clearly due to the domain restriction because the main page does receive a message when the domain argument of postMessage is "*".


Expected results:

The main page should have received the message.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I can confirm this issue!
Component: General → DOM
Product: Firefox OS → Core
> I can confirm this issue!

Really?  Care to point to a testcase showing the problem?  Because http://web.mit.edu/bzbarsky/www/testcases/sandboxed-iframes/postMessage-from-sandboxed-iframe.html works just fine as far as I can tell.  And so does http://web.mit.edu/bzbarsky/www/testcases/sandboxed-iframes/postMessage-from-sandboxed-blob-iframe.html
Flags: needinfo?(thewanuki)
Flags: needinfo?(jack83307)
Reporter

Comment 3

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #2)
> > I can confirm this issue!
> 
> Really?  Care to point to a testcase showing the problem?  Because
> http://web.mit.edu/bzbarsky/www/testcases/sandboxed-iframes/postMessage-from-
> sandboxed-iframe.html works just fine as far as I can tell.  And so does
> http://web.mit.edu/bzbarsky/www/testcases/sandboxed-iframes/postMessage-from-
> sandboxed-blob-iframe.html

So I ran in to this problem while working on a project for ForefoxOS called Firetext, you can see the code at http://www.github.com/Codexa/firetext.  Unfortunately, I can't really do much testing to see why it seems to work in this case.  I'm in Spain right now and my laptop's mousepad stopped working so I'll have to get back to you Saturday when I return.
Flags: needinfo?(jack83307)
No problem.  Just let me know!  Hard to fix a problem I can't reproduce and which code inspection suggests should not exist....
Flags: needinfo?(jack83307)
As far as I recall, this problem only presented itself on a Firefox OS device or simulator.  It was fine on a regular (or localhost) web server.

I need to put a testcase together...
Flags: needinfo?(thewanuki)
Please!
Assignee: nobody → thewanuki
My apologies for the delay.  Before compiling a test of my own, I decided to try the two you had linked to on my Firefox OS GP Peak+ with 2.1.  Neither worked.
Hmm.  They work fine for me on Firefox desktop and Firefox on Android.  What's special about Firefox OS here?  I assume Firefox OS does have alert() support but you didn't see an alert when loading those testcases?
I don't know what the difference is on Firefox OS.  It appears to use the latest Gecko.  Also, there is alert() support, but I did not see the success alert.

I do have a hunch that it might be related to additional security measures on FFOS...  Talking to some B2G people.
Those testcases work fine for me in "Firefox" on a Flame as well.  This looks like it has 1.3 installed?
This issue is only present on simulators later than 1.4 (2014-04-04).
----More information----
Platform version: 30.0a2
Build identifier: 20140404014113
Git commit info: 2014-04-04 09:31:33
----Last working version of the simulator----
OS version: 1.3.0.0-prerelease
Platform version: 28.0
Build identifier: 20140331164005
Git commit info: N/A

These simulators can be downloaded from ftp://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/1.4/mac64/ and ftp://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/1.3/mac64/ respectively.
Fabrice, do you know what we can do to narrow this down (e.g. where we can find more prebuilt nightly simulator binaries or equivalent)?
Flags: needinfo?(fabrice)
Boris, all the builds are here:
https://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/

You want to use the "multi" ones, eg https://ftp.mozilla.org/pub/mozilla.org/b2g/nightly/latest-mozilla-central/b2g-34.0a1.multi.linux-i686.tar.bz2 and run b2g/b2g from the archive.
Flags: needinfo?(fabrice)
Thanks!

The regression range I get there is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e7a366c1036c&tochange=cf2d1bd796ea

The obvious change in there that's related to postMessage is bug 949488.  I'll see if I can figure out what's up with that later tonight.
Blocks: 949488
Flags: needinfo?(bzbarsky)
[Tracking Requested - why for this release]:

[Blocking Requested - why for this release]:

So this is definitely a regression from bug 949488.

That bug switched from comparing URIs to comparing principals, and it creates the "target principal" by taking the given domain name and the appid/inmozbrowser booleans from the caller principal.

In this case, the caller principal is a null principal, so claims to have no app id and not be in a mozbrowser.  But the target page is in fact in a mozbrowser, because everything on b2g is, right?  So you get a principal mismatch and the message event is not delivered.

We need to either ignore the mozbrowser/appid stuff here or store them in nullprincipals as needed or something.

We also clearly need some postmessage-to/from-sandbox tests (or need to run them on b2g if we have some already).

Bobby, baku is out.  Can you take a look at this and decide what we should do?

Bug 949488 landed for Firefox 29, so this this affects b2g 1.4 and newer.  :(  I'm marking Firefox as unaffected, since I think we don't end up with mozbrowser/appid bits there.
Assignee: thewanuki → bobbyholley
blocking-b2g: --- → 1.4?
Flags: needinfo?(jack83307)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bobbyholley)
Summary: sandboxed window cannot use postMessage with specific domain specified → sandboxed window cannot use postMessage with specific domain specified inside a mozbrowser
nominate it to 2.1 due to tight schedule on 1.4 now.
blocking-b2g: 1.4? → 2.1?
This has the potential to break web pages on b2g only.  If we get a fix here in time how do we nominate this for approval for 1.4?
Flags: needinfo?(itsay)
Assignee

Comment 20

5 years ago
I wrote up a test and spent a while trying to get it to fail on mochitest-b2g-desktop. As ahal and I finally discovered, IPC and mozBrowser are both disabled in that configuration, making it useless for this purpose.

Emulator mochitests should do this right, but building one would take all day, so I'm going to see if I can just do this using CI.

Here's the test pushed without any code changes. It should fail:

https://tbpl.mozilla.org/?tree=Try&rev=60ff96bf9be9

Assuming that looks good, I'll follow it up with code changes.
Assignee

Comment 21

5 years ago
(In reply to Bobby Holley (:bholley) from comment #20) 
> Here's the test pushed without any code changes. It should fail:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=60ff96bf9be9

It does indeed fail! I'll work up a patch.
Assignee

Comment 22

5 years ago
Attachment #8462116 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #19)
> This has the potential to break web pages on b2g only.  If we get a fix here
> in time how do we nominate this for approval for 1.4?

Hi Boris,

Thanks you for the help. To request approval for 1.4 uplift, please click into the detail of the patch. If this is gecko patch, please set "approval‑mozilla‑b2g30" to "?". If this is gaia patch, please set "approval‑gaia‑v1.4" to "?"
Flags: needinfo?(itsay)
Comment on attachment 8462116 [details] [diff] [review]
Part 4 - Tests. v1

r=me
Attachment #8462116 - Flags: review?(bzbarsky) → review+
Assignee

Updated

5 years ago
Attachment #8462116 - Attachment description: Tests. v1 → Part 4 - Tests. v1
Flags: needinfo?(bobbyholley)
Comment on attachment 8462713 [details] [diff] [review]
Part 1 - Hoist GetAppStatus into a static method on nsScriptSecurityManager. v1

r=me
Attachment #8462713 - Flags: review?(bzbarsky) → review+
Comment on attachment 8462714 [details] [diff] [review]
Part 2 - Let Null Principals have App IDs and mozBrowser status. v1

r=me
Attachment #8462714 - Flags: review?(bzbarsky) → review+
Comment on attachment 8462716 [details] [diff] [review]
Part 3 - Borrow App ID and mozBrowser-ness when creating sandbox null principals. v1

This seems fine, though I'd also be ok with making nsNullPrincipal::Init infallible, I think.
Attachment #8462716 - Flags: review?(bzbarsky) → review+
You may need to adjust the callsite added in bug 1037271 too...
Assignee

Comment 33

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #32)
> You may need to adjust the callsite added in bug 1037271 too...

I don't think so, because if the loading principal is null, we've got nothing to inherit attributes from anyway.

But yes, the push in comment 28 still failed the test. I rejiggered the test so that I could run it in a mozBrowser locally, and it passed. That suggested that the problem was with the 'app' bit, and I took another look at the patches:

> +/* static */ already_AddRefed<nsNullPrincipal>
> +nsNullPrincipal::CreateWithInheritedAttributes(nsIPrincipal* aInheritFrom)
> +{
> +  nsRefPtr<nsNullPrincipal> nullPrin = new nsNullPrincipal();
> +  nsresult rv = nullPrin->Init(aInheritFrom->GetAppStatus(),
> +                               aInheritFrom->GetIsInBrowserElement());
> +  return NS_SUCCEEDED(rv) ? nullPrin.forget() : nullptr;
> +}
> +

Spot the bug? Solution at [1]. The 90s called and want their untyped enums back.

Fixed this and pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=e78f643222bb

[1] We should be invoking GetAppId() instead of GetAppStatus().
Assignee

Comment 35

5 years ago
Comment on attachment 8462116 [details] [diff] [review]
Part 4 - Tests. v1

Approval request applies to all the patches.

[Approval Request Comment]
[Feature/regressing bug #]: bug 949488
[User impact if declined]: serious web-compat regression, affecting b2g only.
[Describe test coverage new/current, TBPL]: Automated test included. Just pushed to m-i. 
[Risks and why]: Low risk.
[String/UUID change made/needed]: None
Attachment #8462116 - Flags: approval-mozilla-b2g32?
Attachment #8462116 - Flags: approval-mozilla-b2g30?
Attachment #8462116 - Flags: approval-mozilla-aurora?
Backed out for more non-unified bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d735d53ecce2

https://tbpl.mozilla.org/php/getParsedLog.php?id=44731671&tree=Mozilla-Inbound

Note that you can test non-unified locally or on Try via the --disable-unified-compilation flag.
Comment on attachment 8462116 [details] [diff] [review]
Part 4 - Tests. v1

minus as I see this patch was backed out.

Aurora - Current 33, does not correspond with a B2G release so uplift is not required.
b2g30 - We're pretty late for B2G 1.4, but may be able to accept this change. You should nom for 1.4?
b2g32 - We can still accept this change on 2.0 if necessary.
m-c - This is 2.1 and can land without any approval.

I skimmed the bug but am not clear how severe of a web compatibility issue this is. Can you expand on how serious think issue is?
Attachment #8462116 - Flags: approval-mozilla-b2g32?
Attachment #8462116 - Flags: approval-mozilla-b2g32-
Attachment #8462116 - Flags: approval-mozilla-b2g30?
Attachment #8462116 - Flags: approval-mozilla-b2g30-
Attachment #8462116 - Flags: approval-mozilla-aurora?
Attachment #8462116 - Flags: approval-mozilla-aurora-
Meant also to note that I only see approval requests on the test patch but I assume that you want all the parts uplifted. Can you please set the approval requests on each patch that you are proposing for uplift?
Assignee

Comment 42

5 years ago
(In reply to Lawrence Mandel [:lmandel] from comment #40)
> minus as I see this patch was backed out.

In future cases like this (where the backout is mechanical rather than functional), please consider just leaving this to marinate in the request queue for a day or two rather than marking it a-. Doing so means that the developer has to remember to flag for approval again, which increases the risk of losing time on the fix (especially in cases like this one where there is no one from product tracking the bug). The uplifts themselves never come from the patch on bugzilla, but rather are cherry-picked from whatever actually makes it to m-c.

> Aurora - Current 33, does not correspond with a B2G release so uplift is not
> required.
> b2g30 - We're pretty late for B2G 1.4, but may be able to accept this
> change. You should nom for 1.4?

This happened, and it sounds like b2g isn't going to block on this, but would potentially take a patch (see comment 18, comment 19, and comment 23).

> I skimmed the bug but am not clear how severe of a web compatibility issue
> this is. Can you expand on how serious think issue is?

When a web page uses <iframe sandbox> (an HTML5 security feature), the page inside the sandbox is supposed to be able to communicate with pages outside the sandbox via postMessage. With this bug, web pages (and apps) loaded in b2g won't be able to do that. I don't know how widely sandboxed iframes are actually used, nor how common it is for the sandboxed content to attempt to communicate with the sandboxer, so it's pretty hard for me to say. :\

I do know that Boris tends to have excellent spidey-sense about these things, and he seems moderately worried. The idea of it breaking some big (potentially locale-specific) website somewhere doesn't seem all that far-fetched.

(In reply to Lawrence Mandel [:lmandel] from comment #41)
> Meant also to note that I only see approval requests on the test patch but I
> assume that you want all the parts uplifted.

Yes. See comment 35.

> Can you please set the approval requests on each patch that you are proposing for uplift?

That seems suboptimal, and has never been required before - can you elaborate on why?

These patches should all be uplifted together, or not at all, so 4 separate requests increases data (and noise, if there's a screwup) without increasing information. IMO, we should actively avoid per-patch procedural overhead because such overhead discourages developers from splitting their patches into bite-sized reviewable/bisectable chunks, which impacts regression analysis.
Assignee

Comment 45

5 years ago
Comment on attachment 8462116 [details] [diff] [review]
Part 4 - Tests. v1

This has hit m-c now. Renominating per comment 42.

This approval request applies to all 4 patches in this bug.
Attachment #8462116 - Flags: approval-mozilla-b2g32?
Attachment #8462116 - Flags: approval-mozilla-b2g32-
Attachment #8462116 - Flags: approval-mozilla-b2g30?
Attachment #8462116 - Flags: approval-mozilla-b2g30-
(In reply to Bobby Holley (:bholley) from comment #42)
> In future cases like this (where the backout is mechanical rather than
> functional), please consider just leaving this to marinate in the request
> queue for a day or two rather than marking it a-. Doing so means that the
> developer has to remember to flag for approval again, which increases the
> risk of losing time on the fix (especially in cases like this one where
> there is no one from product tracking the bug). The uplifts themselves never
> come from the patch on bugzilla, but rather are cherry-picked from whatever
> actually makes it to m-c.

Good point. Noted.

> I do know that Boris tends to have excellent spidey-sense about these
> things, and he seems moderately worried. The idea of it breaking some big
> (potentially locale-specific) website somewhere doesn't seem all that
> far-fetched.

Agreed.

> > Can you please set the approval requests on each patch that you are proposing for uplift?
> 
> That seems suboptimal, and has never been required before - can you
> elaborate on why?
> 
> These patches should all be uplifted together, or not at all, so 4 separate
> requests increases data (and noise, if there's a screwup) without increasing
> information. IMO, we should actively avoid per-patch procedural overhead
> because such overhead discourages developers from splitting their patches
> into bite-sized reviewable/bisectable chunks, which impacts regression
> analysis.

Flagging each patch is helpful as many bugs have multiple patches/attachments and it can be difficult to determine exactly which patches should be uplifted. I would rather be explicit in the request rather than miss a patch or uplift a patch accidentally. To be clear, I don't want to see the questionnaire multiple times. Just the flag on the patch. (I'm going to have to set the flag on each patch so that the sheriffs know what to uplift anyway.)

I'm definitely not trying to create extra overhead. If setting the flag on multiple patches is a real burden, I'm happy to talk about improvements that we can make that make your life easier.
Comment on attachment 8462713 [details] [diff] [review]
Part 1 - Hoist GetAppStatus into a static method on nsScriptSecurityManager. v1

Approving this B2G Web compatibility fix for 1.4 and 2.0.
Attachment #8462713 - Flags: approval-mozilla-b2g32+
Attachment #8462713 - Flags: approval-mozilla-b2g30+
Comment on attachment 8462714 [details] [diff] [review]
Part 2 - Let Null Principals have App IDs and mozBrowser status. v1

[Triage Comment]
Attachment #8462714 - Flags: approval-mozilla-b2g32+
Attachment #8462714 - Flags: approval-mozilla-b2g30+
Attachment #8462716 - Flags: approval-mozilla-b2g32+
Attachment #8462716 - Flags: approval-mozilla-b2g30+
Attachment #8462116 - Flags: approval-mozilla-b2g32?
Attachment #8462116 - Flags: approval-mozilla-b2g32+
Attachment #8462116 - Flags: approval-mozilla-b2g30?
Attachment #8462116 - Flags: approval-mozilla-b2g30+
Assignee

Comment 49

5 years ago
(In reply to Lawrence Mandel [:lmandel] from comment #46)
> Flagging each patch is helpful as many bugs have multiple
> patches/attachments and it can be difficult to determine exactly which
> patches should be uplifted.

The issues is really that, IMO, the canonical thing being uplifted generally shouldn't be "the patches attached to bugzilla", but rather "the patches for this bug that landed on mozilla-central". it's quite common for extra fixes (and even additional patches!) to end up in the final version, and we don't want to miss those on the uplift.

The usual procedure goes something like this:
* I say "we should uplift this bug to X". Note that the granularity is generally "this bug" and not "this patch".
* Ryan tries to uplift it by cherry-picking the bug's patches from central. He succeeds 85% of the time, in which cases I don't have to do anything. In the other cases (where there are non-trivial merge conflicts) he flags me for needinfo, and I do the cherry-picking and pushing myself.

> (I'm
> going to have to set the flag on each patch so that the sheriffs know what
> to uplift anyway.)

Ryan has never taken issue with my doing it this way (and he uplifts lots and lots of my bugs). Ryan, do you have an opinion?

> I'm definitely not trying to create extra overhead. If setting the flag on
> multiple patches is a real burden

When a bug has more than 2 or 3 patches and needs to be uplifted "everywhere" (something that I do quite often in my line of work), it is.

> I'm happy to talk about improvements that
> we can make that make your life easier.

At least for me, requesting approval on patches is the Wrong Thing. Bugzilla doesn't let me do otherwise, hence the hack of saying "this request applies to the entire bug". But a bugzilla feature that lets me flag the entire bug for uplift would be very welcome. :-)
(In reply to Bobby Holley (:bholley) from comment #49)
> Ryan has never taken issue with my doing it this way (and he uplifts lots
> and lots of my bugs). Ryan, do you have an opinion?

Yes, it's always been a bit of a grey area for multi-patch bugs. I think that in general, the assumption is that if there are multiple patches that landed on m-c, all need to land on the branches unless explicitly noted otherwise by devs. That said, a "this request is for all patches" comment never hurts for clarity (but agreed that Bugzilla is really cumbersome for mass-setting flags on multiple attachments). I think if it were easier to do so, that'd be the preferred way, but at least for now, what we've been doing seems to work well, so I'm not overly worried about it.
Speaking of which, I made it as far as rebasing part 3 onto b2g32 before I hit conflicts that you'll need to take care of :)
Flags: needinfo?(bobbyholley)
Assignee

Comment 52

5 years ago
(In reply to Bobby Holley (:bholley) from comment #49)
> At least for me, requesting approval on patches is the Wrong Thing. Bugzilla
> doesn't let me do otherwise, hence the hack of saying "this request applies
> to the entire bug". But a bugzilla feature that lets me flag the entire bug
> for uplift would be very welcome. :-)

I filed bug 1046827 for this.
clearing the nom for 2,1?, as this has landed already on that branch.
blocking-b2g: 2.1? → ---
Depends on: 1331295
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.