1565744 - MemMapSnapshot can be written by a malicious child process

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

MemMapSnapshot, from bug 1463587, creates an unsecured (and, on older Windows versions, unsecurable due to being unnamed) object for its shared memory, so a process receiving a read-only handle to it can use DuplicateHandle to gain write access and attack other processes.

The patches currently posted to bug 1479960, which move this functionality into base::SharedMemory, have the same problem.

Specifically, SharedStringMap is used with MemMapSnapshot to store prefs, so it's possible that this bug could be used to escalate from a compromised Web content process to a more privileged child process, possibly including the unsandboxed GPU process. (It's also easily usable for out-of-bounds reads.)

Chrome's version of this bug is CVE-2014-3196 and their bug 338538; our exposure to it was reported as part of bug 1564979.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Do we need to care about this comment about WRITE_DAC? The upstream Chromium version postdates some major changes to how they deal with handle passing, so I'm not clear on how it fits into our current situation.

Comment 2

[Bob Owen (:bobowen)]

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #1)

Do we need to care about this comment about WRITE_DAC? The upstream Chromium version postdates some major changes to how they deal with handle passing, so I'm not clear on how it fits into our current situation.

As far as I can see, the duplication code in the broker (which we still use and I have to add back in) doesn't check for the WRITE_DAC permission and block it and didn't at the time that comment was added, so I'm not quite sure what they're talking about here.
However, it seems that using something like CreateFileMappingWithReducedPermissions to remove these permissions wouldn't hurt.
Presumably we only want to duplicate a handle into the other process with the least permissions possible.

Comment 3

[Kris Maglione [:kmag]]

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #0)

Specifically, SharedStringMap is used with MemMapSnapshot to store prefs, so it's possible that this bug could be used to escalate from a compromised Web content process to a more privileged child process, possibly including the unsandboxed GPU process. (It's also easily usable for out-of-bounds reads.)

Just to be 100% accurate, shared preferences don't use SharedStringMap. They use something similar, but somewhat more intricate, that's still backed by MemMapSnapshot.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

It looks like we already remove WRITE_DAC permission when we create the read-only handle, and also in SharedMemory::ShareToProcessCommon. The places where we use DUPLICATE_SAME_ACCESS, on the other hand, we seem to intend that all holders of a handle have full access to the object, so that shouldn't be a problem.

I also tried using GetSecurityInfo+SetSecurityInfo to read and write the DACL with no changes, with a patch to use an empty DACL when creating shared memory, and setting it failed with ERROR_ACCESS_DENIED.

Interestingly, the Freeze implementations ought to remove SECTION_QUERY if I understand correctly — they duplicate with GENERIC_READ | FILE_MAP_READ — but re-duplicating to add it (as in ShareToProcessCommon) seems to work even when the object is secured.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I've confirmed that on Windows 10 passing security attributes with an empty DACL is sufficient, and that on Windows 7 the requested security attributes are silently ignored unless there's also a name.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1565744.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Proof-of-concept patch. DO NOT LAND.

For testing, I used this patch which adds a release assertion that fails if the “read-only” handle can have write permissions restored.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 9080674 [details]
Bug 1565744.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch unavoidably makes it obvious (to someone who knows Windows security) what's going on, and gaining the write access is trivial. Using that write access to usefully interfere with another process is harder (and not directly pointed out by the patch), but I predict that a good exploit dev could figure it out.

There are no comments or tests in this patch, but this code will be refactored in bug 1479960, where I am adding unit tests, and I'd like to also add comments because what's going on here isn't obvious to non-experts; however, that might not make things any worse than they already are. I can file a shadow bug for security review of those patches if need be.

• 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?: release, beta, esr68
• If not all supported branches, which bug introduced the flaw?: Bug 1463587
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The patch appears to trivially apply to the affected branches; I haven't tested it yet.
• How likely is this patch to cause regressions; how much testing does it need?: There is some risk that adding security attributes and names to these objects could cause breakage in some unusual configuration (cf. some of the Windows sandboxing bugs). However, Chrome has been doing this for years with no problems. Note that this code is exercised simply by starting the browser, so our regular QA and automated tests should cover it.

Comment 9

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

Sec-approval+ for landing on August 6 or after to avoid some of the exposure time. We'll want this on Beta and ESR68 as well...

Comment 10

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

https://hg.mozilla.org/integration/autoland/rev/25ee714cbf2b80245d0b1e55dc885c59aba4b8a6
https://hg.mozilla.org/mozilla-central/rev/25ee714cbf2b

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 9080674 [details]
Bug 1565744.

Beta/Release Uplift Approval Request

• User impact if declined: Exposure to this security bug, which could potentially be used for a sandbox escape.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: The closest thing I have to STR is to apply the “proof-of-concept patch” attached to this bug, which attempts the attack and release-asserts that it fails. Without this fix, that assertion will fire early in startup. I've done this test locally, so I don't need QE help here.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This does change the access control metadata for relatively important objects, and the Windows security model is complicated, but the patch has passed the test suite and spent several days on Nightly, so any breakage would likely have been caught. We're also doing the same thing that Chrome has successfully done for years.
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is sec-high.
• User impact if declined: Exposure to this security bug, which could potentially be used for a sandbox escape.
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): See beta request.
• String or UUID changes made by this patch: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9080674 [details]
Bug 1565744.

Fixes a possible sandbox escape. Approved for 69.0b13 and 68.1esr.

Comment 13

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

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/16ef2c272cbf