Make shared memory segment writable only by its rightful owner.

Make shared memory segment writable only by its rightful owner.

BUG=143846
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155546

[palmer, 2012-08-21 01:17:36]

PTAL

[Jamie, 2012-08-21 01:42:57]

lgtm

[Jamie, 2012-08-22 21:28:36]

On 2012/08/21 01:42:57, Jamie wrote:
> lgtm

Responding to your ping about how to test this, your best bet is to verify that
the permissions are correct using the approach outlined in the bug. If so, then
assign it to be to verify that the capture still works (or if you're familiar
with how to set up a host and connect to it, that will probably suffice.)

[palmer, 2012-09-06 22:26:28]

> Responding to your ping about how to test this, your best bet is to verify
that
> the permissions are correct using the approach outlined in the bug. If so,
then
> assign it to be to verify that the capture still works (or if you're familiar
> with how to set up a host and connect to it, that will probably suffice.)

I think there must also be a call to create a 0666 shm segment somewhere in the
extension code, because I still get one. However, it is only when I actually try
to Share my desktop — previously, I got a 0666 shm segment just because I
installed the extension. So I think this CL somewhat improves the situation, but
does not completely solve the problem.

Jamie, can I get a look at the extension code or can you look into it for me?
Thanks.

[Jamie, 2012-09-06 22:32:34]

On 2012/09/06 22:26:28, Chris P. wrote:
> > Responding to your ping about how to test this, your best bet is to verify
> that
> > the permissions are correct using the approach outlined in the bug. If so,
> then
> > assign it to be to verify that the capture still works (or if you're
familiar
> > with how to set up a host and connect to it, that will probably suffice.)
> 
> I think there must also be a call to create a 0666 shm segment somewhere in
the
> extension code, because I still get one. However, it is only when I actually
try
> to Share my desktop — previously, I got a 0666 shm segment just because I
> installed the extension.

Can you clarify what you mean by "installed the extension"? Just adding the
web-app to Chrome should not create any shared memory segments (at least, our
app is no different from any other at that point), so I'm curious as to exactly
what you were doing when the segment was created.

[Wez, 2012-09-06 22:50:56]

On 2012/09/06 22:26:28, Chris P. wrote:
> > Responding to your ping about how to test this, your best bet is to verify
> that
> > the permissions are correct using the approach outlined in the bug. If so,
> then
> > assign it to be to verify that the capture still works (or if you're
familiar
> > with how to set up a host and connect to it, that will probably suffice.)
> 
> I think there must also be a call to create a 0666 shm segment somewhere in
the
> extension code, because I still get one. However, it is only when I actually
try
> to Share my desktop — previously, I got a 0666 shm segment just because I
> installed the extension. So I think this CL somewhat improves the situation,
but
> does not completely solve the problem.
> 
> Jamie, can I get a look at the extension code or can you look into it for me?
> Thanks.

The X server won't map a shared memory segment with permissions of 0600, will
it? (See ui/surface/transport_dib_linux.cc:42)

Jamie, have you actually successfully run a host with this patch in place
against a console (i.e. running as root) X server? 

All the segments you'll see created by Chrome (e.g. used to pass image data
between renderers and the browser) will have permissions 0666 on Linux, and
there may be segments created by Chrome's plugin support code, and by GTK.

[Wez, 2012-09-06 22:52:31]

On 2012/09/06 22:50:56, Wez wrote:
> Jamie, have you actually successfully run a host with this patch in place
> against a console (i.e. running as root) X server? 

Sorry; that question is for Chris!

[palmer, 2012-09-06 22:53:24]

> The X server won't map a shared memory segment with permissions of 0600, will
> it? (See ui/surface/transport_dib_linux.cc:42)

See also: https://codereview.chromium.org/10854242/

I find that it works perfectly well.

> All the segments you'll see created by Chrome (e.g. used to pass image data
> between renderers and the browser) will have permissions 0666 on Linux, and
> there may be segments created by Chrome's plugin support code, and by GTK.

Right, I am trying to fix those too. So far it seems like it should be able to
work.

[palmer, 2012-09-06 22:59:18]

> Can you clarify what you mean by "installed the extension"? Just adding the
> web-app to Chrome should not create any shared memory segments (at least, our
> app is no different from any other at that point), so I'm curious as to
exactly
> what you were doing when the segment was created.

I did two things:

1. In about:plugins, set Chrome Remote Desktop Viewer (Chromoting Viewer in a
debug/ToT build) to [x] Always allowed

2. Install the Chromoting extension from CWS.

I think, previously, that that alone caused a new 0666 segment to appear. But
perhaps it was appearing for some other reason.

Anyway, now, with and without this patch, I only get any 0666 segments when
actually Sharing. So some other code I can't see is doing it. (See also
https://codereview.chromium.org/10854242/)

[Wez, 2012-09-06 22:59:48]

