1421099 - (CVE-2018-5108) blob URLs do not respect OriginAttributes segregation

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

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 1

[Andrea Marchesini [:baku]]

Attachment: blobUrl_subsumes.patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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 3

[Andrea Marchesini [:baku]]

Attachment: blobUrl_subsumes.patch

Comment 4

[Andrea Marchesini [:baku]]

Attachment: blobUrl_subsumes.patch

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8932497 [details] [diff] [review]
blobUrl_subsumes.patch

As agreed on IRC, use ChromeUtils::IsOriginAttributesEqualIgnoringFPD

Comment 6

[Andrea Marchesini [:baku]]

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

comment applied

Comment 7

[Andrea Marchesini [:baku]]

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.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Andrea Marchesini [:baku]]

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 12

[Gerry Chang [:gchang]]

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

Fix a sec issue. Let's take it in Beta58.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/122e73935baa

Comment 14

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

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

Comment 15

[Anne (:annevk)]

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

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 17

[Anne (:annevk)]

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.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 19

[Andrea Marchesini [:baku]]

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

Comment 20

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

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?

Comment 21

[Andrew Sutherland [:asuth] (he/him)]

(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

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

(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

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

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 24

[Andrew Sutherland [:asuth] (he/him)]

Attachment: System principal does not need to match originAttributes

Comment 25

[Andrea Marchesini [:baku]]

Comment on attachment 8935094 [details] [diff] [review]
System principal does not need to match originAttributes

Review of attachment 8935094 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

(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.

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04af51f7fbdb9a171703e69a3d5651d0c344d5bb

Comment 28

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 29

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/860ae422d233

Comment 31

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

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

Comment 32

[Anne (:annevk)]

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 33

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

(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 35

[Julien Cristau [:jcristau] (back April 22)]

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.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/051581a5ece8

Comment 37

[Andrew Sutherland [:asuth] (he/him)]

[e-mail] 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.