1559858 - (CVE-2019-11708) Sending `Prompt:Open` from the child allows for a sandbox escape

Status: RESOLVED FIXED

(Firefox :: Security, defect, P1)

Description

[Frederik Braun [:freddy]]

A successful exploit in the child process may use the messageManager to request itself (or really any URL) being opened in the parent process with the Prompt:Open message.

This allows for running the exploit again and thus bypassing the sandbox.

Comment 1

[Frederik Braun [:freddy]]

Attachment: PromptOpenSandboxEscape.txt

Comment 2

[Frederik Braun [:freddy]]

The attached TXT file, when executed in the Browser Content Toolbox can reliably open arbitrary documents in the parent.
Note that bug 1521794 wouldn't prevent this, as a data or a blob URI would be totally sufficient.

Comment 3

[Andrew McCreight [:mccr8]]

It looks like this was added way back in 2014 in bug 899648, so basically any version of e10s we shipped would have this issue.

Comment 4

[Frederik Braun [:freddy]]

Gijs, I'm adding you as you seem one of the people who were lately contributing to this part of the codebase, but please feel free to redirect me as needed. This bug here was used as the sandbox escape in the chemspill release, we are just doing. We do not intend to ship something right away for the sandbox escape, but I think this week might pose a good opportunity to chat about fixing this.

Comment 5

[Johann Hofmann [:johannh]]

Is there really any use case where the uri parameter should be provided by web content? That sounds like a total bug. Off-hand I bet that we could just disallow the uri parameter in the content process and run a green try. I'll try to find out...

Comment 6

[Johann Hofmann [:johannh]]

Seems like it's just used for alternating between two hardcoded strings: https://searchfox.org/mozilla-central/rev/b3b401254229f0a26f7ee625ef5f09c6c31e3949/toolkit/components/prompts/src/Prompter.jsm#553-560

