Closed Bug 1365232 Opened 7 years ago Closed 7 years ago

Have _delayedStartup() provide the correct triggeringPrincipal

Categories

(Core :: DOM: Security, enhancement, P2)

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 1 obsolete file)

bug_1365232_triggeringprincipal_delayedstartup.patch
3.89 KB, patch
Gijs
: review+
Details | Diff | Splinter Review 
      No description provided.
Assignee: nobody → ckerschb
Blocks: 1363977
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Gijs, I tried to assemble a list of all the URLs that get loaded from within _delayedStartUp(). There is only one test that calls loadTabs directly within _delayzedstartup() [see first bullet], all other tests fall through to loadOneOrMoreURIs.

Anyway, I don't think that this list is getting us any further than we are right now, what do you think? I am happy to provide more info to keep this bug moving:

* browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js
  uriToLoad: http://example.com/,http://example.org/,http://example.net/
  calls loadOneOrMoreURIs: no

* toolkit/components/extensions/test/mochitest/test_ext_contentscript_incognito.html
  uriToLoad: http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/test-oop-extensions/file_sample.html
  calls loadOneOrMoreURIs: yes

* toolkit/components/extensions/test/mochitest/test_ext_inIncognitoContext_window.html
  uriToLoad: moz-extension://e1b90117-8388-488e-a6db-b3feb381da5f/tab.html
  calls loadOneOrMoreURIs: yes

* jetpack-package/addon-sdk/source/test/test-context-menu.js.testNewPrivateEnabledWindow
  uriToLoad: about:privatebrowsing
  calls loadOneOrMoreURIs: yes

* content/a11y/accessible/tests/mochitest/bounds/test_zoom.html
  uriToLoad: data:text/html,<html><body><p id='p1'>para 1</p><p id='p2'>para 2</p><map name='atoz_map' id='map'>
  calls loadOneOrMoreURIs: yes

* browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js
  uriToLoad: about:home
  calls loadOneOrMoreURIs: yes

* dom/tests/browser/browser_localStorage_privatestorageevent.js
  uriToLoad: about:privatebrowsing
  calls loadOneOrMoreURIs: yes

* toolkit/components/places/tests/browser/browser_visituri_privatebrowsing_perwindowpb.js
  uriToLoad: about:privatebrowsing
  calls loadOneOrMoreURIs: yes

* browser/components/extensions/test/browser/browser_ext_windows_allowScriptsToClose.js
  uriToLoad: moz-extension://754e779e-9e77-4df3-9148-e00482742fb4/dummy.html
  calls loadOneOrMoreURIs: yes

* browser/components/customizableui/test/browser_885530_showInPrivateBrowsing.js
  uriToLoad: about:privatebrowsing
  calls loadOneOrMoreURIs: yes

* browser/components/extensions/test/browser/browser_ext_incognito_popup.js
  uriToLoad: http://example.com/incognito
  calls loadOneOrMoreURIs: yes

* browser/components/extensions/test/browser/browser_ext_tabs_cookieStoreId.js
  uriToLoad: about:newtab
  calls loadOneOrMoreURIs: yes
  
* browser/components/extensions/test/browser/browser_ext_windows_create_url.js
  uriToLoad: moz-extension://b8d07bed-296e-4822-9853-3a6b1e2bd42c/test.html
  calls loadOneOrMoreURIs: yes

* browser/components/sessionstore/test/browser_354894_perwindowpb.js
  uriToLoad: about:privatebrowsing
  calls loadOneOrMoreURIs: yes

* browser/components/sessionstore/test/browser_394759_behavior.js
  uriToLoad: http://example.com/?window=5
  calls loadOneOrMoreURIs: yes

* browser/base/content/test/general/browser_tab_dragdrop2.js
  uriToLoad: chrome://mochitests/content/browser/browser/base/content/test/general/browser_tab_dragdrop2_frame1.xul
  calls loadOneOrMoreURIs: yes

* dom/plugins/test/mochitest/test_privatemode_perwindowpb.xul
  uriToLoad: about:privatebrowsing
  calls loadOneOrMoreURIs: yes

* toolkit/components/extensions/test/mochitest/test_chrome_ext_identity.html
  uriToLoad: https://example.com/chrome/toolkit/components/extensions/test/mochitest/oauth.html
  calls loadOneOrMoreURIs: yes
Flags: needinfo?(gijskruitbosch+bugs)
I think what we need to know is what calls into these methods. If there's a variety of things, I guess we need to pass a principal from the callsites.

