Closed Bug 1562582 (CVE-2019-11765) Opened 1 year ago Closed 1 year ago

browser-plugin.js has a sandbox related issue which allows content process to set arbitrary permissions

Categories

(Firefox Graveyard :: Security: Review Requests, task)

task
Not set
normal

Tracking

(firefox-esr60 wontfix, firefox-esr68 wontfix, firefox68 wontfix, firefox69 wontfix, firefox70 fixed)

RESOLVED FIXED
Firefox 70
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: pauljt, Assigned: Gijs)

References

(Blocks 2 open bugs)

Details

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

Attachments

(1 file)

browser-plugins.js#44 has a broken security model. One of the parameters that it receiveMessages from the child is message.data.plugin. This includes information about the plug-in to show the "click-to-play" prompt. The problem is that it takes the permissions that is used in the call to Services.perms.addFromPrincipal from the child. In my testing it was possible to add "uitours" permission to an arbitrary host.

There is a mitigation in that it did actually require the user interaction to "click-to-play" but then it added the wrong permission. Because of the mitigation here, and because it's only permissions, which for the most part aren't that powerful, I'm rating this sec-moderate. That said, its a simple fix and we really should do something about it.

To see the code flow for that bad path, see the LGTM query here. Scroll to bottom and then click "Show Paths" to see the vulnerable data flows.

I confirmed this was a bug by testing in the debugger, and modifying the permissionString value at /browser-plugins.js#44.

PS Thanks to Semmle team for their help developing the query that lead to this issue being found!

I intend to come up with a fix in bug 1505913, unless we think we need to fix this more urgently.

(In reply to :Gijs (he/him) from comment #2)

I intend to come up with a fix in bug 1505913, unless we think we need to fix this more urgently.

Radar'ing this to mconley so me picking that up makes more sense. :-)

Group: core-security → firefox-core-security

This should be fixed on current tip now that bug 1505913 is fixed. Paul, can you confirm?

Flags: needinfo?(ptheriault)

Harder to evaluate without permalinks in the first comment :)

Looking at the diff; the path I see that's vulnerable is receiving a message 'ShowClickToPlayNotification' that provides msg.principal. The principal - previously provided by the content process - is now obtained from this.browsingContext.currentWindowGlobal.documentPrincipal.

But Paul's initial comment seems like something different; so maybe I missed a flow. He seems to be talking about providing an alternate permission... Previously, that came from aPluginInfo.permissionString; now it comes from aActivationInfo. Previously the permissionString in aPluginInfo came from plugins and argument to showClickToPlayNotification where it came from the content process in msg.data.plugins. Now it comes from gPluginHost.getPermissionStringForTag(pluginTag); where pluginTag ultimately comes from an id provided from the content process.

Yeah okay, the first thing I saw was me looking with my Fission "we shouldn't be getting principals from the content process" hat on; but now I see what this bug is really about; which is the arbitrary permission thing. This looks fixed to me.

(In reply to Tom Ritter [:tjr] from comment #6)

Harder to evaluate without permalinks in the first comment :)

Oops. Sorry for lack of permalink.

But yes, your analysis is correct here, this looks fixed to me. The issue of was the arbitrary permission string which is now replaced by selection from an allowlist in PluginParent.jsm#540

I originally just tested this by modifying the permission name in the Browser Toolbox. Its completely changed now so my PoC code obviously no longer works. But to verify i did confirm that we are only sending limited data from child to parent now. The parent only receives data like:

{
  "plugin": {
    "id": 0,
    "fallbackType": 8
  },
  "showNow": true
}

For posterity, the original code should have permalinked in the first comment was here. IE the part where we take the permssion string from the msg.data object.

Status: NEW → RESOLVED
Closed: 1 year ago
Flags: needinfo?(ptheriault)
Resolution: --- → FIXED

Are we OK with this fix riding the trains since bug 1505913 doesn't look like something we can easily backport?

Assignee: nobody → gijskruitbosch+bugs
Group: firefox-core-security → core-security-release
Flags: needinfo?(ptheriault)
Target Milestone: --- → Firefox 70

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)

Are we OK with this fix riding the trains since bug 1505913 doesn't look like something we can easily backport?

Yep, this is not an urgent fix. The impact is probably limited to things that are already a problem until we implement Fission anyways.

Flags: needinfo?(ptheriault)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main70+]
Product: Firefox → Firefox Graveyard
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.