Opened 3 years ago

Closed 3 years ago

#18811 closed defect (fixed)

Our first-party isolation patch incorrectly rejects blobs retrieved in workers

Reported by: arthuredelstein Owned by: arthuredelstein
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff45-esr, TorBrowserTeam201605R, tbb-6.0-must
Cc: gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When isolation is enabled, blobs retrieved by an XHR inside a worker are rejected even when the blob's first party matches the worker's first party. I found that the regression was caused by this Mozilla patch:
https://hg.mozilla.org/mozilla-central/diff/12a852867c16/dom/base/nsXMLHttpRequest.cpp#l1694

Because of the Mozilla patch, when we are in a worker, NS_NewChannel is no longer passed a document, so our patch code in nsHostObjectProtocolHandler::NewChannel2 is not able to obtain the correct first party. Therefore the blob URI is rejected even if the first party of the worker matches. I haven't yet figured out how to fix this problem.

Child Tickets

Change History (10)

comment:1 Changed 3 years ago by gk

Cc: gk added

comment:2 Changed 3 years ago by gk

Parent ID: #15197

We should fix this but that is not necessary for switching our nightlies to ESR45.

comment:3 Changed 3 years ago by gk

Keywords: tbb-6.0a5 removed

comment:4 Changed 3 years ago by gk

Keywords: TorBrowserTeam201605 added; TorBrowserTeam201604 removed

Moving tickets

comment:5 Changed 3 years ago by gk

Owner: changed from tbb-team to arthuredelstein
Status: newassigned

comment:6 Changed 3 years ago by gk

Keywords: tbb-6.0-must added

comment:7 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201605R added; TorBrowserTeam201605 removed
Status: assignedneeds_review

Here's a patch to fix the problem.

https://github.com/arthuredelstein/tor-browser/commit/18811+3
94fa4b050a1252914c57e59c747d4c9342cdf2cb

Unfortunately I had to resort to making the blob URL a special case to undo the Tor Browser regression caused by the Mozilla patch mentioned in the description. I considered various alternatives but didn't find a better solution -- at least the change here is localized. Mozilla is working on blob isolation using origin attributes, so hopefully a cleaner solution will emerge from that approach.

The problem reported in this ticket was originally revealed by two failing tests in
./mach mochitest dom/base/test/test_tor_bug15502.html

After this patch is applied, the tests all pass.

comment:8 Changed 3 years ago by gk

Status: needs_reviewneeds_information

Could you elaborate whey we don't care about CSP just for blob: URLs?

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

Replying to gk:

Could you elaborate whey we don't care about CSP just for blob: URLs?

blob: URLs result in pure JavaScript data that don't result in further content being loaded from the network. So I don't think CSP is needed at this stage in the blob loading process. I also looked downstream of the function I am patching here, and there is apparently no access to CSP settings.

But it's possible I am missing something here. Is there any reason why a blob would need an associated CSP?

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

comment:10 Changed 3 years ago by gk

Resolution: fixed
Status: needs_informationclosed

Jonas' mail seems to indicate we are not in a bad shape with this patch. We should try to figure this out, though, with his help if possible. Taking this patch for 6.0 and we'll back it out and rebuild if we indeed overlooked things. This is fixed on tor-browser-45.1.0esr-6.0-1 (commit 464f9221b09b70e05950a44ebb4f3c1f0bfde179).

Note: See TracTickets for help on using tickets.