STR: 1. in a normal tab, open a page with this code: var b = new Blob(["<script>var a = new BroadcastChannel('b'); a.onmessage = e => { alert(e.data); }</" + "script>"], { type: 'text/html' }); var u = URL.createObjectURL(b); console.log(u); 2. open a private browsing window with the blobURL shown in the web console. 3. in the web console for the normal tab do: var a = new BroadcastChannel('b'); a.postMessage(42); Expected result: First of all, a blob URL generated on a normal tab should not be accessible from a private browsing tab. Additionally, it should not be able to receive messages from the normal tab. Note that this is not a bug in BroadcastChannel, but it's related on how we treat blob URLs. This test works on any OriginAttributes segregation: containers, first party isolation and so on. We should probably revisit the idea of keeping the principal together with a blob URL, via nsIURIWthPrincipal.

Comment on attachment 8932332 [details] [diff] [review] blobUrl_subsumes.patch Doesn't this break sensing blob urls across cross-domain windows and loading urls in non-original window? Please test, and perhaps test also some other browser too. But I assume that just works atm.

Comment on attachment 8932497 [details] [diff] [review] blobUrl_subsumes.patch As agreed on IRC, use ChromeUtils::IsOriginAttributesEqualIgnoringFPD

Comment on attachment 8932504 [details] [diff] [review] blobUrl_subsumes.patch comment applied

This bug should me marked as sec-moderate. Patch landing because this issue can be reproduced only if the user users a blobURL as top-level URL manually.

Backed out for build bustages on nsHostObjectProtocolHandler.cpp: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd365ce7e6b2a1ea7b053bc2c249201b7127cfa5 Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ca1502df901c61e175634b15c8f53c7f4069f9b7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=148195584&repo=mozilla-inbound dom/file/nsHostObjectProtocolHandler.cpp:839:72: error: macro "MOZ_ASSERT_IF" passed 3 arguments, but takes just 2

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e989b32e54030e39ec6a603d884d3d614fd6e1 https://hg.mozilla.org/mozilla-central/rev/b2e989b32e54

Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.

