Make shared memory segments writable only by their rightful owners.

Make shared memory segments writable only by their rightful owners.

BUG=143859
TEST=Chrome's UI still works on Linux and Chrome OS
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=158289

[palmer, 2012-08-21 02:05:27]

PTAL. Thank you!

[agl, 2012-08-21 02:14:00]

The fact that X needs 0666 memory was certainly true in the past. I'm afraid
that this needs more investigation. What Linux did you test it on? Did this
change with the switch to non-root X, and has that completed on all the
platforms we support?

[palmer, 2012-08-30 20:45:05]

On 2012/08/21 02:14:00, agl wrote:
> The fact that X needs 0666 memory was certainly true in the past. I'm afraid
> that this needs more investigation. What Linux did you test it on? Did this
> change with the switch to non-root X, and has that completed on all the
> platforms we support?

As mentioned in the bug, gLucid and gPrecise both have X running as root. I did
find this:

https://wiki.ubuntu.com/X/Rootless

CCing Kees to see if he can tell us what is up. I can't really get a clear story
about how widespread/default/well-supported/working this is, from this document.

[palmer, 2012-08-30 20:45:43]

Kees, thoughts about the viability of non-root X, and/or about this patch in
general? Thanks.

[keescook, 2012-08-30 22:22:00]

On 2012/08/30 20:45:43, Chris P. wrote:
> Kees, thoughts about the viability of non-root X, and/or about this patch in
> general? Thanks.

Most of the problems with non-root X come in the form of dealing with the input
devices sanely. Everything else is ready (mostly due to KMS). Generally,
rootless X should work fine, though it's still (obviously) a relatively new way
to do things.

As for this patch, it seems fine, but the comment being removed seems odd to me,
like we're overlooking something here. I'm not sure why the 0666 was used --
that seems to have nothing to do with the fact that X with appropriate privs
could read an arbitrary shm segment. That seems entirely separate from the mode.

What is this shm used for?

[agl, 2012-08-30 22:27:23]

On Thu, Aug 30, 2012 at 6:22 PM,  <keescook@google.com> wrote:
> What is this shm used for?

SHM is used so that the renderers can write into a memory segment
that's shared directly with the X server.

Far back in the mists of time, X wouldn't attach to a shared memory
segment that wasn't 0666. That might be superfluous now (or might have
been totally bogus then!)

Cheers

AGL

[agl, 2012-08-30 22:27:23]

On Thu, Aug 30, 2012 at 6:22 PM,  <keescook@google.com> wrote:
> What is this shm used for?

SHM is used so that the renderers can write into a memory segment
that's shared directly with the X server.

Far back in the mists of time, X wouldn't attach to a shared memory
segment that wasn't 0666. That might be superfluous now (or might have
been totally bogus then!)

Cheers

AGL

[palmer, 2012-08-30 22:35:08]

So the next question is, what UID will non-root X run as? If it runs as the user
that runs Chrome, we are solid, and 0600 should work as it does now with
root-owned X.

If X runs as some dedicated third user, say "xorg", then X won't be able to
write to a shared memory segment owned and writable only by palmer. It sounds
like we could — in principle, and if it turns out to be necessary — make the
segment 0660 and use shmctl(2) to set the GID to xorg's GID.

We would obviously need some portable way to discover the UID/GID that runs the
X process. I don't know what that would be. I am hoping Kees will say "X runs as
the user running Chrome". :)

jln also would like for us to investigate using POSIX IPC instead of Sys V IPC,
if that is possible. Can X even handle that? (Obviously that's an issue for
another CL, in any case.)

[palmer, 2012-08-30 22:35:31]

CCing jln for thoughts/info.

[Wez, 2012-09-06 23:10:23]

FWIW, the access-check appears to be applied to XShmAttach in Xorg here:

http://cgit.freedesktop.org/xorg/xserver/tree/Xext/shm.c#n416

[palmer, 2012-09-07 19:06:21]

> FWIW, the access-check appears to be applied to XShmAttach in Xorg here:
> 
> http://cgit.freedesktop.org/xorg/xserver/tree/Xext/shm.c#n416

That check appears to succeed — i.e. access is allowed — due to massive #ifdef
gyrations in GetLocalClientCreds (see
http://cgit.freedesktop.org/xorg/xserver/tree/os/access.c). Using
DISPLAY=machinename:0, and NX from the host itself and from a remote host,
results in XShmAttach success (according to the verbose logging I added).

The only time XShmAttach fails is when I ssh -X from my Mac to my Linux. That's
expected, of course. And Chrome still works in this scenario, it's just slow.
Much of the slowness is due to network latency, of course (I am WFH today).

So I haven't yet seen a scenario in which the reduced permissions causes an
XShmAttach failure when it should have succeeded. If anyone can cook one up,
I'll try to tweak this patch to first try with 0600 then fall back on 0666 so
that in normal scenarios people don't expose their shared memory when they don't
have to. If nobody can cook up a plausible failure scenario, then I think this
CL should go through.

[palmer, 2012-09-07 19:07:07]

Adding cevans and jorgelo in case they have additional insight.

[agl, 2012-09-07 19:08:07]

lgtm

[commit-bot: I haz the power, 2012-09-07 22:47:54]

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

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

Presubmit check for 10854242-6006 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  ui/base/x/x11_util.cc:1326
    void InitXKeyEventForTesting(EventType type,

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/renderer_host
    ui/surface
    ui/base/x

Presubmit checks took 1.2s to calculate.

[palmer, 2012-09-07 22:54:21]

Adding kbr for ui/surface/OWNERS

ben for content/browser/renderer_host/OWNERS

derat and sadrul for ui/base/x/OWNERS 

Thank you!

[palmer, 2012-09-13 21:38:03]

Hmm, somehow I failed to actually add kbr and derat. :]

[Ken Russell (switch to Gerrit), 2012-09-13 21:47:45]

LGTM as long as this has been well tested.

[Daniel Erat, 2012-09-13 22:04:39]

https://codereview.chromium.org/10854242/diff/6006/content/browser/renderer_h...
File content/browser/renderer_host/backing_store_gtk.cc (right):

https://codereview.chromium.org/10854242/diff/6006/content/browser/renderer_h...
content/browser/renderer_host/backing_store_gtk.cc:525: LOG(WARNING) << "Failed
to get shared memory segment. "
How frequently can this be logged?

[Daniel Erat, 2012-09-13 22:36:50]

(LGTM if the logging question isn't an issue; just wanted to make sure that
we're not logging on every update)

[commit-bot: I haz the power, 2012-09-22 06:35:55]

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

[commit-bot: I haz the power, 2012-09-22 06:36:04]

Presubmit check for 10854242-6006 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  ui/base/x/x11_util.cc:1398
    void InitXKeyEventForTesting(EventType type,

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/renderer_host

Presubmit checks took 1.2s to calculate.

[palmer, 2012-09-24 16:26:10]

https://codereview.chromium.org/10854242/diff/6006/content/browser/renderer_h...
File content/browser/renderer_host/backing_store_gtk.cc (right):

https://codereview.chromium.org/10854242/diff/6006/content/browser/renderer_h...
content/browser/renderer_host/backing_store_gtk.cc:525: LOG(WARNING) << "Failed
to get shared memory segment. "
> How frequently can this be logged?

This one, once. Some of the other logging statements happen more often, but I'd
like to leave them for now in case we do hit upon a real-life case where
XShmAttach fails during the canary/dev bug shake-out.