Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#16429 closed enhancement (fixed)

Isolate mediasource and blob URLs in all contexts

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: ff38-esr, tbb-linkability, TorBrowserTeam201507R, tbb-5.0a4
Cc: arthuredelstein, brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In #15502 we isolated blob URLs and disabled them in Worker contexts. This ticket is a follow-up to get the URL-bar-domain-based isolation implemented in Worker environments.

Child Tickets

Change History (16)

comment:1 Changed 4 years ago by mcs

Cc: brade mcs added

comment:2 Changed 4 years ago by arthuredelstein

Please see the follow branch for review:

https://github.com/arthuredelstein/tor-browser/commits/16429+7

It contains several related patches:

  1. A fixup patch that gets the blob URI regression tests working after they broke in the FF31->FF38 transition (fixes #16416).
  2. A "revert" patch that removes the old blob URI isolation patch (that disabled blob URIs in web workers).
  3. A new version of the blob URI patch that isolates blob URIs in workers without disabling them (#16429). The new patch also fixes mediasource (and mediastream) URIs (#15703).
  4. A second fixup patch for the blob URI regression tests that makes sure they test blob URI isolation in Web workers (#16429).
  5. A new patch that tests mediasource URI isolation (#15703).

comment:3 Changed 4 years ago by arthuredelstein

Keywords: TorBrowserTeam201507R added

comment:4 Changed 4 years ago by arthuredelstein

Status: newneeds_review

comment:5 Changed 4 years ago by arthuredelstein

I just confirmed that, if I revert the (new) #15703 patch, that the regression tests for both blob and mediasource URIs fail. (This is the desired behavior.)

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

comment:6 Changed 4 years ago by mikeperry

Some issues:

  1. In URL::RevokeObjectURL(), you can have a null pointer deref if window is null, since you try to obtain the document before checking window.
  2. We probably should have some error checking in your GetFirstPartyHost() functions, if nothing else to differentiate between the case where isolation is disabled (NS_OK, but empty isolation URI) and failure (NS_FAILED(rv), with empty isolation URI). The returned host should probably also represent these two cases as different special string keys?
  3. I found a meta-bug (https://bugzilla.mozilla.org/show_bug.cgi?id=60697) where Mozilla was trying to remove static global uses of nsCOMPtr. I'm wondering if this is limited to static initialization or if we should be using something other than nsCOMPtr for gThirdPartyUtilService. The bug has been dormant for years, though...

comment:7 in reply to:  6 Changed 4 years ago by arthuredelstein

Replying to mikeperry:

Some issues:

  1. In URL::RevokeObjectURL(), you can have a null pointer deref if window is null, since you try to obtain the document before checking window.
  2. We probably should have some error checking in your GetFirstPartyHost() functions, if nothing else to differentiate between the case where isolation is disabled (NS_OK, but empty isolation URI) and failure (NS_FAILED(rv), with empty isolation URI). The returned host should probably also represent these two cases as different special string keys?
  3. I found a meta-bug (https://bugzilla.mozilla.org/show_bug.cgi?id=60697) where Mozilla was trying to remove static global uses of nsCOMPtr. I'm wondering if this is limited to static initialization or if we should be using something other than nsCOMPtr for gThirdPartyUtilService. The bug has been dormant for years, though...

These are all good points. I've attempted to fix them. The new version is here:
https://github.com/arthuredelstein/tor-browser/commits/16429+8

comment:8 Changed 4 years ago by mikeperry

The new branch 16429+8 looks OK to me. I would like a second opinion from mcs+brade before merge, though.

comment:9 in reply to:  8 Changed 4 years ago by arthuredelstein

Replying to mikeperry:

The new branch 16429+8 looks OK to me. I would like a second opinion from mcs+brade before merge, though.

A patch I proposed for Mozilla Bug 1078657 was recently accepted in mozilla-central. This patch adds a file, SpanwTask.js, that makes it simple to use coroutines in mochitests. So here's a new version of the 16429 branch, with the patch backported and the regression tests refactored to make use of SpawnTask.js. Otherwise everything in this branch is the same.

https://github.com/arthuredelstein/tor-browser/commits/16429+9

comment:10 Changed 4 years ago by mikeperry

Summary: Isolate blob URLs in Worker contextsIsolate mediasource and blob URLs in all contexts

Generalizing the ticket title so we can dup #15703 here.

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

comment:11 Changed 4 years ago by mikeperry

Keywords: tbb-5.0a4 added

comment:12 Changed 4 years ago by mcs

Kathy and I reviewed all of these changes and they look OK to us. Just a few comments:

  • Are there cases where the isolation key will be "--ISOLATION-FAILED-" when it would be better to just fail? For example, we could fail the creation of a blob URL if we cannot get an isolation key.
  • A nit: replace "isolationKey" with "aIsolationKey" in the nsHostObjectProtocolHandler::AddDataEntry() parameter list.
  • The messageSent variable is not really used inside dom/base/test/bug15703_page_retrieve.html; you could just remove it.

comment:13 Changed 4 years ago by mikeperry

Arthur: Do you think you will be able to address these comments soon? I particularly like the idea of failing a blob request when we can't get an isolation key.

comment:14 in reply to:  12 Changed 4 years ago by arthuredelstein

Replying to mcs:

Kathy and I reviewed all of these changes and they look OK to us. Just a few comments:

  • Are there cases where the isolation key will be "--ISOLATION-FAILED-" when it would be better to just fail? For example, we could fail the creation of a blob URL if we cannot get an isolation key.
  • A nit: replace "isolationKey" with "aIsolationKey" in the nsHostObjectProtocolHandler::AddDataEntry() parameter list.
  • The messageSent variable is not really used inside dom/base/test/bug15703_page_retrieve.html; you could just remove it.

Thanks for the review and these helpful suggestions. I've made changes to address each of them. Here's the new branch:

https://github.com/arthuredelstein/tor-browser/commits/16429+11

comment:15 Changed 4 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

Ok, I took a look at these and they seem good, thanks! Merged for 5.0a4.

Out of curiosity, I also noticed you added preliminary isolation for fetch? Was that to test #16326, or do you believe you've solved the odd caching problems there?

comment:16 in reply to:  15 Changed 4 years ago by arthuredelstein

Replying to mikeperry:

Ok, I took a look at these and they seem good, thanks! Merged for 5.0a4.

Out of curiosity, I also noticed you added preliminary isolation for fetch? Was that to test #16326, or do you believe you've solved the odd caching problems there?

This was intended to protect against using Fetch with a blob URI. But I think we then realized that Fetch is not active until FF39. So it will hopefully serve as a reminder when we rebase to FF45-ESR.

Note: See TracTickets for help on using tickets.