1770137 - (CVE-2022-1802) Spot fix for pwn2own-2022 sandbox escape

Status: VERIFIED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Peter Van der Beken [:peterv]]

+++ This bug was initially created as a clone of Bug #1770057 +++

This bug is for the sandbox escape portion of the pwn2own-2022, described in section 2.2 of the PDF.

I think the basic, specific issue is that on this line of code we use an untrusted string from the content process to index into an Object:
this.notifications[origin][notification.id] = notification;

If origin is the string "__proto__", then we're now polluting the prototype of Object used by all JSMs, which can be taken advantage of later.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Bug 1770137 - Make notification code use Object.create. r?gijs!,freddyb!

Comment 2

[Frederik Braun [:freddy]]

For posterity: We've decided that a spot fix is the saner approach given the reduced time we have and the amount of code it would have to touch otherwise. Fix for the underlying issue of prototype pollutions in JS IPC code will happen in through Bug #1770057, but not immediately.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 9277284 [details]
Bug 1770137 - Make notification code use Object.create. r?gijs!,freddyb!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it shouldn't be too hard for people who know about prototype pollution to figure out how to exploit this.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Patch should just apply.
• How likely is this patch to cause regressions; how much testing does it need?: This shouldn't be very risky.

I'll note here that nightly uses a different implementation, so the patch would be a noop there :-/.

• Is Android affected?: Yes

Comment 4

[Andrew McCreight [:mccr8]]

This code is disabled in Nightly (see bug 1770154), so I'm marking 102 as disabled. We still want to land the patch there, but that should be kept in mind for testing and verification purposes.

Comment 5

[Tom Ritter [:tjr]]

Comment on attachment 9277284 [details]
Bug 1770137 - Make notification code use Object.create. r?gijs!,freddyb!

Approved to move forward

Comment 6

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

Make notification code use Object.create. r=Gijs,freddyb
https://hg.mozilla.org/integration/autoland/rev/60f027a372e5baff03aa1463aeed366e2a26d468
https://hg.mozilla.org/mozilla-central/rev/60f027a372e5

Comment 7

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

Comment on attachment 9277284 [details]
Bug 1770137 - Make notification code use Object.create. r?gijs!,freddyb!

Beta/Release Uplift Approval Request

• User impact if declined: sandbox escape
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: IIRC we don't have a reliable QE test strategy for this change yet. :mccr8 is working on building a snippet which we'll be able to paste in the browser content toolbox to test this fix (though it will only work on Beta and Release)
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This shouldn't be very risky. Nightly uses a different version of the code.
• String changes made/needed: None
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: sandbox escape
• Fix Landed on Version: 102
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This shouldn't be very risky. Nightly uses a different version of the code.

Comment 8

[Dianna Smith [:diannaS]]

Comment on attachment 9277284 [details]
Bug 1770137 - Make notification code use Object.create. r?gijs!,freddyb!

Approved for 100.0.2
Approved for 101.0b9
Approved for 91.9.1esr

Comment 9

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/ab6ade69feee (91.9.1)
https://hg.mozilla.org/releases/mozilla-esr91/rev/f26740996746
https://hg.mozilla.org/releases/mozilla-beta/rev/97f2fc7c28b7
https://hg.mozilla.org/releases/mozilla-release/rev/d6d9df73d017

Comment 10

[Andrew McCreight [:mccr8]]

Steps to reproduce:

You want to start with a fresh profile. (The important factor is that we need to make sure there are no existing saved notifications, which will show up as notificationstore.json in the profile directory. You can just delete that file to get back to the "fresh" state when the browser is not running.)

1. First, you have to enable the browser toolbox.

a) Hamburger menu --> More tools --> Developer tools

b) Click on the ... next to the x on the developer tools bar, then click on settings. Under "Advanced settings" in the lower right, check "Enable browser chrome and addon-on debugging toolboxes" and "Enable remote debugging".

2. Now, load the browser toolbox. Hamburger menu --> More tools --> Browser toolbox.

3. In the window that pops up, at the bottom there is an area for console input starting with ">>". Click on that to get a cursor, then paste in the following string and hit enter:

Services.cpmm.sendAsyncMessage("Notification:Save", {origin:"__proto__", notification: { id: "someKey" }}); setTimeout(() => { alert("This should be undefined: " + Cu.getGlobalForObject(Services).Object.prototype.someKey); }, 100);

This will pop up an alert in main browser window. It will say "This should be undefined" followed by a JS value. If it says "undefined", then the exploit is fixed. If it says "[Object]", it is not.

4. Now, exit the browser, start the browser again, then repeat steps 2 and 3. The pop up should again say undefined. (This tests a separate code path when we restore the notifications from disk, and was unfixed in the initial patch.)

Also note that in my experiments the browser would hang and crash on exit in the cases where the test failed, probably because the Object prototype got messed up, but there's no need to worry about that.

(Nika helped me put these steps together.)

Comment 11

[Andrew McCreight [:mccr8]]

Additional testing notes: It looks like this code goes through a slightly different code path in a fresh profile and in an existing profile, which we found out because the test case was failing because I tried it in a old profile, so it would be good to exit the browser and repeat the steps in the same profile a second time, if it passes the first time. (Nika is working on a fix for that.)

Also note that in my experiments the browser would hang and crash on exit in the cases where the test failed, probably because the Object prototype got messed up, but there's no need to worry about that.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Per the Slack channel, we need a follow-up patch for this bug.

Comment 13

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

Attachment: Bug 1770137 - Part 2, r=gijs

Comment 14

[Andrew McCreight [:mccr8]]

For the purposes of this bug, a "fresh state" in a profile seems to mean that notificationstore.json doesn't exist in the profile directory, so you can delete that file to get back to a fresh state. With only the initial patch, you get something like: delete notificationstore.json, start browser, run test. Test passes. (Test causes a new notificationstore.json to be created...) Restart browser, run test. Now the test fails.

Comment 15

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

Comment on attachment 9277369 [details]
Bug 1770137 - Part 2, r=gijs

Beta/Release Uplift Approval Request

• User impact if declined: sandbox escape for existing profiles
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: see comment 10, try on both new and existing profiles.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): quick fix to remaining hole from previous spot-fix.
• String changes made/needed: None
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: sandbox escape for existing profiles
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): quick fix to remaining hole from previous spot-fix.

Comment 16

[Dianna Smith [:diannaS]]

Comment on attachment 9277369 [details]
Bug 1770137 - Part 2, r=gijs

Approved for 100.0.2
Approved for 101.0b9
Approved for 91.9.1esr

Comment 17

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/262b3b86a564 (91.9.1)
https://hg.mozilla.org/releases/mozilla-esr91/rev/842ab8d9394a
https://hg.mozilla.org/releases/mozilla-beta/rev/49f60cc53ea3
https://hg.mozilla.org/releases/mozilla-release/rev/7ce9f0fe6cb4

Comment 18

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

Part 2, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/27b6009eb1f7cef2177e3e42aed6823fd510a54a
https://hg.mozilla.org/mozilla-central/rev/27b6009eb1f7

Comment 19

[Hani Yacoub, Desktop QA]

Verified as fixed on macOS 11.6, Windows 10 x64 and on Ubuntu 20.04 x64.

Comment 20

[Delia Pop (Android QA)]

Verified as fixed on Mobile Fenix/Focus RC 100.3.0 and Beta 101.0.0 Beta 6.