:((

Comment 7

[Frederik Braun [:freddy]]

Ah good spot. I've been pushing this stop gap to try. A blacklist approach meh: https://treeherder.mozilla.org/#/jobs?repo=try&revision=abb9da9c4609bff1f2e302efbcfc4ed89728d799

It seems we could just pass the name of the dialog ("commonDialog" or "selectDialog") and use that instead of a URI then? That would be much better

Comment 8

[Johann Hofmann [:johannh]]

If my assumptions are correct I have a patch for this...

Comment 9

[Johann Hofmann [:johannh]]

Attachment: Bug 1559858 - Avoid unnecessary uri passing. r=Gijs

Comment 10

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b490a19608943fbb8f5314f69589d5d5589314e

Comment 11

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Based on the discussion in Slack, it sounds like we're aiming to ship this in 68 so it gets a bit of bake time first.

Comment 12

[:Gijs (he/him)]

Please file a sec-sensitive follow-up to audit all callers of openWindow and/or window.open in chrome-privileged JS, and another follow-up to forbid anything other than chrome: and resource: (and maybe about:) params for either. We probably use some data: / blob: things today for tests, if there's no obvious workaround for that we can use a pref that only works in automation to punch the requisite hole.

Comment 13

[:Gijs (he/him)]

(In reply to Frederik Braun [:freddyb] from comment #4)

Gijs, I'm adding you as you seem one of the people who were lately contributing to this part of the codebase, but please feel free to redirect me as needed. This bug here was used as the sandbox escape in the chemspill release, we are just doing. We do not intend to ship something right away for the sandbox escape, but I think this week might pose a good opportunity to chat about fixing this.

Is it too late to reconsider this? I'd be pretty nervous about there just being another JIT thing lying around if we leave the sandbox escape in, esp. if we do not yet have any ideas how they found the JIT thing to begin with.

Comment 14

[Andrew McCreight [:mccr8]]

Yeah, this seems pretty bad, unless we want to land all sec fixes from now until the next release in a shadow repo, which seems unfeasible...

Comment 15

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

Based on the discussion in Slack, it sounds like we're aiming to ship this in 68 so it gets a bit of bake time first.

Updating to reflect the consensus that we want to dot release 67 for this after all.

Comment 16

[Andrew McCreight [:mccr8]]

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

Please file a sec-sensitive follow-up to audit all callers of openWindow and/or window.open in chrome-privileged JS, and another follow-up to forbid anything other than chrome: and resource: (and maybe about:) params for either.

I filed bug 1559990 for this first part, because I was looking at it anyways.

Comment 17

[Frederik Braun [:freddy]]

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

(In reply to Frederik Braun [:freddyb] from comment #4)

Gijs, I'm adding you as you seem one of the people who were lately contributing to this part of the codebase, but please feel free to redirect me as needed. This bug here was used as the sandbox escape in the chemspill release, we are just doing. We do not intend to ship something right away for the sandbox escape, but I think this week might pose a good opportunity to chat about fixing this.

Is it too late to reconsider this? I'd be pretty nervous about there just being another JIT thing lying around if we leave the sandbox escape in, esp. if we do not yet have any ideas how they found the JIT thing to begin with.

Yes :-P

Comment 18

[Johann Hofmann [:johannh]]

Comment on attachment 9072628 [details]
Bug 1559858 - Avoid unnecessary uri passing. r=Gijs

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Pretty easily, but the attacker needs a local privilege escalation on the content process first.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all of them
• 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?: Probably very easy
• How likely is this patch to cause regressions; how much testing does it need?: This is a well understood patch that is not likely to cause regressions, and even if it does those would be easy to spot and fix. QA could do some exploratory testing around opening prompts but ultimately this is a small refactoring that can't be fully verified by QA.

Comment 19

[Johann Hofmann [:johannh]]

I should mention that Gijs suggested we open up a public bug for landing this patch, to avoid raising attention. Since the patch is mostly a code cleanup I could easily add a short phrase to the new bug that suggests I would like to clean up this part of the prompting code to make it non-suspicious.

Dan, what do you think?

Comment 20

[Al Billings [:abillings - ex-MoCo]]

(In reply to Johann Hofmann [:johannh] from comment #19)

I should mention that Gijs suggested we open up a public bug for landing this patch, to avoid raising attention. Since the patch is mostly a code cleanup I could easily add a short phrase to the new bug that suggests I would like to clean up this part of the prompting code to make it non-suspicious.

Dan, what do you think?

I'm not Dan but I'd like to see this as a condition of sec-approval. I'm giving sec-approval+ for trunk but we should land this with a cover patch.
Ideally, we'd get this on Beta as well.

Comment 21

[Johann Hofmann [:johannh]]

Ok, I'll do that today when I find some time during all hands. I'll add the bug number as comment here and make sure not to link to any involved bugs.

Comment 22

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #21)

Ok, I'll do that today when I find some time during all hands. I'll add the bug number as comment here and make sure not to link to any involved bugs.

Just request review again instead of landing on inbound, if we're going to do this by the book... :)

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Based on discussion with Dan and Paul it sounds like we want to chemspill asap because Coinbase is now intending to discuss this in public. So we will aim to land this patch on m-c (maybe under a different bug), give it an hour or so to let tests run and see if builds are going ok, then uplift and do beta, esr, and release desktop builds for release tomorrow morning.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #23)

Based on discussion with Dan and Paul it sounds like we want to chemspill asap because Coinbase is now intending to discuss this in public. So we will aim to land this patch on m-c (maybe under a different bug), give it an hour or so to let tests run and see if builds are going ok, then uplift and do beta, esr, and release desktop builds for release tomorrow morning.

I've already asked them not to discuss this until we ship a fix (the sandbox portion). See the other bug.

Comment 25

[:Gijs (he/him)]

Landed through https://bugzilla.mozilla.org/show_bug.cgi?id=1560192 .

https://hg.mozilla.org/integration/autoland/rev/bfd88bfc61c4

Comment 26

[Johann Hofmann [:johannh]]

Comment on attachment 9072628 [details]
Bug 1559858 - Avoid unnecessary uri passing. r=Gijs

Beta/Release Uplift Approval Request

• User impact if declined: Sandbox escape if attacker has local privilege escalation
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: If you'd like to test this you could test all kinds of alert prompts, such as regular alert() calls in JavaScript or anywhere we spawn prompts in the browser. Note that we did a quick smoke test and they still worked fine.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a very straightforward patch that changes how JS code sends a parameter to the parent process. The risk is that it's mostly untested.
• String changes made/needed: None

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch:

Comment 27

[Pascal Chevrel:pascalc]

Comment on attachment 9072628 [details]
Bug 1559858 - Avoid unnecessary uri passing. r=Gijs

Approved for all our branches, thanks.

Comment 28

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/mozilla-central/rev/bfd88bfc61c4
https://hg.mozilla.org/releases/mozilla-beta/rev/4c21940d5185
https://hg.mozilla.org/releases/mozilla-release/rev/ea5154beddff
https://hg.mozilla.org/releases/mozilla-esr60/rev/32bd10cdfd75

Comment 29

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Adding :tmaity to CC list as part of RCA Phase II review.

Comment 30

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Requesting a root cause entered for blockers in recent releases in security groups. See :tmaity for details.