1789440 - MessageChannel IPC reply potential type confusion

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

The code in MessageChannel which handles callbacks holds a map from seqnos to type-erased callback holders, however it doesn't hold the type of the callback itself anywhere. Instead, when a reply message is received, the handler pops out the callback holder (https://searchfox.org/mozilla-central/rev/3f9dcc016dd96a0336d46f4a19aeabdd796ab9e9/ipc/glue/MessageChannel.cpp#788-799) and then static_casts it to the response type without checks in the generated code (https://searchfox.org/mozilla-central/rev/3f9dcc016dd96a0336d46f4a19aeabdd796ab9e9/ipc/ipdl/ipdl/lower.py#4855), meaning that if a response is received with the wrong type but with the correct seqno, the type of the callback holder could be confused, leading to all sorts of potential issues.

We should make sure to actually check the message type which the callback is registered for.

Comment 1

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: probably sec-high or worse

Nika found this after seeing a patch for the recent Chrome Mojo zero day, so we should probably fix this sooner rather than later.

Comment 2

[Andrew McCreight [:mccr8]]

It sounds like this is a sandbox escape from a compromised content process, so I'll mark it sec-high, partially because it is a close variant of a Chrome zero day that has been patched recently.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

tracking+ for this cycle, but note that we're a week from going to RC so we'll need a pretty fast fix here.

Comment 4

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

Attachment: Bug 1789440 - Track reply message IDs for MessageChannel async replies, r=#ipc-reviewers

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: patch for objdir/ipc/ipdl/PContentChild.cpp DO NOT LAND

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Bug 1789440 - Demo. DO NOT LAND

Comment 7

[Andrew McCreight [:mccr8]]

I wanted to make sure I understood the problem Nika found, so I put together a demonstration of her idea. The first part is a patch to the source code, then the second one, in the attachment here, is a patch for the generated IPDL output because I was too lazy to change the code generator.

First, this patch adds a call to send a CollectPerfStatsJSON message (async CollectPerfStatsJSON() returns (nsCString aStats)) right before we send a RequestMemoryReport message (which has returns (uint32_t aGeneration)). It also adds a trivial use of the string that the child sends us in a callback. This is effectively requiring cooperation from the parent process, but mostly I do this to make it easier to trigger. If the content process changes were more sophisticated, the only real requirement is that the parent is waiting for two async replies at the same time where one reply is an integer and the other is a pointer. The child controls when the reply happens, so it shouldn't be that hard.

Next, this patch modifies the code in MemoryReportRequest.cpp that runs in the child process and does the reply. It just changes the return value to 0xDEADBEEF so it is easier to see when the demo worked.

In ContentChild.cpp, the patch adds a pair of getter/setter methods (MagicSaveSeqno and GetMagicSaveSeqno) which are used in the patch to the generated code. The rest of the changes in this file just modify ContentChild::RecvCollectPerfStatsJSON() so that it leaks the resolver, to prevent the child process from sending back some kind of default failure reply for CollectPerfStatsJSON.

Finally, the attached diff to PContentChild.cpp just saves the seqno when we get the CollectPerfStatsJSON message, and then uses that saved value for the RequestMemoryReport reply.

Once you have it all built the actual steps to reproduce are:

1. start the browser and load any web page (I used example.com).
2. open about:memory and click "measure".

The browser then crashes when trying to print out a string in the CollectPerfStatsJSON reply callback. On my OSX machine, the crash report includes this, indicating the child process has successfully caused type confusion in the parent:

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x00000000deadbee0

Comment 8

[Andrew McCreight [:mccr8]]

Decoder, you might be interested in seeing if there's some way IPC fuzzing could detect an issue like this. On some level, it just requires messing with the seqno, but you have to be in the right situation where two async messages are waiting for replies at the same time.

Comment 9

[Andrew McCreight [:mccr8]]

Here's the diff for the patch for the Chrome zero day I linked to earlier.

Comment 10

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

Comment on attachment 9293321 [details]
Bug 1789440 - Track reply message IDs for MessageChannel async replies, r=#ipc-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Requires a compromised content process, and then manipulation of state in the parent process over IPC.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• 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?: I expect that this patch will land directly on all impacted branches with minimal modifications, however adapting the patch shouldn't be difficult if I am wrong about that.
• How likely is this patch to cause regressions; how much testing does it need?: This patch should be very unlikely to cause regressions - it just adds a few more checks.
• Is Android affected?: Yes

Beta/Release Uplift Approval Request

• User impact if declined: potential content process sandbox escape (type confusion in parent process using IPC)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch should be very unlikely to cause regressions - it just adds a few more checks.
• 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: sec-high
• User impact if declined: potential content process sandbox escape (type confusion in parent process using IPC)
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch should be very unlikely to cause regressions - it just adds a few more checks.

Comment 11

[Christian Holler (:decoder)]

(In reply to Andrew McCreight [:mccr8] from comment #8)

Decoder, you might be interested in seeing if there's some way IPC fuzzing could detect an issue like this. On some level, it just requires messing with the seqno, but you have to be in the right situation where two async messages are waiting for replies at the same time.

So the parent process has to be waiting for two async messages to be replied to? I am not sure how that is easily possible in current IPC fuzzing, as we are initiating the fuzzing at a point where the parent is mostly idle. Can the child do things that make the parent send such async messages that wait for a reply? Or when does that usually happen?

Also, the fuzzer makes sure that sequence numbers are always correct for the packages that are sent out, but I guess that's fine for the problem here?

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 9293321 [details]
Bug 1789440 - Track reply message IDs for MessageChannel async replies, r=#ipc-reviewers

Approved to land (and uplift, we should do both.)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9293321 [details]
Bug 1789440 - Track reply message IDs for MessageChannel async replies, r=#ipc-reviewers

Not into a dot release, though :-)

Comment 14

[Andrew McCreight [:mccr8]]

(In reply to Christian Holler (:decoder) from comment #11)

So the parent process has to be waiting for two async messages to be replied to? I am not sure how that is easily possible in current IPC fuzzing, as we are initiating the fuzzing at a point where the parent is mostly idle.

Yes.

Can the child do things that make the parent send such async messages that wait for a reply? Or when does that usually happen?

I'm not sure. Maybe I could try adding some logging when there is more than one async message up at once.

Also, the fuzzer makes sure that sequence numbers are always correct for the packages that are sent out, but I guess that's fine for the problem here?

The seqno has to be valid, but it just comes from another message that is being waited on at the same time.

Comment 15

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

Track reply message IDs for MessageChannel async replies, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/82eeb083f83087b57d0dca4c7094661f0bca7324
https://hg.mozilla.org/mozilla-central/rev/82eeb083f830

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9293321 [details]
Bug 1789440 - Track reply message IDs for MessageChannel async replies, r=#ipc-reviewers

Approved for 105.0b9 and 102.3esr.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/32ad35a64cdc

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/c9f28f6ce836

Comment 19

[Andrew McCreight [:mccr8]]

Nika pointed out that the condition for triggering this issue is easier than I thought: it only requires the parent to be waiting for a single reply, because it was not checking that the type of the message it is getting is something we're actually waiting for, so you can just send any message that is valid with respect to the message type.