Crash in mozilla::dom::PostMessageEvent::Run

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: calixte, Assigned: baku)

Tracking

(Blocks 1 bug, {crash, regression})

52 Branch
mozilla55
Unspecified
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

(Whiteboard: [clouseau], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-9ff58ec7-b2ba-4f63-a09f-a305d2170309.
=============================================================

There are 2 crashes on nightly 55 with buildid 20170308030207. In analyzing the backtrace, the regression may have been introduced by patch [1ÿ to fix bug 1314361.

[1] https://hg.mozilla.org/mozilla-central/rev?node=73265f9e2940d06af625ee3b2bd6f57a87c932c2
Reporter

Comment 1

2 years ago
:kmag, is it related to your bug fix ?
Blocks: 1314361
Flags: needinfo?(kmaglione+bmo)
Whiteboard: [clouseau]
It looks like it may be. See bug 1314361 comment 86. It looks like IsOriginAttributesEqualIgnoringAddonId was never updated to check first-party domain when that attribute and the IsOriginAttributesEqualIgnoringFPD helper were added, so it accidentally served that purpose. That check probably needs to be updated to use IsOriginAttributesEqualIgnoringFPD now instead of the IsOriginAttributesEqualIgnoringAddonId check that was removed.
Depends on: 1319773
Flags: needinfo?(kmaglione+bmo) → needinfo?(amarchesini)
Assignee

Comment 4

2 years ago
Posted patch check.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8845832 - Flags: review?(tom)

Comment 5

2 years ago
So I just want to unpack this to make sure I understand it correctly.

What's happening here is we're sending a postMessage event from one window to the other. 

The current assert says:

>       MOZ_DIAGNOSTIC_ASSERT(providedOrigin != targetOrigin ||
>                            (mProvidedPrincipal->OriginAttributesRef() ==
>                              targetPrin->OriginAttributesRef()),
>                            "Unexpected postMessage call to a window with mismatched "
>                            "origin attributes");

Which says: "The two domains that are talking to each other are not the same domain. But, if they are the same domain, then they have identical Origin Attributes." 

This is important to assert because we don't want windows postMessaging to each other if they're in different OA 'zones' - PBM vs non-PBM, Container A vs Container B. Are we certain this isn't possible? I guess if the only ways to get a handle are window.frames, iframe.contextWindow, window.open(), and window.[parent,top,opener] - none of those can trigger opening in a different OA 'zone', so we're safe because of that (and that alone probably...)

Except... if First Party Isolation is turned on, then the different windows will have the FirstParty string filled in in OA, and the equals check will fail. We _want_ First Party Isolation to be ignored in this case because you should be able to postMessage between domains with FPI turned on. The fact that this worked before was just a bug of the (old) IsOriginAttributesEqualIgnoringAddonId function. 


Assuming all that's correct, I have a few questions:

1) How does this assert get triggered? The part I'm confused about is the 'providedOrigin != targetOrigin' - to continue to the OA part of the assert, the two origins have to be identical. Is this it: With FPI enabled, a.com in Window1 opens a window to b.com (Window2) - b.com makes an iframe to a.com (Window2) and passes its window.opener handle to it's iframe. So a.com(in an iframe in Window2 with FirstParty=b.com) attempts to talk to a.com(Window1 with FirstParty=a.com)?  This should be allowed (but it is asserting and crashing.)

2) This check only occurs if the target principal (the principal of the receiving message) is not equal to the provided principal (the principal of the _sending_ window *at the time it was sent*). "If these two windows are the same domain, with identical Origin Attributes - why would they have different principals?" Assuming I got #1 right, is the answer "Because the Target Principal is (or includes) b.com" ?

3) Why do we let them pass if they aren't the same domain? There isn't a way, currently, for a window to get a handle to a window that is in a different OA 'zone'. But hypothetically let's imagine there was (for example we created the feature that lets all example.com windows automatically go to Container Zed). In this case, you could postMessage between containers because the domain check passes. In contrast, if we *only* asserted that the OA attributes were identical (ignoring FPI) - we should still be safe here (not getting any assertions) and we'd be a bit future-proofed. Or did I miss something?
Flags: needinfo?(amarchesini)
Assignee

Comment 6

2 years ago
> with FirstParty=a.com)?  This should be allowed (but it is asserting and
> crashing.)

What do you mean with "and passes its window.opener handle to it's iframe" ?

> 3) Why do we let them pass if they aren't the same domain?

This is a good point. We should change the assertion to compare OAs always.
Flags: needinfo?(amarchesini) → needinfo?(tom)

Comment 7

2 years ago
(In reply to Andrea Marchesini [:baku] from comment #6)
> > with FirstParty=a.com)?  This should be allowed (but it is asserting and
> > crashing.)
> 
> What do you mean with "and passes its window.opener handle to it's iframe" ?

So the only way to postMessage is by getting a handle to a window, right? You get a handle as the return value of window.open(), and you can also get a handle from window.opener. 

So if b.com(in Window2) - which was created by a window.open() call by a.com(in Window1) - has an iframe containing a.com, it could pass its window.opener value (which is a handle to a.com(in Window1) to the iframe and then the iframe could call postMessage.

At least I think that's correct.
Flags: needinfo?(tom) → needinfo?(amarchesini)
STR:

1. Open a container
2. Go to: https://www.alttickets.com search for a concert and click book ticket
3. Go through cart process as guest, then pay by paypal.
4. When paypal loads, it crashes both tabs.
I also compiled Baku's patch works for the STR that I had, but then I also could not reproduce the crash when I removed it also. Maybe something else in central fixed the crash?

My STR was super reproducible in Nightly 55.0a1 Ubuntu.

There appears to be two cart flows for the site above, sometimes it's in an iframe which only replicates when you have purchased. It appears the iframe flow is when you are already logged in.
Comment on attachment 8845832 [details] [diff] [review]
check.patch

As discussed, change it to remove the domain check and just check the OA (using IsOriginAttributesEqualIgnoringFPD) always.
Attachment #8845832 - Flags: review?(tom) → review-
I could reproduce in Nightly 54.01a and 55.01a, but I can't reproduce in a local build (with --enable-debug and with --enable-optimize or --disable-optimize). I wasn't able to debug this via C++, but was able to learn some more via javascript, and ultimately confirm.

So we have Window 1, top level domain is alttickets.com (so FirstParty=alttickets.com) and an iframe for paypal.com.  Then we pop Window 2 whose top level domain is paypal.com (so FirstPArty=paypal.com) and whose window.opener is paypal.com.
Priority: -- → P1
Assignee

Comment 14

2 years ago
Attachment #8845832 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8864425 - Flags: review?(tom)

Updated

2 years ago
Attachment #8864425 - Flags: review?(tom) → review+

Comment 15

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/558841792eab
Fixing an assertion about OriginAttribute comparison in PostMessage, r=tjr

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/558841792eab
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

2 years ago
Depends on: 1373735
You need to log in before you can comment on or make changes to this bug.