1756056 - Filter eventDispatcher messages from content process

Status: RESOLVED FIXED

(GeckoView :: General, defect, P2)

Description

[[ex-Mozilla] Agi Sferro | :agi]

Currently, we don't check whether eventDispatcher messages originate in the content process or in the main process.

This is potentially a security problem, as it weakens the content process sandbox and it increases the security envelope allowing the content process to exploit a bug in any messages that the parent process listens to.

We should limit what messages we allow the content process to send to the minimum and discard any unexpected message.

While here, we could also limit to top-level frames when appropriate.

See also Bug 1610969.

Comment 1

[Chris Peterson [:cpeterson]]

This bug is more of a sandboxing bug than a Fission bug.

Comment 2

[Chris Peterson [:cpeterson]]

Moving [sandboxing] bugs to the new GeckoView::Sandboxing component.

Comment 3

[Chris Peterson [:cpeterson]]

Enhancements should have severity N/A.

Comment 4

[Daniel Veditz [:dveditz]]

Without knowing exactly what's supported, this sounds like a straight up sandbox escape feature if a child content can send messages that should only have been sent from parent JS. That's not an "enhancement"; you could argue it's an unfinished feature "task" maybe

Comment 5

[Daniel Veditz [:dveditz]]

nika: mccr8 said you had more specific examples or thoughts you could add here while it's still on your mind

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Daniel Veditz [:dveditz] from comment #5)

nika: mccr8 said you had more specific examples or thoughts you could add here while it's still on your mind

There are many different messages sent over the EventDispatcher bus which could be sent by an untrusted content process, which have various powers. Fortunately most of the really scary messages (like GeckoView:WebExtension:Install) are on the global event dispatcher, which doesn't appear to have a way for content processes to send messages on them.

I think the most notable ones are messages which the browser UI uses to control the session's state, such as GeckoView:LoadUri (https://searchfox.org/mozilla-central/rev/12ea2c521cdd071a6d25b0894f31f8f23b18b76a/mobile/android/modules/geckoview/GeckoViewNavigation.sys.mjs#171) to perform an arbitrary load on behalf of the user, with arbitrary load flags & headers, or GeckoView:RestoreState (https://searchfox.org/mozilla-central/rev/12ea2c521cdd071a6d25b0894f31f8f23b18b76a/mobile/android/modules/geckoview/GeckoViewContent.sys.mjs#233-235), which will effectively restore the current session to an arbitrarily specified sessionstore state, including doing things like performing navigations. There may be other APIs which are more powerful than I am immediately noticing though. I believe some features like permissions flags are also passed over these APIs.

