2038695 - Validate IsKnownForChildID in more places

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

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

There are a few places in core APIs where we don't use MaybeDiscardedBrowsingContext for various reasons, which means those callers don't get the automatic checks from bug 2008912. We should change those places to re-add those checks.

Comment 1

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

Attachment: (secure)

Comment 2

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

Attachment: (secure)

Comment 3

[Daniel Veditz [:dveditz]]

I guessed sec-moderate because it sounded like a spin-off of bug 2008912, but maybe this should be sec-high like the see-also bug 2038573?

Comment 4

[Andrew McCreight [:mccr8]]

It really depends on the specific uses. The JS actors fixed by Nika's other bug are very scary and should not be run in a web content process, which is why those are so bad. At the mildest, this is just like a site isolation bypass. I'm not sure how bad these specific fixes are. Maybe with BrowserBridgeParent you could get or send graphics from another site? Eh doesn't seem too terrible, maybe.

Comment 5

[Andrew McCreight [:mccr8]]

Ah, okay. BrowserParent::RecvNewWindowGlobal is specifically called out in bug 2038573 as failing to do this check that the patch is adding (step 4).

Comment 7

[BugBot [:suhaib / :marco]]

There are some r+ patches which didn't land and no activity in this bug for 1 week.
: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 BugBot documentation.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c9d81296d751
https://hg.mozilla.org/integration/autoland/rev/972999718087
Part 1: Validate IsKnownForChildID in more places, r=smaug
https://github.com/mozilla-firefox/firefox/commit/afe526490b76
https://hg.mozilla.org/integration/autoland/rev/b06f06b489bf
Part 2: Use MaybeDiscardedBrowsingContext for OpenContinuationWindow, r=bvandersloot

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/972999718087
https://hg.mozilla.org/mozilla-central/rev/b06f06b489bf

Comment 10

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:nika, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox152 to wontfix.

For more information, please visit BugBot documentation.

Comment 11

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

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D299734

Comment 12

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Extra validation on methods around BrowsingContext to ensure that a BC which the process should not know about is not named.

This is more of a defence in depth. I don't know ottomh if there's anything exploitable being prevented here. It's probably OK if we choose not to take this in beta.

• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Fairly straightforward check being added to some IPC receive call-sites.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 13

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

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D299735

Comment 14

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/e5ba04320e31
https://hg.mozilla.org/releases/mozilla-beta/rev/7d4d9d7c66b7
https://github.com/mozilla-firefox/firefox/commit/8fb2d05d687a
https://hg.mozilla.org/releases/mozilla-beta/rev/3bb65b29ea7c