Closed Bug 1539590 Opened 5 years ago Closed 5 years ago

Parent process should prompt when installing a language pack

Categories

(Toolkit :: Add-ons Manager, defect, P1)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox-esr60 --- fixed
firefox68 --- fixed

People

(Reporter: Alex_Gaynor, Assigned: aswan)

References

Details

(Keywords: sec-want)

Allowing 0-click translation pack installation enabled the exploit in bug 1538007. We should introduce a user prompt.

I think that the more general fix for this is prompting for all mozAddonManager installs that don't come from the discovery pane, or don't point to listed AMO add-ons.

I thought we already had a bug for this, but it appears we don't. I'll file one and add a link here when I do. For obvious reasons, I won't add any dependencies to that bug.

No objection from me to an even more general solution :-) Thanks Kris!

Can we avoid lowering the UX of installing langpacks from the Preferences Language Selection UI?

Yes. My plan is to only change the behavior when install requests come from untrusted websites. about:preferences and the about:addons discovery pane will be unaffected.

that sounds great, thank you!

Group: core-security → firefox-core-security
Component: Localization → Add-ons Manager
Keywords: sec-want
Product: Core → Toolkit
Priority: -- → P1

(In reply to Kris Maglione [:kmag] from comment #1)

I think that the more general fix for this is prompting for all mozAddonManager installs that don't come from the discovery pane,

Does this actually help with bug 1538007? Seems like a compromised content process could rather easily make an install request look like it is coming from the discovery pane.

or don't point to listed AMO add-ons.

This is much lower-hanging fruit. AMO download links for unlisted addons require that you be logged into AMO as the author of the addon. mozAddonManager should never require credentials, we can just make all downloads initiated from mozAddonManager anonymous.

(In reply to Andrew Swan [:aswan] from comment #7)

(In reply to Kris Maglione [:kmag] from comment #1)

I think that the more general fix for this is prompting for all mozAddonManager installs that don't come from the discovery pane,

Does this actually help with bug 1538007? Seems like a compromised content process could rather easily make an install request look like it is coming from the discovery pane.

Only if untrusted content is running in the same process as the discovery pane. We can easily prevent that. Even if we couldn't, getting a web page to run in the same process as an open discovery pane requires a lot of luck.

I guess in the short term we can just check for the discovery pane loaded inside about:addons and rely on the fact that that runs in the parent process :(
In the longer term, once bug 1540173 lands we can make mozAddonManager prompt unconditionally and get rid of any special handling.
I'll see what I can do for bug 1539598...

(In reply to Andrew Swan [:aswan] from comment #9)

I guess in the short term we can just check for the discovery pane loaded inside about:addons and rely on the fact that that runs in the parent process

Right. We only want to accept requests from the discovery pane when it's loaded inside about:addons. We don't want to accept requests from the discovery pane URL when it's loaded anywhere else.

Type: enhancement → defect

(In reply to Andrew Swan [:aswan] from comment #7)

or don't point to listed AMO add-ons.

This is much lower-hanging fruit. AMO download links for unlisted addons require that you be logged into AMO as the author of the addon. mozAddonManager should never require credentials, we can just make all downloads initiated from mozAddonManager anonymous.

Ah, and this is actually already done. I haven't tried the original POC but I suspect it won't work as of Firefox 67. Bug 1504056 landed in 67 and with that patch, we don't send cookies with mozAddonManager install requests.

That said, the exploit would also work with a listed addon, it would just be slightly more likely to get noticed so we will still proceed with prompting for all content-initiated installs outside of about:addons.

(In reply to Andrew Swan [:aswan] from comment #11)

(In reply to Andrew Swan [:aswan] from comment #7)

or don't point to listed AMO add-ons.

This is much lower-hanging fruit. AMO download links for unlisted addons require that you be logged into AMO as the author of the addon. mozAddonManager should never require credentials, we can just make all downloads initiated from mozAddonManager anonymous.

Ah, and this is actually already done. I haven't tried the original POC but I suspect it won't work as of Firefox 67. Bug 1504056 landed in 67 and with that patch, we don't send cookies with mozAddonManager install requests.

Note we'll probably want to take that patch for the chemspill.

I'm not an expert in the addons code, which process is making the decision not to include cookies in bug 1504056, content or parent?

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #13)

I'm not an expert in the addons code, which process is making the decision not to include cookies in bug 1504056, content or parent?

The parent process.
To be specific, this code runs in the parent process
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/toolkit/mozapps/extensions/AddonManager.jsm#2692-2697
and it does not include {sendCookies: true} in the second argument.

Ok, fantastic. Thanks!

:aswan would it be fair to assign this bug to you? or is this something we should assign to :kmag? I'm looking for the correct person to reach out to when we're trying to figure out whether it is resolved or not (due to work happening in public bugs which are not linked).

I'm on it. The public bug now got linked to 1539591 though :/

Assignee: nobody → aswan

Okay bug 1539598 has landed, I can request uplift to 67 if that's desired.
But I don't think there's any more to do here, speak up if you disagree, otherwise I'll close this one.

If the 0-interaction install language pack behavior from a content process immitating AMO has been resolved, then I think this can be closed.

Shall we close this?

Flags: needinfo?(aswan)

Can we get QA to verify that the original exploit no longer works? (I assume QA will see bug 1539598 and do something to verify, but that bug deliberately doesn't have a link to the original steps here)

Needinfo'ing myself to perform the testing described in comment 21. My vague plan was to testing the original exploit, but also just to debug the message flows and try to make certain there are NOT any ways for a compromised child process to bypass the prompt.

Flags: needinfo?(ptheriault)

Clearing my ni? in favor of Paul's. We can either resolve this or discuss next steps depending on the results of his investigations.

Flags: needinfo?(aswan)

Just an update here, I performed some testing to verify that the basic exploit is gated on the prompt, but I haven't had time to completely explore the message flows here to answer the second part of the work I proposed in comment 22. Will get to this asap, hopefully in the next day or two

I'm going to close this out as part of a sec bug triage I'm doing in webext. Leaving paul's NI so he has a reminder on the follow ups mentioned.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1539598
Group: firefox-core-security → core-security-release
status-firefox68: affected → fixed
status-firefox-esr60: --- → fixed
Target Milestone: --- → mozilla68
Flags: needinfo?(ptheriault)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.