Let the utility process access memory-infra

Let the utility process access memory-infra

BUG=712753
Review-Url: https://codereview.chromium.org/2827813002
Cr-Commit-Position: refs/heads/master@{#465656}
Committed: https://chromium.googlesource.com/chromium/src/+/28e11150e03e8d2bd8064fe1fa206e647c1b0030

[chiniforooshan, 2017-04-18 22:21:41]

chiniforooshan@chromium.org changed reviewers:
+ tsepez@chromium.org

[chiniforooshan, 2017-04-18 22:21:41]

ptal

[chiniforooshan, 2017-04-18 22:23:35]

chiniforooshan@chromium.org changed reviewers:
+ nasko@chromium.org
- tsepez@chromium.org

[chiniforooshan, 2017-04-18 22:23:36]

Replacing Tom, who is OOO today and tomorrow, with Nasko.

[Tom Sepez, 2017-04-18 22:24:43]

On 2017/04/18 22:21:41, chiniforooshan wrote:
> ptal

Can you fill me in on what memory-infra does, and why we think its safe to let
the untrusted utility process access it?

[chiniforooshan, 2017-04-18 22:46:24]

On 2017/04/18 22:24:43, Tom Sepez (OOO Wed - Thu) wrote:
> On 2017/04/18 22:21:41, chiniforooshan wrote:
> > ptal
> 
> Can you fill me in on what memory-infra does, and why we think its safe to let
> the untrusted utility process access it?

Processes register themselves to memory_instrumentation::Coordinator. Then, when
a registered process triggers a global memory dump request the coordinator asks
all registered processes to dump their memory usage information to their trace
buffers.

I guess the worst thing an untrusted process can do is to trigger global memory
dump requests unnecessarily. If tracing is enabled and the memory-infra category
is also enabled, this will result filling the trace buffer unnecessarily.
+Primiano: do you know if this was allowed before the mojofication of
memory-infra? I don't remember seeing any logic to prevent this from a utility
process before.

[chiniforooshan, 2017-04-18 22:48:00]

chiniforooshan@chromium.org changed reviewers:
+ primiano@chromium.org, tsepez@chromium.org

[chiniforooshan, 2017-04-18 22:48:00]

+Tom +Primiano

[chiniforooshan, 2017-04-19 14:34:33]

On 2017/04/18 22:46:24, chiniforooshan wrote:
> On 2017/04/18 22:24:43, Tom Sepez (OOO Wed - Thu) wrote:
> > On 2017/04/18 22:21:41, chiniforooshan wrote:
> > > ptal
> > 
> > Can you fill me in on what memory-infra does, and why we think its safe to
let
> > the untrusted utility process access it?
> 
> Processes register themselves to memory_instrumentation::Coordinator. Then,
when
> a registered process triggers a global memory dump request the coordinator
asks
> all registered processes to dump their memory usage information to their trace
> buffers.
> 
> I guess the worst thing an untrusted process can do is to trigger global
memory
> dump requests unnecessarily. If tracing is enabled and the memory-infra
category
> is also enabled, this will result filling the trace buffer unnecessarily.
> +Primiano: do you know if this was allowed before the mojofication of
> memory-infra? I don't remember seeing any logic to prevent this from a utility
> process before.

To add more context:

In old days, before https://codereview.chromium.org/2741203002,
memory_instrumentation::Coordinator (which is in the browser process) was part
of the tracing controller (which is also in the browser process). In that CL, we
moved out memory related IPCs (like this one:
https://cs.chromium.org/chromium/src/content/browser/tracing/trace_message_fi...)
out of tracing controller into memory_instrumentation::Coordinator.

We did not change the behaviour, at least that was not our intention.

Before the CL, the utility process could send an IPC to the browser requesting a
global memory dump. We just changed that IPC to Mojo. The child process tries to
connect to memory_instrumentation::Coordinator in the same place that it adds
the IPC filter for tracing: in ChildThreadImpl::Init.

Given that this CL is just trying to go back to the old behaviour (i.e. before
the Mojofication CL, linked above) that lets the utility process send and
receive memory-infra messages, do you think there is still a security concern?

[Tom Sepez, 2017-04-19 16:22:14]

lgtm

[Primiano Tucci (use gerrit), 2017-04-19 16:37:14]

lgtm

[chiniforooshan, 2017-04-19 16:39:17]

The CQ bit was checked by chiniforooshan@chromium.org

[commit-bot: I haz the power, 2017-04-19 16:39:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 17:35:03]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1492619957386770, "parent_rev":
"942ea5dfadaf084f35cd8a8988809c5c9cfc410c", "commit_rev":
"28e11150e03e8d2bd8064fe1fa206e647c1b0030"}

[commit-bot: I haz the power, 2017-04-19 17:35:14]

Description was changed from

==========
Let the utility process access memory-infra

BUG=712753
==========

to

==========
Let the utility process access memory-infra

BUG=712753

Review-Url: https://codereview.chromium.org/2827813002
Cr-Commit-Position: refs/heads/master@{#465656}
Committed:
https://chromium.googlesource.com/chromium/src/+/28e11150e03e8d2bd8064fe1fa20...
==========

[commit-bot: I haz the power, 2017-04-19 17:35:15]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/28e11150e03e8d2bd8064fe1fa20...