Closed Bug 1380597 (CVE-2017-7816) Opened 7 years ago Closed 7 years ago

sidebarAction.setPopup is missing a URL security check

Categories

(WebExtensions :: Frontend, defect, P1)

defect

Tracking

(firefox-esr52 wontfix, firefox54 unaffected, firefox55 wontfix, firefox56 fixed, firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- unaffected
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: ke5trel, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main56+][post-critsmash-triage] triaged)

Attachments

(1 file)

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."
Blocks: webext-oop
Keywords: regression
Assignee: nobody → mixedpuppy
Group: toolkit-core-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Summary: Cannot load privileged about: pages in sidebar with OOP enabled → sidebarAction.setPopup is missing a URL security check
Kris, why did you make this a security issue?
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
I guess I'll mark this moderate because it requires a malicious extension.
Keywords: sec-moderate
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).
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.
(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.
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.
Whiteboard: triaged
Priority: -- → P1
(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.
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.
Keywords: stale-bug
Keywords: stale-bug
Attachment #8905289 - Flags: review?(kmaglione+bmo)
Comment on attachment 8905289 [details] [diff] [review]
add load checking for url

Or aswan, kmag seems backlogged.
Attachment #8905289 - Flags: review?(aswan)
(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)?
Attachment #8905289 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
(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.
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.
Attachment #8905289 - Flags: review?(aswan)
https://hg.mozilla.org/mozilla-central/rev/446798b4fc37
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: toolkit-core-security → core-security-release
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
Flags: needinfo?(mixedpuppy)
Attachment #8905289 - Flags: approval-mozilla-beta?
Depends on: 1398713
Comment on attachment 8905289 [details] [diff] [review]
add load checking for url

Adds missing security check, let's uplift this to beta.
Attachment #8905289 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Alias: CVE-2017-7816
Whiteboard: triaged → [adv-main56+] triaged
Flags: qe-verify-
Whiteboard: [adv-main56+] triaged → [adv-main56+][post-critsmash-triage] triaged
Group: core-security-release
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.