1380597 - (CVE-2017-7816) sidebarAction.setPopup is missing a URL security check

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Kestrel]

It used to be possible for extensions to load privileged about: pages in the sidebar with browser.sidebarAction.setPanel but now with OOP enabled they fail to load with the following errors:

"Error: You cannot use the AddonManager in child processes!"

"Use of nsIFile in content process is deprecated."

Comment 1

[Andrew McCreight [:mccr8]]

Kris, why did you make this a security issue?

Comment 2

[Kris Maglione [:kmag]]

Some of these URLs fail to load properly in OOP mode, but extensions should not be able to load them at all. The missing security check is the security issue.

Comment 3

[Andrew McCreight [:mccr8]]

I guess I'll mark this moderate because it requires a malicious extension.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Is this also an issue with browserAction and pageAction?  None do a check on the url passed to setPopup/Panel, just context.uri.resolve(newUrl).

Comment 5

[Kestrel]

I tested browserAction and pageAction popups and they are also affected.

I don't see there being anything inherently malicious about simply loading a privileged about: page, I haven't encountered any way to interfere with the privileged content. Bug 1371793 is already considering allowing this for tabs and goes over some of the security considerations.

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Kestrel from comment #5)
> I tested browserAction and pageAction popups and they are also affected.

Much appreciated.

> I don't see there being anything inherently malicious about simply loading a
> privileged about: page, I haven't encountered any way to interfere with the
> privileged content. Bug 1371793 is already considering allowing this for
> tabs and goes over some of the security considerations.

I agree there is nothing inherently malicious, but when it is, there are some about pages that become a problem.  Bug 1371793 is there to figure out what ones are safe or not, etc.  Until that is resolved, it is easier to blanket assume problems.

All our about pages are or will basically end up being designed for tab based browsers.  I don't think we should allow them in other areas.  I've done that, some simply do not work well in small format.

Comment 7

[:Gijs (he/him)]

CC :ckerschb because I would have expected docshell to prevent the load (independent of separate security checks being present or not) iff the triggering principal of the load was correct, which I therefore assume it isn't.

Comment 8

[Kris Maglione [:kmag]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> Is this also an issue with browserAction and pageAction?  None do a check on
> the url passed to setPopup/Panel, just context.uri.resolve(newUrl).

Yes. I think we may have checked those URLs in the popup code in the past. The check should have been moved to the schema when that was removed.

(In reply to :Gijs from comment #7)
> CC :ckerschb because I would have expected docshell to prevent the load
> (independent of separate security checks being present or not) iff the
> triggering principal of the load was correct, which I therefore assume it
> isn't.

Correct, we don't set a triggering principal for these loads, which we probably should. The same goes for tab loads via the tabs and windows APIs.

Comment 9

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: add load checking for url

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8905289 [details] [diff] [review]
add load checking for url

Or aswan, kmag seems backlogged.

Comment 12

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #8)
> (In reply to :Gijs from comment #7)
> > CC :ckerschb because I would have expected docshell to prevent the load
> > (independent of separate security checks being present or not) iff the
> > triggering principal of the load was correct, which I therefore assume it
> > isn't.
> 
> Correct, we don't set a triggering principal for these loads, which we
> probably should. The same goes for tab loads via the tabs and windows APIs.

Can we do this in a followup bug if doing it here is tricky (which it probably is, esp. the tabs/windows API thing)?

Comment 13

[Kris Maglione [:kmag]]

(In reply to :Gijs from comment #12)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #8)
> > Correct, we don't set a triggering principal for these loads, which we
> > probably should. The same goes for tab loads via the tabs and windows APIs.
> 
> Can we do this in a followup bug if doing it here is tricky (which it
> probably is, esp. the tabs/windows API thing)?

Yes. Please file a follow-up bug. I think we should definitely do this, but it also seems separate from this bug.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/446798b4fc37aa40e1a626d0840057d4f805764b

Please request Beta approval on this when you're comfortable doing so. I've verified that it grafts cleanly.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/446798b4fc37

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8905289 [details] [diff] [review]
add load checking for url

Approval Request Comment
[Feature/Bug causing the regression]: webextensions ui
[User impact if declined]: Possible for addons to open some about urls in the extension ui
[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?]: not a trivial change, but still pretty simple
[String changes made/needed]: none

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8905289 [details] [diff] [review]
add load checking for url

Adds missing security check, let's uplift this to beta.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6b935e676fe9