Bug 1421099 (CVE-2018-5108)

blob URLs do not respect OriginAttributes segregation

RESOLVED FIXED in Firefox 58

Status

()

--
major
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Depends on: 1 bug, {sec-moderate})

unspecified
mozilla59
sec-moderate
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox57 wontfix, firefox58 fixed, firefox59 fixed)

Details

(Whiteboard: [adv-main58+][post-critsmash-triage])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8932332 [details] [diff] [review]
blobUrl_subsumes.patch
Assignee: nobody → amarchesini
Attachment #8932332 - Flags: review?(bugs)
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.
Attachment #8932332 - Flags: review?(bugs) → review-
(Assignee)

Comment 3

a year ago
Created attachment 8932497 [details] [diff] [review]
blobUrl_subsumes.patch
Attachment #8932332 - Attachment is obsolete: true
Attachment #8932497 - Flags: review?(bugs)
(Assignee)

Comment 4

a year ago
Created attachment 8932504 [details] [diff] [review]
blobUrl_subsumes.patch
Attachment #8932497 - Attachment is obsolete: true
Attachment #8932497 - Flags: review?(bugs)
Attachment #8932504 - Flags: review?(bugs)
Comment on attachment 8932497 [details] [diff] [review]
blobUrl_subsumes.patch

As agreed on IRC, use ChromeUtils::IsOriginAttributesEqualIgnoringFPD
Attachment #8932497 - Flags: review?(bugs) → review+
(Assignee)

Comment 6

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

comment applied
Attachment #8932504 - Flags: review?(bugs)
(Assignee)

Comment 7

a year ago
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e989b32e54030e39ec6a603d884d3d614fd6e1
https://hg.mozilla.org/mozilla-central/rev/b2e989b32e54
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(Assignee)

Updated

a year ago
Flags: needinfo?(amarchesini)
Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.
status-firefox57: --- → wontfix
status-firefox58: --- → affected
status-firefox-esr52: --- → wontfix
Flags: needinfo?(amarchesini)
Keywords: sec-moderate
(Assignee)

Comment 11

a year ago
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
Flags: needinfo?(amarchesini)
Attachment #8932504 - Flags: approval-mozilla-beta?
Group: dom-core-security → core-security-release
Comment on attachment 8932504 [details] [diff] [review]
blobUrl_subsumes.patch

Fix a sec issue. Let's take it in Beta58.
Attachment #8932504 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1419605
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
Flags: needinfo?(amarchesini)

Comment 15

a year ago
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.

Comment 17

a year ago
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
(Assignee)

Comment 19

a year ago
jhirsch, do you think you can change the use of Blob as described in comment 17?
Flags: needinfo?(amarchesini) → needinfo?(jhirsch)
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?
Flags: needinfo?(jhirsch)
(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.
Created attachment 8935094 [details] [diff] [review]
System principal does not need to match originAttributes
Attachment #8935094 - Flags: review?(amarchesini)
Assignee: amarchesini → bugmail
(Assignee)

Comment 25

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

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

Makes sense.
Attachment #8935094 - Flags: review?(amarchesini) → review+
(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.
Assignee: bugmail → amarchesini
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?

Comment 32

a year ago
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.
Attachment #8935094 - Flags: approval-mozilla-beta?
(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.
Attachment #8935094 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox58: fixed → affected
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.
Whiteboard: [adv-main58+]
Alias: CVE-2018-5108
Flags: qe-verify-
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.