958895 - [meta] Import upstream chromium security fixes into our IPC implementation

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Christoph Diehl [:posidron]]

In the past there have been a couple of critical security issues inside Google's IPC stack. Those issues got fixed at Google but it seems that we never imported to fixes into our tree.

Example:

"IPC Channel does not validate the listener."
http://code.google.com/p/chromium/issues/detail?id=117627

Comment 1

[Benjamin Smedberg]

Maybe I'm not reading the bug carefully enough, but this doesn't really look like a security issue that needs to remain hidden. We assume that if you're running a process on the machine, you're already capable of taking over.

Comment 2

[Christoph Diehl [:posidron]]

I marked this bug as hidden because we haven't implemented those patches - I felt this was the right way.
The particular bug in the IPC code was used in a multi-chained exploit to bypass the Google Chrome's Sandbox, demonstrated in the Pwn2Own challenge at CanSecWest in 2012.

Comment 3

[Benjamin Smedberg]

Sure, but since we currently don't have a sandbox this isn't actually an exploit. Can we just open it up and fix it as a normal bug?

Comment 4

[Christoph Diehl [:posidron]]

We recently implemented the Sandbox in this bug https://bugzilla.mozilla.org/show_bug.cgi?id=922756

Comment 5

[Benjamin Smedberg]

AFAIK that's not in any shipping product. Keeping this bug closed is not protecting any users and just makes finding volunteer fixes harder.

Comment 6

[Christoph Diehl [:posidron]]

Okay, then let's open this bug.

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> Sure, but since we currently don't have a sandbox this isn't actually an exploit.

The security model for Firefox OS is heavily reliant on the IPC code.

Comment 8

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

On Firefox OS, each content process runs as a separate non-root uid.  If this can allow a content process to run arbitrary code as another content process — or, worse, as the uid 0 parent process — then I think it would be at least sec-moderate, and would apply to FxOS version 1.0 and up.

Also, there is work in progress to sandbox GeckoMediaPlugin processes, which use the core IPC framework, so this would affect them as well — including EME CDMs, once that happens.

Comment 9

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

Two months later...Critsmash is wondering if we're going to get these upstream fixes.

Comment 10

[Doug Turner (:dougt)]

brad, do you know much about this?  How far have we drifted from google's ipc and what do we need to fix?

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

We have drifted, but somewhat intentionally. I'd recommend opening individual bugs for each issue so we can track and evaluate them separately.

Comment 12

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

Adding the other two sandbox module owners, because generally an IPC security bug is a sandbox escape.

Comment 13

[Andrew McCreight [:mccr8]]

Jed, is this something you could work at, or at least start on?  Thanks.

Comment 14

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

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #11)
> We have drifted, but somewhat intentionally. I'd recommend opening
> individual bugs for each issue so we can track and evaluate them separately.

A prerequisite for doing that is enumerating those issues.  Comment #0 is one; there are probably others.  This may require going through every Chrome security advisory since the branch point.

That branch point, for reference, was on 2009-05-06:

https://github.com/mozilla/gecko-dev/commit/a64afe22b9cae676a20c4d72776922ca6756648c
https://hg.mozilla.org/mozilla-central/rev/6fd4bb500d42 (tag chromium-import-r15462)

https://src.chromium.org/viewvc/chrome/trunk/src/?pathrev=15462
https://chromium.googlesource.com/chromium/src/+/901065b013787109038f232c0707b245af796599

Comment 15

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

I agree with comment #11: we should have separate bugs for each actual vulnerability.

Comment 16

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

