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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[jack83307]

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.

Comment 1

[Joshua Smith [:joshua-s]]

I can confirm this issue!

Comment 2

[Boris Zbarsky [:bzbarsky]]

> 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

Comment 3

[jack83307]

(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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

No problem.  Just let me know!  Hard to fix a problem I can't reproduce and which code inspection suggests should not exist....

Comment 5

[Joshua Smith [:joshua-s]]

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...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Please!

Comment 7

[Joshua Smith [:joshua-s]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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?

Comment 9

[Joshua Smith [:joshua-s]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Those testcases work fine for me in "Firefox" on a Flame as well.  This looks like it has 1.3 installed?

Comment 11

[Joshua Smith [:joshua-s]]

This issue is only present on simulators later than 1.4 (2014-04-04).

Comment 12

[Joshua Smith [:joshua-s]]

----More information----
Platform version: 30.0a2
Build identifier: 20140404014113
Git commit info: 2014-04-04 09:31:33

Comment 13

[Joshua Smith [:joshua-s]]

----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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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)?

Comment 15

[[:fabrice] Fabrice Desré]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Boris Zbarsky [: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.

Comment 18

[Ivan Tsay (:ITsay)]

nominate it to 2.1 due to tight schedule on 1.4 now.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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?

Comment 20

[Bobby Holley (:bholley)]

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.

Comment 21

[Bobby Holley (:bholley)]

(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.

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 4 - Tests. v1

Comment 23

[Ivan Tsay (:ITsay)]

(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 "?"

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 25

[Bobby Holley (:bholley)]

Attachment: Part 1 - Hoist GetAppStatus into a static method on nsScriptSecurityManager. v1

Comment 26

[Bobby Holley (:bholley)]

Attachment: Part 2 - Let Null Principals have App IDs and mozBrowser status. v1

Comment 27

[Bobby Holley (:bholley)]

Attachment: Part 3 - Borrow App ID and mozBrowser-ness when creating sandbox null principals. v1

Comment 28

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=2fdfc4952922

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8462713 [details] [diff] [review]
Part 1 - Hoist GetAppStatus into a static method on nsScriptSecurityManager. v1

r=me

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8462714 [details] [diff] [review]
Part 2 - Let Null Principals have App IDs and mozBrowser status. v1

r=me

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[Boris Zbarsky [:bzbarsky]]

You may need to adjust the callsite added in bug 1037271 too...

Comment 33

[Bobby Holley (:bholley)]

(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().

Comment 34

[Bobby Holley (:bholley)]

Green.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/154922edf5fe
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d6111b0603f5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/37fbaf69c6e0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/34f9a0e7dbde

Comment 35

[Bobby Holley (:bholley)]

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

Comment 36

[Bobby Holley (:bholley)]

Followup fix for linux build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/d79b991a8d96

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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 38

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=71103a28ff9f

Comment 39

[Bobby Holley (:bholley)]

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

Comment 40

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 41

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 42

[Bobby Holley (:bholley)]

(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.

Comment 43

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f163c6ca88ee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/36fe477bcbcf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d036623c626c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/51b446de4a84

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f163c6ca88ee
https://hg.mozilla.org/mozilla-central/rev/36fe477bcbcf
https://hg.mozilla.org/mozilla-central/rev/d036623c626c
https://hg.mozilla.org/mozilla-central/rev/51b446de4a84

Comment 45

[Bobby Holley (:bholley)]

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.

Comment 46

[Lawrence Mandel [:lmandel] (use needinfo)]

(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 47

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 48

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8462714 [details] [diff] [review]
Part 2 - Let Null Principals have App IDs and mozBrowser status. v1

[Triage Comment]

Comment 49

[Bobby Holley (:bholley)]

(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. :-)

Comment 50

[Ryan VanderMeulen [:RyanVM]]

(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.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 52

[Bobby Holley (:bholley)]

(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.

Comment 53

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/pushloghtml?changeset=3f7db58a354c
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/pushloghtml?changeset=dbba6bee8242

Comment 54

[bhavana bajaj [:bajaj]]

clearing the nom for 2,1?, as this has landed already on that branch.