Closed
Bug 1421099
(CVE-2018-5108)
Opened 7 years ago
Closed 7 years ago
blob URLs do not respect OriginAttributes segregation
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: baku, Assigned: baku)
References
Details
(Keywords: sec-moderate, Whiteboard: [adv-main58+][post-critsmash-triage])
Attachments
(2 files, 2 obsolete files)
1.18 KB,
patch
|
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.69 KB,
patch
|
baku
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Assignee: nobody → amarchesini
Attachment #8932332 -
Flags: review?(bugs)
Comment 2•7 years ago
|
||
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•7 years ago
|
||
Attachment #8932332 -
Attachment is obsolete: true
Attachment #8932497 -
Flags: review?(bugs)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8932497 -
Attachment is obsolete: true
Attachment #8932497 -
Flags: review?(bugs)
Attachment #8932504 -
Flags: review?(bugs)
Comment 5•7 years ago
|
||
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•7 years ago
|
||
Comment on attachment 8932504 [details] [diff] [review]
blobUrl_subsumes.patch
comment applied
Attachment #8932504 -
Flags: review?(bugs)
Assignee | ||
Comment 7•7 years 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.
Comment 8•7 years ago
|
||
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
Flags: needinfo?(amarchesini)
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e989b32e54030e39ec6a603d884d3d614fd6e1
https://hg.mozilla.org/mozilla-central/rev/b2e989b32e54
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(amarchesini)
Comment 10•7 years ago
|
||
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•7 years 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?
Updated•7 years ago
|
Group: dom-core-security → core-security-release
Comment 12•7 years ago
|
||
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+
Comment 13•7 years ago
|
||
uplift |
Comment 14•7 years ago
|
||
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•7 years ago
|
||
Can you only message strings? And it's too expensive to just message the data: URL?
Comment 16•7 years ago
|
||
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•7 years 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.
Comment 18•7 years ago
|
||
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•7 years ago
|
||
jhirsch, do you think you can change the use of Blob as described in comment 17?
Flags: needinfo?(amarchesini) → needinfo?(jhirsch)
Comment 20•7 years ago
|
||
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)
Comment 21•7 years ago
|
||
(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•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
Attachment #8935094 -
Flags: review?(amarchesini)
Updated•7 years ago
|
Assignee: amarchesini → bugmail
Assignee | ||
Comment 25•7 years 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+
Comment 26•7 years ago
|
||
(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
Comment 27•7 years ago
|
||
Comment 28•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/860ae422d2338eb0d8fbc88c381cb5bbbb679270
Bug 1421099 - System principal does not need to match originAttributes. r=baku
Comment 29•7 years ago
|
||
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•7 years ago
|
||
Comment 31•7 years ago
|
||
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•7 years 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 33•7 years ago
|
||
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?
Comment 34•7 years ago
|
||
(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•7 years ago
|
||
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+
Updated•7 years ago
|
Comment 36•7 years ago
|
||
uplift |
Comment 37•7 years ago
|
||
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.
Updated•7 years ago
|
Whiteboard: [adv-main58+]
Updated•7 years ago
|
Alias: CVE-2018-5108
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•