Mike, I know you did a bunch of work on window opening in e10s. Can you help identify in what cases we end up with URIs to load in _delayedStartup (ie here: https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/browser/base/content/browser.js#1416-1495 ) and how we should determine what triggered those loads? Specifically, is it always going to be fully-user-controlled, or is it possible for code calling window.open() in content to end up in that code - or is that handled elsewhere (if so, where?). Thanks!
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mconley)
(In reply to :Gijs from comment #3)
> Specifically, is it always going to be fully-user-controlled, or is it
> possible for code calling window.open() in content to end up in that code -
> or is that handled elsewhere (if so, where?). Thanks!

IIRC, we choose URIs to load in _delayedStartup from callers that open browser.xul and pass one or more URIs as an argument.

Here's an example of where we do that:

http://searchfox.org/mozilla-central/rev/35b37316149108d53a02fb8498e250ea8a22ab5d/browser/components/nsBrowserContentHandler.js#753-755

I believe this is when a series of URIs are passed as cmdline arguments to the firefox binary.

I don't believe window.open from content will ever go through this path. window.open from content (in the e10s-case) will go through ContentParent::CommonCreateWindow which will either end up asking nsIBrowserDOMWindow to open a new tab via openURIInFrame, or will open a new window using nsWindowWatcher::OpenWindowWithTabParent, which ends up asking the tab to load the URI directly rather than going through _delayedStartup.

So, to sum, I don't believe window.open from content ever goes through _delayedStartup. There are a myriad of things, I believe, that go through _delayedStartup to load URIs (by passing them as arguments), but window.open is not one of them.

Does that help?
Flags: needinfo?(mconley)
OK. Here's a scrubbed list (still contains a few one-off unrelated things, but it's short enough to actually audit...):

https://dxr.mozilla.org/mozilla-central/search?q=chrome%3A%2F%2Fbrowser%2Fcontent%2F+-path%3Atest+-%27src%3D%22chrome%27+-%27href%3D%22chrome%27+-%27url(%22chrome%27+-%27url(chrome%27+-%22url(%27chrome%22+-path%3Ajar.mn+-file%3Aaboutredir+-overlay.xul+-.js+-dialog.xul+-cookies+-hiddenwindow+-pageinfo+-desktopbackground+-sanitize+-customizableui+-.css+-.xml+-.xhtml+-gsubdialog+-panel.xul+-panels.xul+-.svg+-ww.openWindow&redirect=false

We'd need to separately check getBrowserURL() and the browser.chromeURL pref (which I've had a cursory look at and which look OK to me, at a glance).

I think system principal is fine for all the callsites I saw. The question is... how do we make sure that stays the case? :-(

I don't much fancy updating all the callsites, but equally I don't have significantly better ideas.

Though, to be fair, we've already updated Services.ww.openWindow(), and not everyone passes a triggering principal there, so maybe I'm worrying over nothing? Equally, I noticed that webextensions' oauth flow uses that without a triggering principal, and AIUI the oauth target is webextension-controlled... I assume we're doing some kind of sanitization of that URL elsewhere, but it would make sense to check.

Decisions, decisions. Christoph, what do you want to do?
Flags: needinfo?(ckerschb)
(In reply to :Gijs from comment #5)
> I think system principal is fine for all the callsites I saw. The question
> is... how do we make sure that stays the case? :-(

Yeah, that's not easy to enforce.

> I don't much fancy updating all the callsites, but equally I don't have
> significantly better ideas.

How about landing this patch using systemPrincipal as the triggeringPrincipal but also file a follow up to pass an explicit triggeringPricnipal around. Problem is (as already discussed) these callsites fan out too much, which means it would take quite some time to actually start passing the correct triggeringPricnipal from the place where the actually loading happens. Ultimately I would like that though.

> Though, to be fair, we've already updated Services.ww.openWindow(), and not
> everyone passes a triggering principal there, so maybe I'm worrying over
> nothing?

Note quite, because ultimately I would like to land the assertion within Bug 1333030 so that all docshell loads need to pass a valid triggeringPricnipal. Although that wouldn't catch the problem e.g. within _delayedstartup(). Anyway, overtime I hope to get there.

> Decisions, decisions. Christoph, what do you want to do?

Would you be fine with using systemPrincipal within _delayedstartup() for now but also file a follow up? My hope is that once we get more and more callsited converted to pass a triggeringPrincipal, then everything becomes a little easier.
Flags: needinfo?(ckerschb)
Gijs, in case you agree to comment 6, then this would be the patch to go down that route.
Attachment #8870042 - Attachment is obsolete: true
Attachment #8872444 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8872444 [details] [diff] [review]
bug_1365232_triggeringprincipal_delayedstartup.patch

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

r=me
Attachment #8872444 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38723a305584
Have _delayedStartup() provide the correct triggeringPrincipal. r=gijs
https://hg.mozilla.org/mozilla-central/rev/38723a305584
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: