1562582 - (CVE-2019-11765) browser-plugin.js has a sandbox related issue which allows content process to set arbitrary permissions

Status: RESOLVED FIXED

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

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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.

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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

Comment 2

[:Gijs (he/him)]

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

Comment 3

[:Gijs (he/him)]

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

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Radar'd!

Comment 5

[:Gijs (he/him)]

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

Comment 6

[Tom Ritter [:tjr]]

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.

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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

Comment 10

[Tom Ritter [:tjr]]

Attachment: advisory.txt