The simplest fix here would be to remvoe these messages from GeckoViewActorChild/Parent and require the callers in the content process to instead send a message to the parent actor, which then in turn dispatches the message to the EventDispatcher. A non-exhaustive search (https://searchfox.org/mozilla-central/search?q=eventDispatcher&path=Child.sys.mjs&case=false&regexp=false) shows at least 8 actors which would need to be updated:

• ContentDelegateChild: https://searchfox.org/mozilla-central/rev/12ea2c521cdd071a6d25b0894f31f8f23b18b76a/mobile/android/actors/ContentDelegateChild.sys.mjs#26,100,141,150,156
• GeckoViewContentChild: https://searchfox.org/mozilla-central/rev/12ea2c521cdd071a6d25b0894f31f8f23b18b76a/mobile/android/actors/GeckoViewContentChild.sys.mjs#325
• GeckoViewPermissionChild: https://searchfox.org/mozilla-central/rev/12ea2c521cdd071a6d25b0894f31f8f23b18b76a/mobile/android/actors/GeckoViewPermissionChild.sys.mjs#17,32,65
• GeckoViewPrompterChild: https://searchfox.org/mozilla-central/rev/12ea2c521cdd071a6d25b0894f31f8f23b18b76a/mobile/android/actors/GeckoViewPrompterChild.sys.mjs#14,22,44
• LoadURIDelegateChild: https://searchfox.org/mozilla-central/rev/12ea2c521cdd071a6d25b0894f31f8f23b18b76a/mobile/android/actors/LoadURIDelegateChild.sys.mjs#27,32
• MediaControlDelegateChild: https://searchfox.org/mozilla-central/rev/12ea2c521cdd071a6d25b0894f31f8f23b18b76a/mobile/android/actors/MediaControlDelegateChild.sys.mjs#37
• ScrollDelegateChild: https://searchfox.org/mozilla-central/rev/12ea2c521cdd071a6d25b0894f31f8f23b18b76a/mobile/android/actors/ScrollDelegateChild.sys.mjs#21
• SelectionActionDelegateChild: https://searchfox.org/mozilla-central/rev/12ea2c521cdd071a6d25b0894f31f8f23b18b76a/mobile/android/actors/SelectionActionDelegateChild.sys.mjs#272,278,434

Comment 7

[Chris Peterson [:cpeterson]]

Jira issue: https://mozilla-hub.atlassian.net/browse/FXDROID-2046

Comment 8

[[:owlish] 🦉 PST]

(In reply to Nika Layzell [:nika] (ooo, ni? for response) from comment #6)

The simplest fix here would be to remvoe these messages from GeckoViewActorChild/Parent and require the callers in the content process to instead send a message to the parent actor, which then in turn dispatches the message to the EventDispatcher.

Hey Nika, looking at this briefly - looks like we actually already do this: the child actors do not have an actual EventDispatcher; when a a child actor is initialized, this.eventDispatcher field is populated by calling EventDispatcher.forActor, which returns new ChildActorDispatcher(aActor) (and the aActor is the child actor). When this.eventDispatcher.sendRequest is called on a child actor, the ChildActorDispatcher communicates with the parent actor.

It looks like we need to implement a check for the origin of the message, and potentially its type, to make sure it was sent from a trusted place. You mentioned that mccr8 has been working on implementing a message schema, I understand it is Bug 1885221. Should this bug be blocked on that work, or can we implement our own check specifically for EventDispatcher messages (because that bug is about actor messages)?

Comment 9

[Nika Layzell [:nika] (ni? for response)]

As we discussed earlier, the problem is that it is possible for a content process to send any EventDispatcher message over the pipe. My suggestion of changing the code to remove ChildActorDispatcher would do this by requiring each actor to implement the forwarding from content to parent themselves. This in turn would mean that the content process could no longer dispatch arbitrary events onto the EventDispatcher bus, as the specific events would be dispatched by the parent process with hard-coded types.

Comment 10

[[:owlish] 🦉 PST]

Attachment: (secure)

Comment 11

[[:owlish] 🦉 PST]

Comment on attachment 9506783 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?:
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?:
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?:
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 12

[[:owlish] 🦉 PST]

Comment on attachment 9506783 [details]
(secure)

How easily can the security issue be deduced from the patch?

• I think fairly easily, if one is familiar with the messaging security

Do comments in the patch, the check-in comment, or tests included in
the patch paint a bulls-eye on the security problem?

• I tried to be as vague as possible

Which older supported branches are affected by this flaw?

• Not sure how to answer

If not all supported branches, which bug introduced the flaw?

• The flaw was present from the start of GeckoView library

Do you have backports for the affected branches? If not, how
different, hard to create, and risky will they be?

• Not sure how to answer

How likely is this patch to cause regressions; how much testing does
it need?

• This is a risky change as far as potential regressions go.

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 9506783 [details]
(secure)

Approved to land. It sounds like there is not a known exploitation vector, and with the high risk of regressions, we probably want this to ride trains?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/3aa4f024fc89
https://hg.mozilla.org/integration/autoland/rev/9cc9fdf2fb4b
Actor messaging improvements r=geckoview-reviewers,nika

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d8f8717930c2
https://hg.mozilla.org/integration/autoland/rev/78c194369663
Revert "Bug 1756056 - Actor messaging improvements r=geckoview-reviewers,nika" for causing lint failures @ GeckoViewPrompterParent.sys.mjs

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for causing lint failures

• Backout link
• Push with failures
• Failure Log

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/mobile/shared/actors/GeckoViewPrompterParent.sys.mjs:130:9 | Unreachable code. (no-unreachable)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/53be33be635f
https://hg.mozilla.org/integration/autoland/rev/d14aa812e5ef
Actor messaging improvements r=geckoview-reviewers,nika

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0ec838865c5d
https://hg.mozilla.org/integration/autoland/rev/65d3aca3d43b
Revert "Bug 1756056 - Actor messaging improvements r=geckoview-reviewers,nika" for causing lint failure on SelectionActionDelegateParent.sys.mjs

Comment 19

[Cosmin Sabou [:CosminS]]

Backed out for causing lint failure on SelectionActionDelegateParent.sys.mjs

Push with failures

Failure log

Backout link

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/9b1981bcb377
https://hg.mozilla.org/integration/autoland/rev/5ca5c93e707e
Actor messaging improvements r=geckoview-reviewers,nika

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5ca5c93e707e

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Is this wontfix for 147?