Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#16300 closed task (fixed)

Make sure the BroadcastChannel API adheres to our URL bar domain isolation

Reported by: gk Owned by: mcs
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: ff38-esr, tbb-linkability, tbb-5.0a-highrisk, TorBrowserTeam201506R, GeorgKoppen201506R
Cc: brade, arthuredelstein, gk, mikeperry Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The BroadcastChannel API allows cross-site communication within the same origin. We have a stronger notion of "same origin": the same URL bar domain. Thus, we must restrain this API to make it adhere to our URL bar isolation scheme.

Child Tickets

Attachments (1)

0001-Bug-16300-Isolate-Broadcast-Channels-to-first-party.patch (17.6 KB) - added by mcs 4 years ago.
proposed fix

Download all attachments as: .zip

Change History (15)

comment:1 Changed 4 years ago by mcs

Cc: brade added
Owner: changed from tbb-team to mcs
Status: newassigned

Kathy and I will take this ticket.

comment:2 Changed 4 years ago by mikeperry

Keywords: tbb-5.0a-highrisk added

Tag the set of things that are risky to debut in the 5.0-stable release without testing in a prior alpha.

comment:3 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201506 added

Ensure all tbb-5.0a items are on the June radar.

comment:4 Changed 4 years ago by mcs

Cc: arthuredelstein gk mikeperry added

comment:5 Changed 4 years ago by mcs

Keywords: TorBrowserTeam201506R added; TorBrowserTeam201506 removed
Status: assignedneeds_review

I attached our proposed fix. Please review.

The patch is kind of long, but many of the changes involve just passing the isolation host through. Kathy and I think this approach is best and that it is what Mozilla will want (an alternative would be to hack the isolation domain into the existing origin string).

We did disallow use of Broadcast Channels from SharedWorkers when isolation is enabled because, as with blob URLs (#15502), there is no good way to get at the document or channel.

comment:6 Changed 4 years ago by gk

The patch looks good to me. Do you have a test up somewhere which would let me play with that API in an ESR 38 based Tor Browser? (+ plus having a unit test for this API modification would be helpful as well, I guess).

comment:7 Changed 4 years ago by gk

Keywords: GeorgKoppen201506R added

comment:8 in reply to:  6 Changed 4 years ago by mcs

Replying to gk:

The patch looks good to me. Do you have a test up somewhere which would let me play with that API in an ESR 38 based Tor Browser? (+ plus having a unit test for this API modification would be helpful as well, I guess).

Thanks for your review so far. We will work on creating some mochitest tests.
You can experiment with the manual tests we have been using by loading these two pages:
https://people.torproject.org/~brade/tests/bug-16300-container.html
https://pearlcrescent.com/tor/bug-16300/bug-16300-container.html
Each one loads an iframe with src=https://pearlcrescent.com/tor/bug-16300/bug-16300.html which contains some fairly self explanatory buttons (and of course you can look at the JS code to see what it does).

Because our patch only checks privacy.thirdparty.isolate at the time a broadcast channel is created, you will need to reload our test pages after changing that pref.

comment:9 in reply to:  5 ; Changed 4 years ago by gk

Replying to mcs:

We did disallow use of Broadcast Channels from SharedWorkers when isolation is enabled because, as with blob URLs (#15502), there is no good way to get at the document or channel.

This is okay and won't hurt especially as we disable SharedWorkers (we have #15564 for the URL bar domain isolation). I am wondering though how they determine the origin and whether we have to update our ThirdPartyUtils for this then...

Might need another pair of eyes due to being a C++ patch but I am happy with it, thanks!

comment:10 in reply to:  9 Changed 4 years ago by mcs

Replying to gk:

Might need another pair of eyes due to being a C++ patch but I am happy with it, thanks!

Thank you. Kathy and I will wait for a second review.

comment:11 Changed 4 years ago by mikeperry

Status: needs_reviewneeds_revision

It seems like GetFirstPartyHost() can fail to get an isolation host in InitializeRunnable::MainThreadRun() in dom/broadcastchannel/BroadcastChannel.cpp if there is no document yet in the Worker. Doesn't this mean that workers who can trigger this case can still broadcast to eachother even if they are launched from different isolation domains, because their empty isolation host strings will match?

I'm not completely clear on what is the best way to handle this case. Perhaps broadcast messages should fail if isolation is enabled and the isolation host is empty (and also if prefixed with "--NoFirstPartyHost-", for when the getFirstPartyHostForIsolation API itself fails)?

Last edited 4 years ago by mikeperry (previous) (diff)

comment:12 in reply to:  11 Changed 4 years ago by mcs

Replying to mikeperry:

It seems like GetFirstPartyHost() can fail to get an isolation host in InitializeRunnable::MainThreadRun() in dom/broadcastchannel/BroadcastChannel.cpp if there is no document yet in the Worker. Doesn't this mean that workers who can trigger this case can still broadcast to eachother even if they are launched from different isolation domains, because their empty isolation host strings will match?

It would mean that, except we tried hard to ensure that failure to obtain an isolation host leads to failure to create a BroadcastChannel. The code in BroadcastChannel::Constructor() fails if the InitializeRunnable throws an error (passed back and checked via the aRv parameter). The check does not show up in the patch because it is already present just after the runnable->Dispatch(cx); call, here:

http://mxr.mozilla.org/mozilla-central/source/dom/broadcastchannel/BroadcastChannel.cpp#481

And the GetFirstPartyHost() method that we added starts with a check for a non-NULL aDoc:

void
GetFirstPartyHost(nsIDocument* aDoc, nsAString& aFirstPartyHost,
                  ErrorResult& aRv)
{
  if (!aDoc) {
    aRv.Throw(NS_ERROR_FAILURE);
    return;
  }
...

So I think the patch is OK as is, but maybe I am missing something.

comment:13 Changed 4 years ago by mikeperry

Resolution: fixed
Status: needs_revisionclosed

Ok, so long as the runnable->Dispatch() in fact blocks (which I believe it should, due to syncLoop.Run()), then I think we're good here.

Merged for 5.0a3.

comment:14 in reply to:  13 Changed 4 years ago by mcs

Replying to mikeperry:

Ok, so long as the runnable->Dispatch() in fact blocks (which I believe it should, due to syncLoop.Run()), then I think we're good here.

Yes, it seems to block. But Kathy and I could not find much documentation in the source files or elsewhere for this stuff.

Note: See TracTickets for help on using tickets.