1915280 - UITours does permission checking in the child process

Status: RESOLVED FIXED

(Firefox :: Tours, defect, P3)

Description

[Andrew McCreight [:mccr8]]

Gijs mentioned UITours to me as an actor that is supposed to run in only a handful of specific Mozilla domains, with powerful capabilities. Unfortunately, it looks like the permission check is done in the child process which means that if an attacker takes over the content process it could bypass those checks.

Scanning through UITour.sys.mjs, I'm not sure how bad this would be. The setDefaultSearchEngine case looks like they could set the default search engine which honestly is probably more valuable than most things you could do with control over somebody's machine, though at least it would be relatively easy to fix. There's the resetFirefox which pops up a window to confirm that somebody wants to reset their Firefox profile. That doesn't seem too useful except to try to trick somebody into removing their addons. There's a few accounts related things like showFirefoxAccounts and showConnectAnotherDevice but those look benign.

Comment 1

[Andrew McCreight [:mccr8]]

What this really kind of wants is a way to register an actor like "matches", except it is based on permissions and not the domain name. Getting that right would be a little tricky (permissions can change over time) but maybe that's even more of an argument for doing it in one place.

Comment 2

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

The severity field is not set for this bug.
:Mardak, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[:Gijs (away/busy until Mar 10)]

Attachment: Bug 1915280 - improve UITour origin checks, r?Mardak

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dedd7e191458
improve UITour origin checks, r=Mardak,nika

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/dedd7e191458

Comment 6

[Daniel Veditz [:dveditz]]

Should we uplift to 138? backport to ESR-128?

Comment 7

[:Gijs (away/busy until Mar 10)]

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

Should we uplift to 138? backport to ESR-128?

This is defense-in-depth so tentatively I don't think so, at least for ESR? Backporting to ESR would be a bit annoying because I'm using moz-src which isn't available there. I didn't think that at this point we were aware of any active issues around UITour so I had not considered backporting this.

138 I could be convinced about, though I'd like to get confirmation from the web team that I unbroke their local dev setup with this change (by allowing on localhost iff that is set up in the testing origins pref).

Comment 8

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

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

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox138 to wontfix.

For more information, please visit BugBot documentation.

Comment 9

[:Gijs (away/busy until Mar 10)]

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

138 I could be convinced about, though I'd like to get confirmation from the web team that I unbroke their local dev setup with this change (by allowing on localhost iff that is set up in the testing origins pref).

Got this from Alex so going to request beta uplift.

Comment 10

[:Gijs (away/busy until Mar 10)]

Attachment: Bug 1915280 - improve UITour origin checks, r?Mardak

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

Comment 11

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Brokenness for webdev + security defense in depth improvement
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: low-ish
• Explanation of risk level: Relatively small change to UITour which wouldn't affect core product (so could potentially mitigate breakage on the web-side), still decent beta runway left
• String changes made/needed: No
• Is Android affected?: no

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/d0fd99640c4f

Comment 13

[Daniel Veditz [:dveditz]]

It's fine to skip this for ESR-128.

Comment 14

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt

Comment 15

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt