Closed
Bug 1497749
(CVE-2018-18505)
Opened 6 years ago
Closed 6 years ago
IPC channels created via Endpoint passing don't authenticate the client
Categories
(Core :: IPC, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla66
People
(Reporter: jld, Assigned: bobowen)
References
(Blocks 1 open bug)
Details
(Keywords: csectype-priv-escalation, sec-high, Whiteboard: [post-critsmash-triage][adv-main65+][adv-esr60.5+])
Attachments
(1 file)
(deleted),
patch
|
jld
:
review+
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr60+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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
Updated•6 years ago
|
Keywords: csectype-priv-escalation,
sec-high
![]() |
||
Updated•6 years ago
|
Flags: needinfo?(bobowencode)
Assignee | ||
Comment 1•6 years ago
|
||
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
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Flags: needinfo?(bobowencode)
Priority: -- → P1
Assignee | ||
Comment 2•6 years ago
|
||
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.
Attachment #9028909 -
Flags: review?(jld)
Reporter | ||
Updated•6 years ago
|
Attachment #9028909 -
Flags: review?(jld) → review+
Assignee | ||
Comment 3•6 years ago
|
||
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.
Attachment #9028909 -
Flags: sec-approval?
Reporter | ||
Comment 4•6 years ago
|
||
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•6 years ago
|
||
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.
Attachment #9028909 -
Flags: sec-approval? → sec-approval+
Updated•6 years ago
|
status-firefox64:
--- → wontfix
status-firefox65:
--- → affected
status-firefox66:
--- → affected
status-firefox-esr60:
--- → affected
tracking-firefox65:
--- → +
tracking-firefox66:
--- → +
tracking-firefox-esr60:
--- → 65+
Assignee | ||
Comment 6•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d060df6e3258ed74b800c7ee0e89e001116f98d
Assignee | ||
Comment 7•6 years ago
|
||
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
Attachment #9028909 -
Flags: approval-mozilla-esr60?
Attachment #9028909 -
Flags: approval-mozilla-beta?
![]() |
||
Comment 8•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3d060df6e325
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Comment 9•6 years ago
|
||
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.
Attachment #9028909 -
Flags: approval-mozilla-esr60?
Attachment #9028909 -
Flags: approval-mozilla-esr60+
Attachment #9028909 -
Flags: approval-mozilla-beta?
Attachment #9028909 -
Flags: approval-mozilla-beta+
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•6 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main65+][adv-esr60.5+]
Updated•6 years ago
|
Alias: CVE-2018-18505
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•