Comment on attachment 8932504 [details] [diff] [review] blobUrl_subsumes.patch Approval Request Comment [Feature/Bug causing the regression]: blob URLs [User impact if declined]: a blob URL can be opened, manually, on a private browsing session. [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: manual test can be done following the STR. [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: low [Why is the change risky/not risky?]: The patch introduces a check on the originAttributes of the blob and the one from the loadInfo. They must always be in sync. [String changes made/needed]: none

Comment on attachment 8932504 [details] [diff] [review] blobUrl_subsumes.patch Fix a sec issue. Let's take it in Beta58.

This patch breaks a feature in Firefox Screenshots: screenshots can no longer be downloaded in 'never remember history' mode[1]. The Screenshots implementation involves generating a blob from a canvas in content[2], sending the blob to the webextension background page[3], then triggering a download from the background page[4]. This patch (deliberately) breaks the messaging step. Is there any way we could restore the Screenshots download feature when history is disabled? We'd ideally like to get this bug fixed and uplifted to 58 this week. [1] https://github.com/mozilla-services/screenshots/issues/3886 [2] https://github.com/mozilla-services/screenshots/blob/41e27fd2567743f7067fd3d3649df8203f05f978/addon/webextension/selector/shooter.js#L168 [3] https://github.com/mozilla-services/screenshots/blob/58c82fb9fa151f7db4a094fe9bd5eb85741a7acd/addon/webextension/selector/ui.js#L934 [4] https://github.com/mozilla-services/screenshots/blob/58c82fb9fa151f7db4a094fe9bd5eb85741a7acd/addon/webextension/background/main.js#L226

Can you only message strings? And it's too expensive to just message the data: URL?

Elaborating on Anne's comment, I presume the problem is that https://github.com/mozilla-services/screenshots/blob/713f2e3fbeefcaaecba634515184d00029c69d89/addon/webextension/blobConverters.js#L11 does new Blob() in a content script, and the Blob is coming from the content window global. I think we want to add "Blob" and "File" to the wantGlobalProperties of the extension content script's sandbox at https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/toolkit/components/extensions/ExtensionContent.jsm#451 but this probably also wants webextensions to add a bunch more tests.

If you can message the actual Blob object and only create a URL from it at the point of download, there should be no problem. Blob objects by themselves don't carry an origin (principal if you will), those only get associated with blob: URLs.

Ah, whoops, right. Then the mis-match here probably is the situation where "Never Remember History" is really "always use private browsing mode" which means that the extension's background page principal will end up having an mPrivateBrowsingId > 0 but the download is using the system principal[1] and the system principal has mPrivateBrowsingId == 0. And so the new error case will fire because IsOriginAttributesEqualIgnoringFPD will fail because it doesn't special case for the system principal. Or at least I think we haven't fixed the extension background page private browsing id issue... 1: https://searchfox.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#1912

jhirsch, do you think you can change the use of Blob as described in comment 17?

Yes, thanks for the responses. I'll experiment with actually creating the URL inside the background window. It seems odd to me that downloads *do* currently work in private browsing mode. Might it make sense to set the incognito flag to true on all windows in 'never remember history' mode?

(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #20) > Yes, thanks for the responses. I'll experiment with actually creating the > URL inside the background window. As I read https://github.com/mozilla-services/screenshots/blob/58c82fb9fa151f7db4a094fe9bd5eb85741a7acd/addon/webextension/background/main.js#L227, you're already doing that and this is a function of the originattributes for the background window having an mPrivateBrowsingId > 0. > It seems odd to me that downloads *do* currently work in private browsing > mode. What type of download are you referring to? Normal file downloads are handled by the nsExternalHelperAppService[1] creating a DownloadLegacyTransfer[2] by way of it implementing nsITransfer[3]. In that case the channel is already opened by the uriloader which "diverts" the channel to the external helper app stuff in the parent. Compare with the screenshot extension which uses the Downloads.jsm API via the webext downloads API to explicitly trigger a download which I believe uses the DownloadCopySaver[4] which takes on the responsibility of opening the channel and decides to do it with the system principal, rather than the relevant docshell (which would inherently be associated with the principal that minted the URL via createObjectUrl). 1: https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/uriloader/exthandler/nsExternalHelperAppService.cpp#2142 2: https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/toolkit/components/jsdownloads/src/DownloadLegacy.js#24 3: https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/toolkit/components/jsdownloads/src/Downloads.manifest#2 4: https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/toolkit/components/jsdownloads/src/DownloadCore.jsm#1779

(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #20) > Might it make sense to set the incognito flag to true on all windows in > 'never remember history' mode? As I understand it, this is already happening and most of our problem[1]. Unless it's been corrected, I believe the webextensions' background windows end up with private browsing flags set on their docshell. Okay, and yeah, I just ran a nightly build under linux with "never remember history" and attached gdb and it agrees with me[1]. Also if I set a breakpoint at https://hg.mozilla.org/mozilla-central/rev/b2e989b32e54#l1.58 we see it's a private browsing flag mismatch[2] 1: (gdb) pp sTabChildren TabChildMap 0x7f9403bd9af0 6: 4294967299 6: mozilla::dom::TabChild 0x7f93fe039800 mWebNav: nsWebBrowser 0x7f93fe3f8970 mOriginAttributes: mPrivateBrowsingId: 1 mUserContextId: 0 mDocShell: nsDocShell 0x7f93fe03a800 desc: mCurrentURI: mozilla::net::SubstitutingURL 0x7f93fe590d00 mSpec: 0x7f93fe581a0c "moz-extension://14d4c303-26b6-458d-8b0f-4c78e3aed76f/_generated_background_page.html" mPrivateBrowsingId: 1 mOriginAttributes: mPrivateBrowsingId: 1 mUserContextId: 0 mTitle: 0x7f940e888be4 <gNullChar> u"" load: mLoadGroup: mozilla::net::nsLoadGroup 0x7f93fe26bb00 mLoadFlags: LOAD_DOCUMENT_NEEDS_COOKIE 0x4 2: Thread 1 "firefox" hit Breakpoint 1, nsHostObjectProtocolHandler::NewChannel2 (this=this@entry=0x7f759ad32a40, uri=uri@entry=0x7f7583b9fc40, aLoadInfo=aLoadInfo@entry=0x7f75656f8420, result=0x7ffdb6877eb0) at /home/visbrero/rev_control/hg/mozilla-unified/dom/file/nsHostObjectProtocolHandler.cpp:953 953 !ChromeUtils::IsOriginAttributesEqualIgnoringFPD(aLoadInfo->GetOriginAttributes(), (gdb) pp aLoadInfo mozilla::net::LoadInfo 0x7f75656f8420 mLoadingPrincipal: No mapping or pretty-printer for SystemPrincipal, switching to gdb print. ... mInIsolatedMozBrowser = false, mPrivateBrowsingId = 0, mUserContextId = 0 ... gdb) pp principal ContentPrincipal 0x7f75671dbd30 mCodebase: mozilla::net::SubstitutingURL 0x7f7571e4bd00 mSpec: 0x7f756bb32d8c "moz-extension://14d4c303-26b6-458d-8b0f-4c78e3aed76f/_generated_background_page.html" mOriginAttributes: mPrivateBrowsingId: 1 mUserContextId: 0

And to propose a short-term fix, we probably want to avoid the originattributes equality check if the aLoadInfo's principal is the system principal. I'll quickly create a patch and test it and ask baku to review/handle from there.

Comment on attachment 8935094 [details] [diff] [review] System principal does not need to match originAttributes Review of attachment 8935094 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense.

(fixing assignee... I keep forgetting bzexport does that and it doesn't honor my .hgrc) We should probably add an explicit test that covers the downloads API, possibly as exposed by the webextensions API, downloading a blob url minted in an origin that will not match up with the system principal. This would likely take the form of a test where we use a content page that's in a container or private browsing mode, have it perform a createObjectUrl, and have the webextension pass that URL to the downloads API. Unless we think the webextensions API should explicitly forbid that use-case.

https://hg.mozilla.org/integration/mozilla-inbound/rev/860ae422d2338eb0d8fbc88c381cb5bbbb679270 Bug 1421099 - System principal does not need to match originAttributes. r=baku

sec-wise, I landed the fix per discussion with baku and the clean try run, and consistent with the decision in comment 7 to land the original fix which this regression fix amends.

This patch fixes the bug in Screenshots. Is there good reason to still update Screenshots to pass around a blob, not a blob URL?

I'd say it's generally cleaner to pass around objects. Especially since if you pass on a string you don't know when to revoke it (basically manual memory management).

Comment on attachment 8935094 [details] [diff] [review] System principal does not need to match originAttributes Approval Request Comment [Feature/Bug causing the regression]: This bug, the patch already uplifted to beta. [User impact if declined]: The webext Downloads API will fail to download URLs created via createObjectURL() that originate in a page context whose originattributes aren't the same as those used by the system principal. This means private browsing pages and user container pages. Because of weird fallout from the "always use private browsing"/"never remember history" preference surfaced in our preferences UI, the built-in Firefox Screenshots extension thing breaks when that is enabled. It's also likely other webexts can break even without that preference enabled if they are using createObjectURL from within a content script. [Is this code covered by automated tests?]: Not for this specific edge case, but we have test coverage that ensures the world won't break. [Has the fix been verified in Nightly?]: By me, locally. The fix has been in nightly since the 20171207220423 build. [Needs manual test from QE? If yes, steps to reproduce]: This was found by QE in https://github.com/mozilla-services/screenshots/issues/3886 and probably wants QE to re-verify. I'm going to comment on that issue again to ask for a verification. [List of other uplifts needed for the feature/fix]: No. [Is the change risky?]: No. [Why is the change risky/not risky?]: It's just a system principal check and the check explicitly handles edge-cases to a null principal being provided. [String changes made/needed]: None.

(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #31) > This patch fixes the bug in Screenshots. Is there good reason to still > update Screenshots to pass around a blob, not a blob URL? For clarity, I believe a data URL is being passed around, if we're talking about this code path: https://github.com/mozilla-services/screenshots/blob/41e27fd2567743f7067fd3d3649df8203f05f978/addon/webextension/selector/shooter.js#L194 And here's where it goes from data URL to Blob and then Blob URL that gets downloaded: https://github.com/mozilla-services/screenshots/blob/58c82fb9fa151f7db4a094fe9bd5eb85741a7acd/addon/webextension/background/main.js#L226,L241 aside: I was confused about this previously and thought the Blob was being directly passed because I confused the "takeShot" code path, which does send a Blob, with the "downloadShot" path, which does not, and is the actual thing we're talking about. Double-aside: The use of data URLs instead of blobs is understandable given that IndexedDB is inoperable for webexts for the "never remember history" snafu we're dealing with here. But it is sad, Blobs rock! :)

Comment on attachment 8935094 [details] [diff] [review] System principal does not need to match originAttributes might have been cleaner to do the followup in a separate bug, but let's get this new patch in beta so things are consistent.

cristina.badescu@softvision.ro re-tested the nightly and confirmed it's fixed and is waiting for a new beta-spin to re-test the beta uplift, cc'ing.