(In reply to Jed Davis [:jld] from comment #14)
> This may require going through every Chrome security advisory since the branch point.

Or just getting a list of all the public Chromium bugs with CVE tags (848 at the time of this writing):

  https://code.google.com/p/chromium/issues/list?can=1&q=label%3ACVE&mode=grid&y=&x=&cells=ids

From which it's a copy/paste and a few CPU-seconds to search the Chromium commit history for the "ipc" directory[*]:

  git log -E --grep "$(printf 'BUG=(.*[^0-9])?(%s)($|[^0-9])' $(tr ' ' '|' < .../bugs.txt))" ipc

In addition to CVE-2011-3079 cited in comment #0, we have:

CVE-2013-0830, https://crbug.com/162066, “LOGFONT IPC deserializer doesn't require NULL terminated lfFaceName”; rated low severity, Windows-specific, uncertain exploitability.  Also, I don't know if we ever pass a LOGFONT over IPC. 

CVE-2013-0842, https://crbug.com/166867, “ReferencesParent bypass with a 0x00 byte”.  I think we're not passing Chromium FilePaths over IPC, in which case we're not affected.  Regardless, we should be careful about similar null-termination issues with untrusted strings.

CVE-2013-2854, https://crbug.com/243339, “CheckDuplicateHandle (BreakDebugger) browser crash with (Web) Workers and WebSQL”.  The vulnerable code postdates our fork, and mozilla::ipc::FileDescriptor looks like it avoids the same bug (calling DuplicateHandle with ((HANDLE)-1) instead of a file handle).

The patches for the two null-termination issues look backportable despite the divergence.

There are also a lot of Chromium bugs that are marked as security-related but weren't assigned CVEs, and other parts of the Chromium source that could be relevant.


[*] The code that's now in ipc/ was in chrome/common/ before chromium r21342 (git 946d1b2c8), but no CVE-tagged bugs were found in that part of the history.

Comment 17

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

https://crbug.com/117341, “Heap-use-after-free in MessageLoop::AddToIncomingQueue”.  Our IPC fork is too old to be affected (bug added in chromium r124272 if I understand correctly).

https://crbug.com/43304, “Linux sandbox escape”, “POSIX: make sure that we never pass directory descriptors into the sandbox”.  On Linux, I believe we're safe as long as we disallow the *at syscalls (and fchdir) with seccomp-bpf; also, we can't yet avoid content processes' needing to get directory descriptors in some cases (bug 1078971).  I don't know if there are implications for our Mac sandboxing.

https://crbug.com/31364, “problems calling resize() on vectors with no sanitization”.  Integer overflows (and maybe uncaught allocation failures?) related to vector lengths passed over IPC.  If we don't pass std::vector over IPC then we're not affected, and the ParamTraits for nsTArray looks like it does the right thing.

https://crbug.com/22451, “Use-after-free in IPC::Channel::ChannelImpl::ProcessOutgoingMessages()”.  This could be bad.

Comment 18

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

A few more found by checking the paths in ipc/chromium/moz.build and trying to adjust them for moves that have happened since our fork.

CVE-2014-3196, https://crbug.com/338538, “Security: Windows Sandbox Anonymous Kernel Object Unrestricted DACL”.  Unsure if it affects us — the case they were concerned about was passing read-only shared-memory handles (cf. https://crbug.com/302724 and possibly others), where the child could use this bug to gain write access.


https://crbug.com/312250, “Access after the end of the buffer due to undefined behavior in Pickle::FindNext”.  Unsure if it affects us.

https://crbug.com/70376, “Pickle::FindNext reads payload_size without checking that the header is complete”.  Could be serious.

https://crbug.com/56449, “Pickle: handle invalid data on 64 bit systems.”  Could be serious.


https://crbug.com/177956, “cross-process memory address leak via sa_restorer”.  Actually a Linux kernel bug; this is a workaround.  Lowish severity — allows defeating the parent's ASLR if the child is already compromised.  (And therefore meaningless on B2G, because Nuwa is forked from the parent now.)


There are also a number of bugs about the Chromium base/ code for filesystem paths — pathname traversals and similar, usable to exploit broker code in the parent.  I'm assuming we're not using this copy of that code in that way.

Comment 19

[Andrew McCreight [:mccr8]]

Thanks for looking into this, Jed!  I'll assign it to you so we can track that somebody is keeping an eye on it.

Comment 20

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

I have some data on a few of these.  We're not using the ParamTraits for LOGFONT or FilePath; I can delete them and everything still compiles.  std::vector is used for mozilla::plugin::IPCByteRanges, but that's easy to convert to nsTArray and NPAPI isn't a security boundary.  std::vector is also used by Chromium's IPC::Logging class; it looks like we might not be using it, but I'm not sure about that.

Relevant try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=44cde22217e0

Comment 21

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

Question n+1, for an IPC peer: Are we using IPC::Logging / IPC_MESSAGE_LOG_ENABLED, or can that be removed?

Comment 22

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

(In reply to Jed Davis [:jld] from comment #18)
> CVE-2014-3196, https://crbug.com/338538, “Security: Windows Sandbox
> Anonymous Kernel Object Unrestricted DACL”.  Unsure if it affects us — the
> case they were concerned about was passing read-only shared-memory handles
> (cf. https://crbug.com/302724 and possibly others), where the child could
> use this bug to gain write access.

Question n+2, for someone who knows Windows security issues better than I do: Do we need to care about this bug, or are we not using anonymous shared memory in that way?

Comment 23

[Bob Owen (:bobowen)]

(In reply to Jed Davis [:jld] from comment #22)
> (In reply to Jed Davis [:jld] from comment #18)
> > CVE-2014-3196, https://crbug.com/338538, “Security: Windows Sandbox
> > Anonymous Kernel Object Unrestricted DACL”.  Unsure if it affects us — the
> > case they were concerned about was passing read-only shared-memory handles
> > (cf. https://crbug.com/302724 and possibly others), where the child could
> > use this bug to gain write access.
> 
> Question n+2, for someone who knows Windows security issues better than I
> do: Do we need to care about this bug, or are we not using anonymous shared
> memory in that way?

I am certainly no expert in this area, but as far as the sandbox code is concerned, I think it only uses shared memory for IPC and to hold the policy see [1].
Looks like this is anonymous, but as far as I can tell it is only used read/write anyway.
Certainly, the current Chromium code is the same.
We don't have a copy of shared_memory_win.cc in the Chromium sandboxing code.

I don't know if we are using any read only anonymous shared memory (etc.) in the e10s code, Jim Mathies might know.
I think there are plans to use shared memory for the media related things, but I don't know if it will fall foul of this issue.

Maybe we should take the fix anyway, if it's fairly straight forward.
At first glance, looks like they are just creating a random name when read only and there isn't a name already.

[1] http://dxr.mozilla.org/mozilla-central/source/security/sandbox/win/src/target_process.cc#255

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I don't think we use IPC::Logging or IPC_MESSAGE_LOG_ENABLED. We have our own logging mechanism (MOZ_IPC_MESSAGE_LOG).

Comment 25

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

(In reply to Jed Davis [:jld] from comment #18)
> https://crbug.com/177956, “cross-process memory address leak via
> sa_restorer”.  Actually a Linux kernel bug; this is a workaround.

This is fixed in kernel 3.9, inapplicable to B2G as mentioned, and a relatively minor issue to begin with; I don't think implementing the workaround is worth it.

(In reply to Jed Davis [:jld] from comment #18)
> CVE-2014-3196, https://crbug.com/338538, “Security: Windows Sandbox
> Anonymous Kernel Object Unrestricted DACL”.

Previous comments indicate this doesn't affect us — and, if it does, it's probably not because of ipc/chromium/src.

Between those and the bugs I've filed, this just leaves the ParamTraits bugs mentioned in comment #20 — which are either dead code or not security-sensitive.

Comment 26

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

The IPC::Logging thing might be exploitable on debug builds — it looks like the child can send an unsolicited message of type IPC_LOGGING_ID, and then if the first word of that message overflows int when multiplied by sizeof(LogData), heap buffer overrun.

But this is debug-only — non-debug builds #define NDEBUG, which disables IPC_MESSAGE_LOG_ENABLED, so we don't compile the affected code in that case.

I filed bug 1111810 for the IPC::Logging removal publicly before I realized it had security implications; it's not linked to this bug or otherwise obviously security-related.  I don't know if it needs to be security-flagged, given that it's debug-only.

Comment 27

[Andrew McCreight [:mccr8]]

Exploitable code that is only in debug builds is not a security issue.

Comment 28

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

Bug 1112747 now exists for the NPAPI plugin usage of ParamTraits<vector<T>>.

Comment 29

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Bob, I know you have done a bunch of work updating our IPC code - I assume this bug can probably be closed now, but I thought I should check with you first?

Comment 30

[Bob Owen (:bobowen)]

(In reply to Paul Theriault [:pauljt] from comment #29)
> Bob, I know you have done a bunch of work updating our IPC code - I assume
> this bug can probably be closed now, but I thought I should check with you
> first?

I did some of the patching, but it was jld that did the initial analysis, so he's probably better placed to decide if this bug should be closed.

In theory we should be getting advanced warning of bug reports now (monitored by dveditz and ckerschb as part of the normal sec triage).
I'm not sure if that process is working (I spoke to dveditz briefly in Hawaii, but he hadn't had chance to look at any reports that the access gives him by then).

I suppose that we might want to look again at anything reported since the first analysis and when we got that access either way.
The chances of finding something new that still applies to our IPC code are probably relatively small, given how much our code has diverged from theirs.

Comment 31

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

I repeated the procedure from comment #16, and I haven't found anything new in ipc or base.  So if we now have a better way to stay informed about these than scraping their public bugs and commit history, then I think this bug can be closed.

Comment 32

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Close?