1566608 - (CVE-2022-22750) IPC passing of Windows HANDLEs and macOS Mach ports is insecure

Status: RESOLVED FIXED

(Core :: IPC, task, P2)

Description

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

This is Chromium bug 493414 from 2015; they also have a design document that goes into more details. The way they used to pass HANDLEs between processes, and what we still do, is for the sending process to do the equivalent of DuplicateHandle (brokered through the sandbox) to create a handle valid for the destination process, then send that through IPC as bits.

We also do something similar with Mach ports, basically taking Mach's primitive for atomically transferring data with attached capabilities, which is what we want, and turning it into the equivalent of DuplicateHandle, which we don't want, but unfortunately IPC was designed around it.

The problem is that, if the hostile process can guess a HANDLE value valid for the destination process, it can send that instead and potentially gain access to resources it shouldn't have. The Chromium bug gives some examples of how that could be a security problem in the context of graphics data, but there may be additional security implications for us, such as with Endpoints. (For Mach ports this affects shared memory but not Endpoint.) I don't know how guessable HANDLEs or mach_port_ts are in practice.

In the general case, we'll need to send some kind of unforgeable token in place of the HANDLE, and have both ends communicate with the trusted third party (parent / sandbox broker) to transfer the actual handle. The Chromium devs came to the same conclusion; their design document explores some variations on this theme.

In the special case case of parent→child the parent has a process handle for the destination and can DuplicateHandle for real; for child→parent the child could send its own HANDLE and have the parent extract a copy. These are optimizations and shouldn't be necessary for correctness.

In conclusion, this is a security issue whose severity is unknown but at most a complete sandbox escape (for the Windows version; maybe less severe for Mac), it's been publicly documented in another context for years now, and the best case for fixing it seems to be a major infrastructure change that we normally wouldn't want to uplift.

Comment 1

[Bob Owen (:bobowen)]

Damn, the comments on the parts I reverted seemed innocuous, so I obviously never went to look at the actual bug.
It looks like they only ever had this labelled as a site isolation issue, but I agree it is at least potentially more severe than that.

Comment 2

[Jim Mathies [:jimm]]

This represents around 6 months of effort, and involves significant changes / new code in our ipc library. The attached google doc helps define what needs to change.

Things we need to do before this gets fixed:

1. Finish up the design and research phase (Q4 2019)
2. allocate the time and resources, currently we're short here but working on it. (H1 2020)
3. Resolve a conflict of interest with the Fission team, which has requested no major changes to IPC prior to shipping Fission.

Comment 3

[Daniel Veditz [:dveditz]]

The amount of work represented here is closer to a "task" than a "defect".

Although this could result in a sandbox escape (normally sec-high), it does require "guessing" so I think it's safe to call this "moderate".

Comment 4

[Daniel Veditz [:dveditz]]

Could you just iterate through all the handles until you found one, discarding errors as you go? That probably wouldn't take too long.

Comment 5

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

To refine comment #2, there's a simpler way to deal with this bug than the full message-attachment plan: implement secure handle passing in some form — see comment #0 about unforgeable tokens — and hook it in where handles are currently serialized/deserialized. IPC channels/messages would still just be carrying bits (and Unix fds); some typedefs would have to change, but the consumers already have to be generic over Unix/Windows differences so that probably wouldn't be too difficult.

This means we'd be doing sync IPC to a broker from an actor thread (often a main thread), but we're already doing that. (Specifically we'd need to either do sync IPC on both the sending and receiving side, or else deal with the sender's async message to provide handle+token arriving after the receiver's sync message to convert the token into the translated handle; IPDL can do the former, but I don't think we expose the ability to answer sync IPC asynchronously, yet. Currently the sync IPC is just from the sender's side.)

As for Mac, I believe the Mach API allows both giving and taking port rights to/from other processes (given appropriate rights, with the parent process has for all child processes), so most of the same code could be reused, but that should be double-checked.

It's still not a trivial project, but should be easier than what I laid out in that document — at the cost of possibly worse performance, and not solving the sync/async launch problem. (In particular, we don't yet have a good story for launches during browser startup, like the WebExtension and GPU processes, other than the full handle renovation plan.)

Finally, about Fission, my understanding was that they're not opposed to this work happening (e.g., concerned about regressions) but they don't want to block on it given that the specific thing they need (asynchronous launch for a subset of content processes) can be accomplished more quickly with the infrastructure we already have.

Comment 6

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

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

Could you just iterate through all the handles until you found one, discarding errors as you go? That probably wouldn't take too long.

Probably. I haven't confirmed this experimentally yet, but I've been told by people who do Windows work that HANDLEs are typically low-entropy.

Comment 7

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

These issues are being fixed in bug 1732343 (for HANDLEs) and bug 1734735 (for mach ports).

Comment 8

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

This was fixed by the bugs mentioned in comment 7.

Comment 9

[Frederik Braun [:freddy]]

Attachment: advisory.txt