On 2012/09/06 22:53:24, Chris P. wrote:
> > The X server won't map a shared memory segment with permissions of 0600,
will
> > it? (See ui/surface/transport_dib_linux.cc:42)
> 
> See also: https://codereview.chromium.org/10854242/
> 
> I find that it works perfectly well.
> 
> > All the segments you'll see created by Chrome (e.g. used to pass image data
> > between renderers and the browser) will have permissions 0666 on Linux, and
> > there may be segments created by Chrome's plugin support code, and by GTK.
> 
> Right, I am trying to fix those too. So far it seems like it should be able to
> work.

Are you testing against a console X server running as root, though, or against
something else?  The comments on the CL you link to don't seem to indicate that
it works, but rather that your patch only works if the X server happens to be
running under the same account as the X client, which won't normally be the case
on the console.

[Wez, 2012-09-06 23:13:45]

On 2012/09/06 22:59:48, Wez wrote:
> On 2012/09/06 22:53:24, Chris P. wrote:
> > > The X server won't map a shared memory segment with permissions of 0600,
> will
> > > it? (See ui/surface/transport_dib_linux.cc:42)
> > 
> > See also: https://codereview.chromium.org/10854242/
> > 
> > I find that it works perfectly well.
> > 
> > > All the segments you'll see created by Chrome (e.g. used to pass image
data
> > > between renderers and the browser) will have permissions 0666 on Linux,
and
> > > there may be segments created by Chrome's plugin support code, and by GTK.
> > 
> > Right, I am trying to fix those too. So far it seems like it should be able
to
> > work.
> 
> Are you testing against a console X server running as root, though, or against
> something else?  The comments on the CL you link to don't seem to indicate
that
> it works, but rather that your patch only works if the X server happens to be
> running under the same account as the X client, which won't normally be the
case
> on the console.

I've added a link to Xorg's access-check code to the other CL, FYI.  At a
first-glance it looks like 0666 is still required for segments that the X server
will write to, but read-only segments could be created 0x644, for example.

[palmer, 2012-09-06 23:16:37]

> > Jamie, have you actually successfully run a host with this patch in place
> > against a console (i.e. running as root) X server? 
> 
> Sorry; that question is for Chris!

With normal Goobuntu — X as root on the console — I can offer my display for
remote access and access it from my Windows machine's Chrome. It seems to work
just fine.

In the other direction, both with this patch (Debug/ToT) and without (official
Release), I could not see the Windows desktop in my Chrome window — but Windows
claimed that palmer@chromium.org *was* seeing the desktop. Eventually, both with
and without the patch, Chrome says "Unable to reach the host. This is probably
due to the configuration of the network you are using." Hmm, maybe I need to
disable my Windows firewall. Anyway, the breakage seems unrelated to this patch.

Should I try other tests too?

[palmer, 2012-09-06 23:20:06]

Yeah it was the Windows firewall. I can share the desktop in both directions, no
apparent problems.

[Wez, 2012-09-06 23:26:04]

On 2012/09/06 23:16:37, Chris P. wrote:
> > > Jamie, have you actually successfully run a host with this patch in place
> > > against a console (i.e. running as root) X server? 
> > 
> > Sorry; that question is for Chris!
> 
> With normal Goobuntu — X as root on the console — I can offer my display for
> remote access and access it from my Windows machine's Chrome. It seems to work
> just fine.

That's expected; X clients can't rely on being able to use XShm so they will
generally fall-back to the standard X APIs, which pass the data over the
client's Unix-domain or TCP socket connection to the X server.

To verify that shared memory is working as intended you'll need to add some
print-out in the fall-back codepath, or actually measure rendering performance;
simply checking whether rendering succeeds isn't sufficient.

> In the other direction, both with this patch (Debug/ToT) and without (official
> Release), I could not see the Windows desktop in my Chrome window — but
Windows
> claimed that mailto:palmer@chromium.org *was* seeing the desktop. Eventually,
both with
> and without the patch, Chrome says "Unable to reach the host. This is probably
> due to the configuration of the network you are using." Hmm, maybe I need to
> disable my Windows firewall. Anyway, the breakage seems unrelated to this
patch.

Sounds unrelated to this change; feel free to file a bug at new.crbug.com.

> Should I try other tests too?

See above. :)

[palmer, 2012-09-06 23:34:26]

> That's expected; X clients can't rely on being able to use XShm so they will
> generally fall-back to the standard X APIs, which pass the data over the
> client's Unix-domain or TCP socket connection to the X server.
> 
> To verify that shared memory is working as intended you'll need to add some
> print-out in the fall-back codepath, or actually measure rendering
performance;
> simply checking whether rendering succeeds isn't sufficient.

Yes, I know. During development, I added DLOGs in this and in the other CL that
showed that shared memory was still working normally. If you like, I can modify
these patches to have LOG(INFO) in case of fall-back.

Also, just now, playing Robot Unicorn Attack from a remote Chrome seemed to show
similar frame rates with and without this patch.

[commit-bot: I haz the power, 2012-09-07 22:48:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/10854241/5002

[commit-bot: I haz the power, 2012-09-08 01:17:15]

Change committed as 155546