1497749 - (CVE-2018-18505) IPC channels created via Endpoint passing don't authenticate the client

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

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

On Windows we implement IPC channels using named pipes: the server (parent) side creates a pipe and then the client (child) side connects to it by name. 

Bug 1087565 / CVE-2011-3079 dealt with the possibility of a sandboxed process connecting to a channel meant for another process and impersonating it to achieve privilege escalation, by adding a 32-bit shared secret that the client must send to the server.  (According to comments in the upstream patch, the pipe namespace can be enumerated even by low-privileged processes.)

However, this works correctly only for the initial channel created when the process is started; additional channels created by passing Endpoint instances over IPC don't do this.

The Endpoint-bound channels use a different ChannelImpl constructor ([1] vs. [2]); it behaves similarly in the client, but in the server it doesn't initialize the shared secret, which means[3] that it silently ignores the client's secret and doesn't verify it.

This may predate bug 1223240, which added the Endpoint system: that ChannelImpl constructor goes back to bug 564086 (part j in the patch series), and it's not in upstream Chromium, so this could be something that was overlooked during the crossport in bug 1087565.

I haven't constructed a proof-of-concept to verify this; I discovered it by accident while doing Try runs with extra logging to investigate bug 1368036.  It should be relatively easy to make the client send a bad secret (or none) for this type of channel, but I don't currently have a Windows development environment, and a patch like that probably shouldn't be pushed to a public repo like Try.


[1] https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/ipc/chromium/src/chrome/common/ipc_channel_win.cc#64-82
[2] https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/ipc/chromium/src/chrome/common/ipc_channel_win.cc#48-62
[3] https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/ipc/chromium/src/chrome/common/ipc_channel_win.cc#197

Comment 1

[Bob Owen (:bobowen)]

I couldn't see where this was using the other constructor at first, but it's when the TransportDescriptor is opened at [1].

On the face of it this looks fairly easy to fix.

[1] https://searchfox.org/mozilla-central/rev/e22c0d152060f4f8d4ca8904094f15f65a1b6f93/ipc/glue/Transport_win.cpp#82

Comment 2

[Bob Owen (:bobowen)]

Attachment: Get secret from channel ID when pipe handle passed.

This gets the shared secret from the channel ID even when we don't need the pipe name.
I tested that it was checking the secret by incremening it, which caused the message from the NOTREACHED() after the secret check and killed the child.

Comment 3

[Bob Owen (:bobowen)]

Comment on attachment 9028909 [details] [diff] [review]
Get secret from channel ID when pipe handle passed.

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: The issue that this was trying to address was a theoretical one in the original chromium bug, I don't think they had anything close to a PoC.
However I think it is relatively obvious from the patch what is being fixed, assuming you understand the original problem.

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

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?: This code gets used for all channels set up through Endpoints, so just running firefox exercises it multiple times.
The change is very simple and once the shared secret is extracted from the channel ID it is checked by the same code used for the main channel.

Comment 4

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

If it matters for CVE assignment or release notes or something like that: what's going on here is that our original fix for CVE-2011-3079 was incomplete, because of how our fork had diverged from Chromium, and this corrects that.

Comment 5

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

Comment on attachment 9028909 [details] [diff] [review]
Get secret from channel ID when pipe handle passed.

sec-approval+ for trunk. We'll want Beta and ESR60 patches made and nominated.

Comment 6

[Bob Owen (:bobowen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d060df6e3258ed74b800c7ee0e89e001116f98d

Comment 7

[Bob Owen (:bobowen)]

Comment on attachment 9028909 [details] [diff] [review]
Get secret from channel ID when pipe handle passed.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Chromium IPC

User impact if declined: Theoretical attack on IPC pipes from already compromised process.

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 code gets used for all channels set up through Endpoints, so just running firefox exercises it multiple times.
The change is very simple and once the shared secret is extracted from the channel ID it is checked by the same code used for the main channel.

String changes made/needed: None

[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: Theoretical attack on IPC pipes from already compromised process.

Fix Landed on Version: 66

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This code gets used for all channels set up through Endpoints, so just running firefox exercises it multiple times.
The change is very simple and once the shared secret is extracted from the channel ID it is checked by the same code used for the main channel.

String or UUID changes made by this patch: NOne

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/3d060df6e325

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9028909 [details] [diff] [review]
Get secret from channel ID when pipe handle passed.

[Triage Comment]
Hardens our IPC pipes in the event of a compromised content process. Approved for 65.0b6 and 60.5.0esr.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/f46f8873c19e