1800149 - Use parent process values rather than content process values in ClickHandlerParent and ContextMenuParent

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

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

Currently we trust arbitrary values for things like the triggering principal sent by content when loading links from ClickHandlerParent and ContextMenuParent, however the relevant principals are already available in the parent process on the actor objects. We should use the parent process values instead of sending them over unstructured JS IPC.

Comment 1

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

Attachment: Bug 1800149 - Part 1: Stop sending some values from a content process in nsContextMenu, r=gijs!

Currently we are sending some values, such as principals, from the content
process when creating a nsContextMenu. This information is already available in
the parent process on WindowGlobalParent, so changes the code to fetch the
values from there instead.

Comment 2

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

Attachment: Bug 1800149 - Part 2: Stop sending some values from a content process in ClickHandler, r=gijs!

Currently we are sending some values, such as principals, from the content
process when handling clicks from the ClickHandler actor. This information is
already available in the parent process on WindowGlobalParent, so changes the
code to fetch the values from there instead.

Depends on D161835

Comment 3

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Should this be a dep of Bug 1505832?

Comment 4

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

It's not really an information discovery issue, like what Fission was originally aimed to prevent. This sort of bug would require compromising the content process, not just exercising a spectre-like gadget to read its memory. That being said, the wording of bug 1505832 I suppose could include this as it's quite broad.

Comment 5

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Yeah; 1505832 is really a meta bug for "Bad things a compromised content process can do by impersonating another origin". (So it's not for all sandbox escapes since they usually don't require impersonating another origin)

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:nika, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 7

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

I believe I still have some test failures for this patch stack which I need to look into before landing.

Comment 8

[Sandor Molnar[:smolnar]]

Backed out for causing mochitest failures in referrer/browser_referrer_open_link_in_container_tab2.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/1209eebe76f27d7c03aa40aaa2d4be75fa7e3c74

Push with failures

Failure log

INFO - Buffered messages finished
[task 2022-12-13T00:37:23.617Z] 00:37:23     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js | policy=[undefined] rel=[undefined] http:// -> http:// - Got "", expected "http://test1.example.com/browser"
[task 2022-12-13T00:37:23.617Z] 00:37:23     INFO - Stack trace:

Comment 9

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

Part 1: Stop sending some values from a content process in nsContextMenu, r=Gijs,extension-reviewers,robwu
https://hg.mozilla.org/integration/autoland/rev/8694ca5957b766e1f81deb60cbb09084ed1d6a73
https://hg.mozilla.org/mozilla-central/rev/8694ca5957b7

Part 2: Stop sending some values from a content process in ClickHandler, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/a868f954be082e15cc006e9612f25339c4e8bd89
https://hg.mozilla.org/mozilla-central/rev/a868f954be08