Closed
Bug 1398229
Opened 7 years ago
Closed 7 years ago
"Save Link As..." on a link that requires auth doesn't work the same in a container tab
Categories
(Firefox :: File Handling, defect, P1)
Firefox
File Handling
Tracking
()
RESOLVED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | wontfix |
firefox58 | --- | wontfix |
firefox59 | --- | fixed |
People
(Reporter: ted, Assigned: baku)
References
(Blocks 1 open bug)
Details
(Keywords: privacy, sec-other, Whiteboard: [OA][usercontextId][post-critsmash-triage][adv-main59-])
Attachments
(3 files, 5 obsolete files)
6.94 KB,
patch
|
ckerschb
:
review+
gchang
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
6.80 KB,
patch
|
Details | Diff | Splinter Review |
I noticed this on crash-stats.mozilla.com when trying to download minidumps. Getting the link to a raw minidump requires you to log in (and have specific permissions), and then it presents you a link. Clicking on the link works fine, but I like to "Save Link As..." to save them in a specific place (which works really well because Firefox remembers the last directory I chose per-site). This wasn't working properly for me recently. The "Save As" dialog would show the content-type as text/html, and give me a junk .html filename. I thought maybe it was a regression from e10s, but a little more testing showed that it's because I've been opening crash-stats links in container tabs recently (to keep my work Google login separate from my personal one).
Looking at the network requests in the browser console, what seems to be happening is that we do some sort of preflight network request when you click "Save Link As...", presumably to get the filename and content-type, but in this scenario that request doesn't have my session cookie for crash-stats and so crash-stats sends me a 302 redirect to the login page (which is text/html). I assume the network request is happening outside of the container context.
Comment 1•7 years ago
|
||
I'm going to restrict access to this whilst we audit this just because it might be a contextual identity escape that impacts PB mode too.
:baku if you have any cycles (likely no one does at the moment though), this would be good to look at I think.
Blocks: ContextualIdentity
Group: firefox-core-security
Flags: needinfo?(amarchesini)
Keywords: sec-high
Updated•7 years ago
|
Updated•7 years ago
|
status-firefox57:
--- → affected
Assignee | ||
Comment 2•7 years ago
|
||
We are currently using system principal. No OriginAttributes, no correct principal involved.
See https://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1146-1155
ckerschb, you have been working on bug bug 1195555 and bug 1136055. Any thought?
Updated•7 years ago
|
Flags: needinfo?(tanvi)
Comment 3•7 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #2)
> ckerschb, you have been working on bug bug 1195555 and bug 1136055. Any
> thought?
Mhm, the reason we used SystemPrincipal is to bypass some mixed content restrictions. What we could do (though ugly) is to use TYPE_DOCUMENT to bypass mixed content and also take the OA of this.principal, but create a new codebasePrincipal from the URI we are about to download:
let OA = this.principal.originAttributes;
let principal =
Services.scriptSecurityManager.createCodebasePrincipal(makeURI(linkURL), OA);
I know it's not great, but that should work. Open for any discussion, this is just one potential way forward.
What do you think?
Flags: needinfo?(ckerschb)
Comment 4•7 years ago
|
||
What is the current content type?
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(ckerschb)
Comment 5•7 years ago
|
||
(In reply to Tanvi Vyas[:tanvi] from comment #4)
> What is the current content type?
It's TYPE_OTHER, see:
https://hg.mozilla.org/releases/mozilla-release/rev/3f64bf12d5f8#l1.28
Flags: needinfo?(ckerschb)
Comment 6•7 years ago
|
||
Tanvi, this is still marked tracking for FF57. Do we need to fix it or can we remove the tracking flag?
Comment 7•7 years ago
|
||
Ted, can you check if this is also an issue in regular and private mode?
Flags: needinfo?(tanvi) → needinfo?(ted)
Comment 8•7 years ago
|
||
Given how close we are to 57 launch, I think we have to wait for 58 on this one.
Comment 9•7 years ago
|
||
baku, can you take this bug? Probably the fix is using TYPE_DOCUMENT instead of TYPE_OTHER and using this.principal - https://hg.mozilla.org/releases/mozilla-release/rev/3f64bf12d5f8#l1.28
Flags: needinfo?(amarchesini)
Priority: -- → P1
Reporter | ||
Comment 10•7 years ago
|
||
(In reply to Tanvi Vyas[:tanvi] from comment #7)
> Ted, can you check if this is also an issue in regular and private mode?
What would you like me to test exactly? Logging in to the site in regular mode and then trying to save the link from private mode?
Flags: needinfo?(ted)
Comment 11•7 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> (In reply to Tanvi Vyas[:tanvi] from comment #7)
> > Ted, can you check if this is also an issue in regular and private mode?
>
> What would you like me to test exactly? Logging in to the site in regular
> mode and then trying to save the link from private mode?
Yes, exactly. And vice versa.
Flags: needinfo?(ted)
Updated•7 years ago
|
Whiteboard: [OA][usercontextId]
Assignee | ||
Comment 12•7 years ago
|
||
This is the test. Now we have to write the fix :)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8923876 -
Attachment is obsolete: true
Attachment #8926985 -
Flags: review?(ckerschb)
Assignee | ||
Comment 14•7 years ago
|
||
Note that I'm still using TYPE_OTHER. Using TYPE_DOCUMENT breaks LoadInfo, which cannot be created from JS, since bug 1105556.
Attachment #8926987 -
Flags: review?(ckerschb)
Comment 15•7 years ago
|
||
Comment on attachment 8926987 [details] [diff] [review]
part 2 - save-as
Review of attachment 8926987 [details] [diff] [review]:
-----------------------------------------------------------------
Baku, this seems like the wrong fix. Using TYPE_OTHER will be blocked as mixed content and now we are back to why we needed to fix Bug 1136055 in the first place (see also comment 3). But I see your problem, we can't create a channel with TYPE_DOCUMENT, because we have that special constructor for loadInfo with TYPE_DOCUMENT which we only allow from docshell - we also shouldn't change that. We could add a new contentPolicyType TYPE_DOWNLOAD or something and return early within the mixed content blocker? I don't know of other ways to bypass mixed content blocker (besides using systemPrincipal). Looking at nsMixedContentBlocker.cpp, it seems that only TYPE_WEBSOCKET gets a free pass at the moment (because it can't be created from within insecure context).
Tanvi, any other suggestions on how to bypass mixed content blocker? Using TYPE_MEDIA for example would allow the load but would update the UI, right? Maybe that is desireable in the end and we don't even want to return early which we would for TYPE_DOCUMENT, but rather have an indicator that something was loaded using an insecure connection.
I think I am for adding a new contentType, TYPE_DOWNLOAD which is treated as eMixedDisplay.
Attachment #8926987 -
Flags: review?(ckerschb) → review-
Assignee | ||
Comment 16•7 years ago
|
||
Note that my mochitest implements a redirect and it passes correctly with this patch applied.
Flags: needinfo?(ckerschb)
Comment 17•7 years ago
|
||
Comment on attachment 8926985 [details] [diff] [review]
part 1 - mochitest
Review of attachment 8926985 [details] [diff] [review]:
-----------------------------------------------------------------
Holy Moly, what an effort to write a test - thanks for doing that. Please address my questions and flag me again for review.
::: browser/components/contextualidentity/test/browser/browser_saveLink.js
@@ +1,5 @@
> +"use strict";
> +
> +const BASE_ORIGIN = "http://example.com";
> +const URI = BASE_ORIGIN +
> + "/browser/browser/components/contextualidentity/test/browser/saveLink.sjs";
A more robust way of writing this could be:
const kTestPath = getRootDirectory(gTestPath)
.replace("chrome://mochitests/content", "http://example.com")
const kTestURI = kTestPath + "saveLink.sjs";
@@ +10,5 @@
> +add_task(async function setup() {
> + // make sure userContext is enabled.
> + await SpecialPowers.pushPrefEnv({"set": [
> + ["privacy.userContext.enabled", true]
> + ]});
I think you can just take that whole pushPrefEnv and place it at the beginning of add_task(async function test() {
@@ +21,5 @@
> + info("Let's open a new window");
> + let win = await BrowserTestUtils.openNewBrowserWindow();
> +
> + info("Opening tab with UCI: 1");
> + let tab = BrowserTestUtils.addTab(win.gBrowser, URI + "?UCI=1", {userContextId: 1});
Doesn't that end up creating a Systemprncipal within addTab() which in turn causes the this.principal within nsContextMenu.js to use that SystemPrincipal? I think we should pass a principal explicitly here, no?
Attachment #8926985 -
Flags: review?(ckerschb)
Comment 18•7 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #16)
> Note that my mochitest implements a redirect and it passes correctly with
> this patch applied.
Please see previous comment, I guess there is a flaw in the test.
Flags: needinfo?(ckerschb)
Assignee | ||
Comment 19•7 years ago
|
||
> Doesn't that end up creating a Systemprncipal within addTab() which in turn
> causes the this.principal within nsContextMenu.js to use that
> SystemPrincipal? I think we should pass a principal explicitly here, no?
No. The principal is related to what is loaded in the tab. The tab will run a ContentPrincipal with OriginAttributes.UCI = 1.
Note that SystemPrincipal has always UCI = 0.
Comment 20•7 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #19)
> No. The principal is related to what is loaded in the tab. The tab will run
> a ContentPrincipal with OriginAttributes.UCI = 1.
> Note that SystemPrincipal has always UCI = 0.
Are you sure? Have a look at:
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#1463
Assignee | ||
Comment 21•7 years ago
|
||
This is the triggering principal, not the loading principal.
Assignee | ||
Comment 22•7 years ago
|
||
Attachment #8926985 -
Attachment is obsolete: true
Attachment #8927770 -
Flags: review?(ckerschb)
Assignee | ||
Comment 23•7 years ago
|
||
Attachment #8926987 -
Attachment is obsolete: true
Attachment #8927771 -
Flags: review?(ckerschb)
Assignee | ||
Comment 24•7 years ago
|
||
Attachment #8927772 -
Flags: feedback?(ckerschb)
Comment 25•7 years ago
|
||
Comment on attachment 8927770 [details] [diff] [review]
part 1 - mochitest
Review of attachment 8927770 [details] [diff] [review]:
-----------------------------------------------------------------
This test (starting out with https and then redirecting to http) looks good to me now. thanks for updating.
Attachment #8927770 -
Flags: review?(ckerschb) → review+
Comment 26•7 years ago
|
||
Comment on attachment 8927771 [details] [diff] [review]
part 2 - browser
Review of attachment 8927771 [details] [diff] [review]:
-----------------------------------------------------------------
that also looks good.
Attachment #8927771 -
Flags: review?(ckerschb) → review+
Comment 27•7 years ago
|
||
Comment on attachment 8927772 [details] [diff] [review]
part 3 - TYPE_INTERNAL_DOWNLOAD
Review of attachment 8927772 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/security/nsMixedContentBlocker.cpp
@@ +496,5 @@
> + // Quick return for TYPE_INTERNAL_DOWNLOAD. This is always alloed.
> + if (aContentType == nsIContentPolicy::TYPE_INTERNAL_DOWNLOAD) {
> + *aDecision = ACCEPT;
> + return NS_OK;
> + }
I don't think we should give it a free pass, we should rather treat it the same as TYPE_MEDIA, so just add it here:
https://dxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#579
Attachment #8927772 -
Flags: feedback?(ckerschb)
Assignee | ||
Comment 28•7 years ago
|
||
Attachment #8927772 -
Attachment is obsolete: true
Attachment #8927853 -
Flags: review?(ckerschb)
Comment 29•7 years ago
|
||
Comment on attachment 8927853 [details] [diff] [review]
part 3 - TYPE_DOWNLOAD
Review of attachment 8927853 [details] [diff] [review]:
-----------------------------------------------------------------
Ok. Thanks for updating. Just for the record, initially we were considering passing TYPE_DOCUMENT. Using TYPE_DOWNLOAD seems like the best option now. thanks for fixing.
Attachment #8927853 -
Flags: review?(ckerschb) → review+
Comment 30•7 years ago
|
||
Comment on attachment 8927853 [details] [diff] [review]
part 3 - TYPE_DOWNLOAD
>diff --git a/dom/base/nsIContentPolicy.idl b/dom/base/nsIContentPolicy.idl
>--- a/dom/base/nsIContentPolicy.idl
>+++ b/dom/base/nsIContentPolicy.idl
>@@ -177,16 +177,21 @@ interface nsIContentPolicy : nsISupports
> const nsContentPolicyType TYPE_IMAGESET = 21;
>
> /**
> * Indicates a web manifest.
> */
> const nsContentPolicyType TYPE_WEB_MANIFEST = 22;
>
> /**
>+ * Indicates an save-as link download from the front-end code.
>+ */
>+ const nsContentPolicyType TYPE_DOWNLOAD = 43;
>+
Can we be more descriptive here? We aren't talking about all downloads, just this very specific save-link-as download. That should be clear in the content type. TYPE_SAVE-AS_DOWNLOAD, for example? And also should be detailed in the comments in nsMixedCotnetnBlocker.
Attachment #8927853 -
Flags: review-
Updated•7 years ago
|
Flags: needinfo?(tanvi)
Assignee | ||
Comment 31•7 years ago
|
||
I cannot use '-'. TYPE_SAVEAS_DOWNLOAD seems a good name to me. Alternatively: TYPE_SAVE_LINK_AS_DOWNLOAD.
Comment improved.
Attachment #8927853 -
Attachment is obsolete: true
Comment 32•7 years ago
|
||
This was pushed to inbound and then backed out for browser_saveLink.js leaks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b58d7dbfcf60e3dabbca270ff0c4c4d1b233edf0
https://treeherder.mozilla.org/logviewer.html#?job_id=145055417&repo=mozilla-inbound
Flags: needinfo?(amarchesini)
Comment 33•7 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #31)
> Created attachment 8928016 [details] [diff] [review]
> part 3 - TYPE_SAVEAS_DOWNLOAD
>
> I cannot use '-'. TYPE_SAVEAS_DOWNLOAD seems a good name to me.
> Alternatively: TYPE_SAVE_LINK_AS_DOWNLOAD.
> Comment improved.
Add one more comment in this block and you are good to go:
https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/dom/security/nsMixedContentBlocker.cpp#509
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(amarchesini)
Comment 34•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6b4cebf12e3f
https://hg.mozilla.org/mozilla-central/rev/e7d2101e1aa9
https://hg.mozilla.org/mozilla-central/rev/314c6024ea27
status-firefox59:
--- → fixed
Target Milestone: --- → Firefox 59
Comment 35•7 years ago
|
||
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Group: firefox-core-security → core-security-release
Updated•7 years ago
|
Comment 36•7 years ago
|
||
Please request Beta approval on this when you're comfortable doing so.
Assignee | ||
Comment 37•7 years ago
|
||
Comment on attachment 8927770 [details] [diff] [review]
part 1 - mochitest
Approval Request Comment
[Feature/Bug causing the regression]: Containers
[User impact if declined]: save-link-as loads the page using a SystemPrincipal and the wrong OriginAttributes.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: This set of patches introduces a new nsIContentPolicy type. This is used for save a link. It should be relatively low risk.
[String changes made/needed]: none.
Flags: needinfo?(amarchesini)
Attachment #8927770 -
Flags: approval-mozilla-beta?
Comment 38•7 years ago
|
||
Comment on attachment 8927770 [details] [diff] [review]
part 1 - mochitest
This is sec-low. Let's let it ride the train. Beta58-. Feel free to nominate if you disagree.
Attachment #8927770 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•7 years ago
|
Comment 39•7 years ago
|
||
baku, we are still missing a comment here:
https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/dom/security/nsMixedContentBlocker.cpp#509
Flags: needinfo?(tanvi)
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(ted)
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [OA][usercontextId] → [OA][usercontextId][post-critsmash-triage]
Updated•7 years ago
|
Updated•7 years ago
|
Whiteboard: [OA][usercontextId][post-critsmash-triage] → [OA][usercontextId][post-critsmash-triage][adv-main59-]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•