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.

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.

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?

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

What is the current content type?

(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

Tanvi, this is still marked tracking for FF57. Do we need to fix it or can we remove the tracking flag?

Ted, can you check if this is also an issue in regular and private mode?

Given how close we are to 57 launch, I think we have to wait for 58 on this one.

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

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

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

This is the test. Now we have to write the fix :)

Note that I'm still using TYPE_OTHER. Using TYPE_DOCUMENT breaks LoadInfo, which cannot be created from JS, since bug 1105556.

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.

Note that my mochitest implements a redirect and it passes correctly with this patch applied.

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?

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

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

(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

This is the triggering principal, not the loading principal.

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.

Comment on attachment 8927771 [details] [diff] [review] part 2 - browser Review of attachment 8927771 [details] [diff] [review]: ----------------------------------------------------------------- that also looks good.

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

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.

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.

I cannot use '-'. TYPE_SAVEAS_DOWNLOAD seems a good name to me. Alternatively: TYPE_SAVE_LINK_AS_DOWNLOAD. Comment improved.

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

(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

https://hg.mozilla.org/mozilla-central/rev/6b4cebf12e3f https://hg.mozilla.org/mozilla-central/rev/e7d2101e1aa9 https://hg.mozilla.org/mozilla-central/rev/314c6024ea27

Please request Beta approval on this when you're comfortable doing so.

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.

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.

baku, we are still missing a comment here: https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/dom/security/nsMixedContentBlocker.cpp#509